| Message ID | 20231003115237.76828-2-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Generic line based metadata support, internal pads | expand |
On 03/10/2023 14:52, Sakari Ailus wrote: > Internal source pads will be used as routing endpoints in V4L2 > [GS]_ROUTING IOCTLs, to indicate that the stream begins in the entity. It would probably be good to explain here and in the comments/docs, that a sink pad is a source pad =). When you say above "internal source pad", it's actually MEDIA_PAD_FL_INTERNAL | MEDIA_PAD_FL_SINK. I think this will confuse people time and time again, so it's probably good to explain it in as many places as possible. > Also prevent creating links to pads that have been flagged as internal and > initialising source pads with internal flag set. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > Documentation/userspace-api/media/glossary.rst | 6 ++++++ > .../userspace-api/media/mediactl/media-types.rst | 6 ++++++ > drivers/media/mc/mc-entity.c | 10 ++++++++-- > include/uapi/linux/media.h | 1 + > 4 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/Documentation/userspace-api/media/glossary.rst b/Documentation/userspace-api/media/glossary.rst > index 96a360edbf3b..f7b99a4527c7 100644 > --- a/Documentation/userspace-api/media/glossary.rst > +++ b/Documentation/userspace-api/media/glossary.rst > @@ -173,6 +173,12 @@ Glossary > An integrated circuit that integrates all components of a computer > or other electronic systems. > > +_media-glossary-stream: > + Stream > + A distinct flow of data (image data or metadata) over a media pipeline > + from source to sink. A source may be e.g. an image sensor and a sink > + e.g. a memory buffer. > + > V4L2 API > **V4L2 userspace API** > > diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst > index 0ffeece1e0c8..28941da27790 100644 > --- a/Documentation/userspace-api/media/mediactl/media-types.rst > +++ b/Documentation/userspace-api/media/mediactl/media-types.rst > @@ -361,6 +361,7 @@ Types and flags used to represent the media graph elements > .. _MEDIA-PAD-FL-SINK: > .. _MEDIA-PAD-FL-SOURCE: > .. _MEDIA-PAD-FL-MUST-CONNECT: > +.. _MEDIA-PAD-FL-INTERNAL: > > .. flat-table:: Media pad flags > :header-rows: 0 > @@ -382,6 +383,11 @@ Types and flags used to represent the media graph elements > when this flag isn't set; the absence of the flag doesn't imply > there is none. > > + * - ``MEDIA_PAD_FL_INTERNAL`` > + - The internal flag indicates an internal pad that has no external > + connections. Such a pad shall not be connected with a link. The > + internal flag indicates that the :ref:``stream > + <media-glossary-stream>`` either starts or ends in the entity. > > One and only one of ``MEDIA_PAD_FL_SINK`` and ``MEDIA_PAD_FL_SOURCE`` > must be set for every pad. > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > index 543a392f8635..f5f290781021 100644 > --- a/drivers/media/mc/mc-entity.c > +++ b/drivers/media/mc/mc-entity.c > @@ -213,7 +213,9 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads, > iter->index = i++; > > if (hweight32(iter->flags & (MEDIA_PAD_FL_SINK | > - MEDIA_PAD_FL_SOURCE)) != 1) { > + MEDIA_PAD_FL_SOURCE)) != 1 || > + (iter->flags & MEDIA_PAD_FL_INTERNAL && > + !(iter->flags & MEDIA_PAD_FL_SINK))) { > ret = -EINVAL; > break; Would it make sense to have this "only internal-sinks supported"-check as a separate check, with an error print? It is a valid thing to do, we just want to disable it for the time being. > } > @@ -1075,7 +1077,8 @@ int media_get_pad_index(struct media_entity *entity, u32 pad_type, > > for (i = 0; i < entity->num_pads; i++) { > if ((entity->pads[i].flags & > - (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_SOURCE)) != pad_type) > + (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_SOURCE | > + MEDIA_PAD_FL_INTERNAL)) != pad_type) > continue; > > if (entity->pads[i].sig_type == sig_type) > @@ -1100,6 +1103,9 @@ media_create_pad_link(struct media_entity *source, u16 source_pad, > return -EINVAL; > if (WARN_ON(!(sink->pads[sink_pad].flags & MEDIA_PAD_FL_SINK))) > return -EINVAL; > + if (WARN_ON(source->pads[source_pad].flags & MEDIA_PAD_FL_INTERNAL) || > + WARN_ON(source->pads[sink_pad].flags & MEDIA_PAD_FL_INTERNAL)) > + return -EINVAL; > > link = media_add_link(&source->links); > if (link == NULL) > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h > index 1c80b1d6bbaf..80cfd12a43fc 100644 > --- a/include/uapi/linux/media.h > +++ b/include/uapi/linux/media.h > @@ -208,6 +208,7 @@ struct media_entity_desc { > #define MEDIA_PAD_FL_SINK (1U << 0) > #define MEDIA_PAD_FL_SOURCE (1U << 1) > #define MEDIA_PAD_FL_MUST_CONNECT (1U << 2) > +#define MEDIA_PAD_FL_INTERNAL (1U << 3) > > struct media_pad_desc { > __u32 entity; /* entity ID */
On Thu, Oct 05, 2023 at 10:22:42AM +0000, Sakari Ailus wrote: > > Note that "media pipeline" isn't defined either. Should that be added? > > I can add that, too... Instead I'll remove it altogether: it's not needed here.
Moi, Thanks for the review. On Thu, Oct 05, 2023 at 02:04:55PM +0300, Tomi Valkeinen wrote: > On 03/10/2023 14:52, Sakari Ailus wrote: > > Internal source pads will be used as routing endpoints in V4L2 > > [GS]_ROUTING IOCTLs, to indicate that the stream begins in the entity. > > It would probably be good to explain here and in the comments/docs, that a > sink pad is a source pad =). When you say above "internal source pad", it's > actually MEDIA_PAD_FL_INTERNAL | MEDIA_PAD_FL_SINK. I think this will > confuse people time and time again, so it's probably good to explain it in > as many places as possible. I'll add it here. > > > Also prevent creating links to pads that have been flagged as internal and > > initialising source pads with internal flag set. > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > Documentation/userspace-api/media/glossary.rst | 6 ++++++ > > .../userspace-api/media/mediactl/media-types.rst | 6 ++++++ > > drivers/media/mc/mc-entity.c | 10 ++++++++-- > > include/uapi/linux/media.h | 1 + > > 4 files changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/userspace-api/media/glossary.rst b/Documentation/userspace-api/media/glossary.rst > > index 96a360edbf3b..f7b99a4527c7 100644 > > --- a/Documentation/userspace-api/media/glossary.rst > > +++ b/Documentation/userspace-api/media/glossary.rst > > @@ -173,6 +173,12 @@ Glossary > > An integrated circuit that integrates all components of a computer > > or other electronic systems. > > +_media-glossary-stream: > > + Stream > > + A distinct flow of data (image data or metadata) over a media pipeline > > + from source to sink. A source may be e.g. an image sensor and a sink > > + e.g. a memory buffer. > > + > > V4L2 API > > **V4L2 userspace API** > > diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst > > index 0ffeece1e0c8..28941da27790 100644 > > --- a/Documentation/userspace-api/media/mediactl/media-types.rst > > +++ b/Documentation/userspace-api/media/mediactl/media-types.rst > > @@ -361,6 +361,7 @@ Types and flags used to represent the media graph elements > > .. _MEDIA-PAD-FL-SINK: > > .. _MEDIA-PAD-FL-SOURCE: > > .. _MEDIA-PAD-FL-MUST-CONNECT: > > +.. _MEDIA-PAD-FL-INTERNAL: > > .. flat-table:: Media pad flags > > :header-rows: 0 > > @@ -382,6 +383,11 @@ Types and flags used to represent the media graph elements > > when this flag isn't set; the absence of the flag doesn't imply > > there is none. > > + * - ``MEDIA_PAD_FL_INTERNAL`` > > + - The internal flag indicates an internal pad that has no external > > + connections. Such a pad shall not be connected with a link. The > > + internal flag indicates that the :ref:``stream > > + <media-glossary-stream>`` either starts or ends in the entity. > > One and only one of ``MEDIA_PAD_FL_SINK`` and ``MEDIA_PAD_FL_SOURCE`` > > must be set for every pad. > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > > index 543a392f8635..f5f290781021 100644 > > --- a/drivers/media/mc/mc-entity.c > > +++ b/drivers/media/mc/mc-entity.c > > @@ -213,7 +213,9 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads, > > iter->index = i++; > > if (hweight32(iter->flags & (MEDIA_PAD_FL_SINK | > > - MEDIA_PAD_FL_SOURCE)) != 1) { > > + MEDIA_PAD_FL_SOURCE)) != 1 || > > + (iter->flags & MEDIA_PAD_FL_INTERNAL && > > + !(iter->flags & MEDIA_PAD_FL_SINK))) { > > ret = -EINVAL; > > break; > > Would it make sense to have this "only internal-sinks supported"-check as a > separate check, with an error print? It is a valid thing to do, we just want > to disable it for the time being. This would be a driver bug. We don't have such complaints on setting both sink and source flags either, or similar things in the V4L2 framework. I'd leave it as-is. Maybe a WARN_ON() as we have in some cases below for creating links? > > > } > > @@ -1075,7 +1077,8 @@ int media_get_pad_index(struct media_entity *entity, u32 pad_type, > > for (i = 0; i < entity->num_pads; i++) { > > if ((entity->pads[i].flags & > > - (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_SOURCE)) != pad_type) > > + (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_SOURCE | > > + MEDIA_PAD_FL_INTERNAL)) != pad_type) > > continue; > > if (entity->pads[i].sig_type == sig_type) > > @@ -1100,6 +1103,9 @@ media_create_pad_link(struct media_entity *source, u16 source_pad, > > return -EINVAL; > > if (WARN_ON(!(sink->pads[sink_pad].flags & MEDIA_PAD_FL_SINK))) > > return -EINVAL; > > + if (WARN_ON(source->pads[source_pad].flags & MEDIA_PAD_FL_INTERNAL) || > > + WARN_ON(source->pads[sink_pad].flags & MEDIA_PAD_FL_INTERNAL)) > > + return -EINVAL; > > link = media_add_link(&source->links); > > if (link == NULL) > > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h > > index 1c80b1d6bbaf..80cfd12a43fc 100644 > > --- a/include/uapi/linux/media.h > > +++ b/include/uapi/linux/media.h > > @@ -208,6 +208,7 @@ struct media_entity_desc { > > #define MEDIA_PAD_FL_SINK (1U << 0) > > #define MEDIA_PAD_FL_SOURCE (1U << 1) > > #define MEDIA_PAD_FL_MUST_CONNECT (1U << 2) > > +#define MEDIA_PAD_FL_INTERNAL (1U << 3) > > struct media_pad_desc { > > __u32 entity; /* entity ID */ >
Hi Hans, On Thu, Oct 05, 2023 at 01:16:27PM +0200, Hans Verkuil wrote: > On 05/10/2023 12:22, Sakari Ailus wrote: > > Hi Hans, > > > > Thanks for the review! > > > > On Thu, Oct 05, 2023 at 11:52:08AM +0200, Hans Verkuil wrote: > >> On 03/10/2023 13:52, Sakari Ailus wrote: > >>> Internal source pads will be used as routing endpoints in V4L2 > >>> [GS]_ROUTING IOCTLs, to indicate that the stream begins in the entity. > >>> > >>> Also prevent creating links to pads that have been flagged as internal and > >>> initialising source pads with internal flag set. > >>> > >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >>> --- > >>> Documentation/userspace-api/media/glossary.rst | 6 ++++++ > >>> .../userspace-api/media/mediactl/media-types.rst | 6 ++++++ > >>> drivers/media/mc/mc-entity.c | 10 ++++++++-- > >>> include/uapi/linux/media.h | 1 + > >>> 4 files changed, 21 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/Documentation/userspace-api/media/glossary.rst b/Documentation/userspace-api/media/glossary.rst > >>> index 96a360edbf3b..f7b99a4527c7 100644 > >>> --- a/Documentation/userspace-api/media/glossary.rst > >>> +++ b/Documentation/userspace-api/media/glossary.rst > >>> @@ -173,6 +173,12 @@ Glossary > >>> An integrated circuit that integrates all components of a computer > >>> or other electronic systems. > >>> > >>> +_media-glossary-stream: > >>> + Stream > >>> + A distinct flow of data (image data or metadata) over a media pipeline > >>> + from source to sink. A source may be e.g. an image sensor and a sink > >>> + e.g. a memory buffer. > >> > >> Hmm, I think this is a bit confusing. I think it would be better to replace > >> "from source to sink" with "from the initial source to the final sink". > > > > Seems fine to me. I'll use that in v7. > > > >> > >> The original text doesn't make it clear that there can be many hops in > >> between. > >> > >> So also: "A source" -> "An initial source", and "a sink" -> "a final sink". > >> > >> Note that "media pipeline" isn't defined either. Should that be added? > > > > I can add that, too... > > > >> > >> Finally, I think this should be a separate patch as it has nothing to do with > >> adding the INTERNAL pad flag. > > > > I agree, will split in v7. > > > >> > >>> + > >>> V4L2 API > >>> **V4L2 userspace API** > >>> > >>> diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst > >>> index 0ffeece1e0c8..28941da27790 100644 > >>> --- a/Documentation/userspace-api/media/mediactl/media-types.rst > >>> +++ b/Documentation/userspace-api/media/mediactl/media-types.rst > >>> @@ -361,6 +361,7 @@ Types and flags used to represent the media graph elements > >>> .. _MEDIA-PAD-FL-SINK: > >>> .. _MEDIA-PAD-FL-SOURCE: > >>> .. _MEDIA-PAD-FL-MUST-CONNECT: > >>> +.. _MEDIA-PAD-FL-INTERNAL: > >>> > >>> .. flat-table:: Media pad flags > >>> :header-rows: 0 > >>> @@ -382,6 +383,11 @@ Types and flags used to represent the media graph elements > >>> when this flag isn't set; the absence of the flag doesn't imply > >>> there is none. > >>> > >>> + * - ``MEDIA_PAD_FL_INTERNAL`` > >>> + - The internal flag indicates an internal pad that has no external > >>> + connections. Such a pad shall not be connected with a link. The > >>> + internal flag indicates that the :ref:``stream > >>> + <media-glossary-stream>`` either starts or ends in the entity. > >> > >> This suggests that INTERNAL can be used for both sinks and sources, but... > > > > Right. The INTERNAL flag in UAPI shouldn't be limited to sources, but at > > the same time we don't have use for it in sinks. I'd prefer to leave this > > open in the future. We could of course say that explicitly. > > I think it is better to mention that it is for streams that start in the > entity only, but that in the future it might be extended to support streams > that end in the entity. > > When we add support for that, we need to update the documentation as well, > at minimum with one or more examples of how that would be used. I'll use this instead of the original sentence: The internal flag may currently be present only in a source pad only where it indicates that the :ref:``stream <media-glossary-stream>`` originates from within the entity.
diff --git a/Documentation/userspace-api/media/glossary.rst b/Documentation/userspace-api/media/glossary.rst index 96a360edbf3b..f7b99a4527c7 100644 --- a/Documentation/userspace-api/media/glossary.rst +++ b/Documentation/userspace-api/media/glossary.rst @@ -173,6 +173,12 @@ Glossary An integrated circuit that integrates all components of a computer or other electronic systems. +_media-glossary-stream: + Stream + A distinct flow of data (image data or metadata) over a media pipeline + from source to sink. A source may be e.g. an image sensor and a sink + e.g. a memory buffer. + V4L2 API **V4L2 userspace API** diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst index 0ffeece1e0c8..28941da27790 100644 --- a/Documentation/userspace-api/media/mediactl/media-types.rst +++ b/Documentation/userspace-api/media/mediactl/media-types.rst @@ -361,6 +361,7 @@ Types and flags used to represent the media graph elements .. _MEDIA-PAD-FL-SINK: .. _MEDIA-PAD-FL-SOURCE: .. _MEDIA-PAD-FL-MUST-CONNECT: +.. _MEDIA-PAD-FL-INTERNAL: .. flat-table:: Media pad flags :header-rows: 0 @@ -382,6 +383,11 @@ Types and flags used to represent the media graph elements when this flag isn't set; the absence of the flag doesn't imply there is none. + * - ``MEDIA_PAD_FL_INTERNAL`` + - The internal flag indicates an internal pad that has no external + connections. Such a pad shall not be connected with a link. The + internal flag indicates that the :ref:``stream + <media-glossary-stream>`` either starts or ends in the entity. One and only one of ``MEDIA_PAD_FL_SINK`` and ``MEDIA_PAD_FL_SOURCE`` must be set for every pad. diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c index 543a392f8635..f5f290781021 100644 --- a/drivers/media/mc/mc-entity.c +++ b/drivers/media/mc/mc-entity.c @@ -213,7 +213,9 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads, iter->index = i++; if (hweight32(iter->flags & (MEDIA_PAD_FL_SINK | - MEDIA_PAD_FL_SOURCE)) != 1) { + MEDIA_PAD_FL_SOURCE)) != 1 || + (iter->flags & MEDIA_PAD_FL_INTERNAL && + !(iter->flags & MEDIA_PAD_FL_SINK))) { ret = -EINVAL; break; } @@ -1075,7 +1077,8 @@ int media_get_pad_index(struct media_entity *entity, u32 pad_type, for (i = 0; i < entity->num_pads; i++) { if ((entity->pads[i].flags & - (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_SOURCE)) != pad_type) + (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_SOURCE | + MEDIA_PAD_FL_INTERNAL)) != pad_type) continue; if (entity->pads[i].sig_type == sig_type) @@ -1100,6 +1103,9 @@ media_create_pad_link(struct media_entity *source, u16 source_pad, return -EINVAL; if (WARN_ON(!(sink->pads[sink_pad].flags & MEDIA_PAD_FL_SINK))) return -EINVAL; + if (WARN_ON(source->pads[source_pad].flags & MEDIA_PAD_FL_INTERNAL) || + WARN_ON(source->pads[sink_pad].flags & MEDIA_PAD_FL_INTERNAL)) + return -EINVAL; link = media_add_link(&source->links); if (link == NULL) diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h index 1c80b1d6bbaf..80cfd12a43fc 100644 --- a/include/uapi/linux/media.h +++ b/include/uapi/linux/media.h @@ -208,6 +208,7 @@ struct media_entity_desc { #define MEDIA_PAD_FL_SINK (1U << 0) #define MEDIA_PAD_FL_SOURCE (1U << 1) #define MEDIA_PAD_FL_MUST_CONNECT (1U << 2) +#define MEDIA_PAD_FL_INTERNAL (1U << 3) struct media_pad_desc { __u32 entity; /* entity ID */
Internal source pads will be used as routing endpoints in V4L2 [GS]_ROUTING IOCTLs, to indicate that the stream begins in the entity. Also prevent creating links to pads that have been flagged as internal and initialising source pads with internal flag set. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- Documentation/userspace-api/media/glossary.rst | 6 ++++++ .../userspace-api/media/mediactl/media-types.rst | 6 ++++++ drivers/media/mc/mc-entity.c | 10 ++++++++-- include/uapi/linux/media.h | 1 + 4 files changed, 21 insertions(+), 2 deletions(-)