| Message ID | 20230808075538.3043934-4-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Generic line based metadata support, internal pads | expand |
Hi Sakari On Tue, Aug 08, 2023 at 10:55:31AM +0300, 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. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > Documentation/userspace-api/media/glossary.rst | 6 ++++++ > Documentation/userspace-api/media/mediactl/media-types.rst | 6 ++++++ > drivers/media/mc/mc-entity.c | 6 +++++- > include/uapi/linux/media.h | 1 + > 4 files changed, 18 insertions(+), 1 deletion(-) > > 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 4991281dcccc..03a9e0b8ebab 100644 > --- a/drivers/media/mc/mc-entity.c > +++ b/drivers/media/mc/mc-entity.c > @@ -1071,7 +1071,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) > @@ -1094,6 +1095,9 @@ media_create_pad_link(struct media_entity *source, u16 source_pad, > return -EINVAL; > if (WARN_ON(!(source->pads[source_pad].flags & MEDIA_PAD_FL_SOURCE))) > return -EINVAL; > + if (WARN_ON(source->pads[source_pad].flags & MEDIA_PAD_FL_SOURCE && Why do you need to check for this again here ? Hasn't the previous if() clause just validated that source->pads[source_pad].flags & MEDIA_PAD_FL_SOURCE ? > + source->pads[source_pad].flags & MEDIA_PAD_FL_INTERNAL)) > + return -EINVAL; > if (WARN_ON(!(sink->pads[sink_pad].flags & MEDIA_PAD_FL_SINK))) > return -EINVAL; > > 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 */ > -- > 2.39.2 >
Hi Jacopo, Thanks for the review. On Thu, Aug 10, 2023 at 04:12:18PM +0200, Jacopo Mondi wrote: > Hi Sakari > > On Tue, Aug 08, 2023 at 10:55:31AM +0300, 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. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > Documentation/userspace-api/media/glossary.rst | 6 ++++++ > > Documentation/userspace-api/media/mediactl/media-types.rst | 6 ++++++ > > drivers/media/mc/mc-entity.c | 6 +++++- > > include/uapi/linux/media.h | 1 + > > 4 files changed, 18 insertions(+), 1 deletion(-) > > > > 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 4991281dcccc..03a9e0b8ebab 100644 > > --- a/drivers/media/mc/mc-entity.c > > +++ b/drivers/media/mc/mc-entity.c > > @@ -1071,7 +1071,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) > > @@ -1094,6 +1095,9 @@ media_create_pad_link(struct media_entity *source, u16 source_pad, > > return -EINVAL; > > if (WARN_ON(!(source->pads[source_pad].flags & MEDIA_PAD_FL_SOURCE))) > > return -EINVAL; > > + if (WARN_ON(source->pads[source_pad].flags & MEDIA_PAD_FL_SOURCE && > > Why do you need to check for this again here ? Hasn't the previous > if() clause just validated that > > source->pads[source_pad].flags & MEDIA_PAD_FL_SOURCE ? Yes, this needs to be changed. As a link is created here, the internal flag shouldn't be present in neither sink nor source pads. I may have forgotten to address this previously, I'll do this for v4. > > > + source->pads[source_pad].flags & MEDIA_PAD_FL_INTERNAL)) > > + return -EINVAL; > > if (WARN_ON(!(sink->pads[sink_pad].flags & MEDIA_PAD_FL_SINK))) > > return -EINVAL; > > > > 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 Tue, Aug 08, 2023 at 10:15:45AM +0200, Hans Verkuil wrote: > On 08/08/2023 09:55, 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. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > Documentation/userspace-api/media/glossary.rst | 6 ++++++ > > Documentation/userspace-api/media/mediactl/media-types.rst | 6 ++++++ > > drivers/media/mc/mc-entity.c | 6 +++++- > > include/uapi/linux/media.h | 1 + > > 4 files changed, 18 insertions(+), 1 deletion(-) > > > > 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 4991281dcccc..03a9e0b8ebab 100644 > > --- a/drivers/media/mc/mc-entity.c > > +++ b/drivers/media/mc/mc-entity.c > > @@ -1071,7 +1071,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) > > @@ -1094,6 +1095,9 @@ media_create_pad_link(struct media_entity *source, u16 source_pad, > > return -EINVAL; > > if (WARN_ON(!(source->pads[source_pad].flags & MEDIA_PAD_FL_SOURCE))) > > return -EINVAL; > > + if (WARN_ON(source->pads[source_pad].flags & MEDIA_PAD_FL_SOURCE && > > + source->pads[source_pad].flags & MEDIA_PAD_FL_INTERNAL)) > > Even though this code is correct, I prefer to have () around the 'x & y' parts. > It avoids confusion for the reader. There actually should be a check here that neither of the pads have internal flag set --- internal pads may not be connected by a link. I'll fix that for v4. This was actually a leftover from the "internal source" flag. > > Up to you, though. I'd prefer to keep it as-is. :-)
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 4991281dcccc..03a9e0b8ebab 100644 --- a/drivers/media/mc/mc-entity.c +++ b/drivers/media/mc/mc-entity.c @@ -1071,7 +1071,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) @@ -1094,6 +1095,9 @@ media_create_pad_link(struct media_entity *source, u16 source_pad, return -EINVAL; if (WARN_ON(!(source->pads[source_pad].flags & MEDIA_PAD_FL_SOURCE))) return -EINVAL; + if (WARN_ON(source->pads[source_pad].flags & MEDIA_PAD_FL_SOURCE && + source->pads[source_pad].flags & MEDIA_PAD_FL_INTERNAL)) + return -EINVAL; if (WARN_ON(!(sink->pads[sink_pad].flags & MEDIA_PAD_FL_SINK))) return -EINVAL; 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. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- Documentation/userspace-api/media/glossary.rst | 6 ++++++ Documentation/userspace-api/media/mediactl/media-types.rst | 6 ++++++ drivers/media/mc/mc-entity.c | 6 +++++- include/uapi/linux/media.h | 1 + 4 files changed, 18 insertions(+), 1 deletion(-)