| Message ID | 20230505215257.60704-2-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Generic line based metadata support, internal pads | expand |
On 06/05/2023 00: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. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > .../userspace-api/media/mediactl/media-types.rst | 7 +++++++ > drivers/media/mc/mc-entity.c | 8 +++++++- > include/uapi/linux/media.h | 1 + > 3 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst > index 0ffeece1e0c8..c724139ad46c 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-SOURCE: > > .. flat-table:: Media pad flags > :header-rows: 0 > @@ -382,6 +383,12 @@ 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_SOURCE`` > + - This flag indicates an internal pad that has no external > + connections. Such a pad may not be connected with a link. The internal "must not"? Or "shall not"? Tomi
Hi Tomi, Thank you for the review. On Mon, May 08, 2023 at 12:52:24PM +0300, Tomi Valkeinen wrote: > On 06/05/2023 00: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. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > .../userspace-api/media/mediactl/media-types.rst | 7 +++++++ > > drivers/media/mc/mc-entity.c | 8 +++++++- > > include/uapi/linux/media.h | 1 + > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst > > index 0ffeece1e0c8..c724139ad46c 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-SOURCE: > > .. flat-table:: Media pad flags > > :header-rows: 0 > > @@ -382,6 +383,12 @@ 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_SOURCE`` > > + - This flag indicates an internal pad that has no external > > + connections. Such a pad may not be connected with a link. The internal > > "must not"? Or "shall not"? I think "may not" is appropriate. I'd be fine with shall, too, albeit it wouldn't change the meaning in practice.
On 08/05/2023 15:04, Sakari Ailus wrote: > Hi Tomi, > > Thank you for the review. > > On Mon, May 08, 2023 at 12:52:24PM +0300, Tomi Valkeinen wrote: >> On 06/05/2023 00: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. >>> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> --- >>> .../userspace-api/media/mediactl/media-types.rst | 7 +++++++ >>> drivers/media/mc/mc-entity.c | 8 +++++++- >>> include/uapi/linux/media.h | 1 + >>> 3 files changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst >>> index 0ffeece1e0c8..c724139ad46c 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-SOURCE: >>> .. flat-table:: Media pad flags >>> :header-rows: 0 >>> @@ -382,6 +383,12 @@ 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_SOURCE`` >>> + - This flag indicates an internal pad that has no external >>> + connections. Such a pad may not be connected with a link. The internal >> >> "must not"? Or "shall not"? > > I think "may not" is appropriate. I'd be fine with shall, too, albeit it > wouldn't change the meaning in practice. Ok. I don't speak standardize, and I'm not a native English speaker. But to me "may not be connected" sounds like it is possibly not connected, but also that it is possible for it to be connected. Tomi
On Mon, May 08, 2023 at 03:07:31PM +0300, Tomi Valkeinen wrote: > On 08/05/2023 15:04, Sakari Ailus wrote: > > Hi Tomi, > > > > Thank you for the review. > > > > On Mon, May 08, 2023 at 12:52:24PM +0300, Tomi Valkeinen wrote: > > > On 06/05/2023 00: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. > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > --- > > > > .../userspace-api/media/mediactl/media-types.rst | 7 +++++++ > > > > drivers/media/mc/mc-entity.c | 8 +++++++- > > > > include/uapi/linux/media.h | 1 + > > > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst > > > > index 0ffeece1e0c8..c724139ad46c 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-SOURCE: > > > > .. flat-table:: Media pad flags > > > > :header-rows: 0 > > > > @@ -382,6 +383,12 @@ 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_SOURCE`` > > > > + - This flag indicates an internal pad that has no external > > > > + connections. Such a pad may not be connected with a link. The internal > > > > > > "must not"? Or "shall not"? > > > > I think "may not" is appropriate. I'd be fine with shall, too, albeit it > > wouldn't change the meaning in practice. > > Ok. I don't speak standardize, and I'm not a native English speaker. But to > me "may not be connected" sounds like it is possibly not connected, but also > that it is possible for it to be connected. Ah, yes, I think you're actually right. But that is what I thought when writing the text. ;-) Shall we use shall, then?
Hi Sakari, Thank you for the patch. On Sat, May 06, 2023 at 12:52:51AM +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> > --- > .../userspace-api/media/mediactl/media-types.rst | 7 +++++++ > drivers/media/mc/mc-entity.c | 8 +++++++- > include/uapi/linux/media.h | 1 + > 3 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst > index 0ffeece1e0c8..c724139ad46c 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-SOURCE: > > .. flat-table:: Media pad flags > :header-rows: 0 > @@ -382,6 +383,12 @@ 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_SOURCE`` > + - This flag indicates an internal pad that has no external > + connections. Such a pad may not be connected with a link. The internal > + flag indicates that the stream either starts or ends in the There's no mention of "stream" (with this meaning) anywhere in the MC API documentation, so you will need to explain streams first. Apart from that, and addressing Tomi's comment, this patch seems OK. > + entity. For a given pad, the INTERNAL_SOURCE flag may not be set if > + either SINK or SOURCE flags is set. > > 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 ef34ddd715c9..ed99160a2487 100644 > --- a/drivers/media/mc/mc-entity.c > +++ b/drivers/media/mc/mc-entity.c > @@ -1062,7 +1062,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_SOURCE)) != pad_type) > continue; > > if (entity->pads[i].sig_type == sig_type) > @@ -1087,6 +1088,11 @@ 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(sink->pads[sink_pad].flags & > + MEDIA_PAD_FL_INTERNAL_SOURCE) || > + WARN_ON(source->pads[source_pad].flags & > + MEDIA_PAD_FL_INTERNAL_SOURCE)) > + 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 edb8dfef9eba..0e2577e8b425 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_SOURCE (1U << 3) > > struct media_pad_desc { > __u32 entity; /* entity ID */
Hi Laurent, On Fri, Jun 02, 2023 at 12:18:44PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Sat, May 06, 2023 at 12:52:51AM +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> > > --- > > .../userspace-api/media/mediactl/media-types.rst | 7 +++++++ > > drivers/media/mc/mc-entity.c | 8 +++++++- > > include/uapi/linux/media.h | 1 + > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst > > index 0ffeece1e0c8..c724139ad46c 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-SOURCE: > > > > .. flat-table:: Media pad flags > > :header-rows: 0 > > @@ -382,6 +383,12 @@ 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_SOURCE`` > > + - This flag indicates an internal pad that has no external > > + connections. Such a pad may not be connected with a link. The internal > > + flag indicates that the stream either starts or ends in the > > There's no mention of "stream" (with this meaning) anywhere in the MC > API documentation, so you will need to explain streams first. I'll add it to glossary and add a reference here. > > Apart from that, and addressing Tomi's comment, this patch seems OK. > > > + entity. For a given pad, the INTERNAL_SOURCE flag may not be set if > > + either SINK or SOURCE flags is set. > > > > 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 ef34ddd715c9..ed99160a2487 100644 > > --- a/drivers/media/mc/mc-entity.c > > +++ b/drivers/media/mc/mc-entity.c > > @@ -1062,7 +1062,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_SOURCE)) != pad_type) > > continue; > > > > if (entity->pads[i].sig_type == sig_type) > > @@ -1087,6 +1088,11 @@ 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(sink->pads[sink_pad].flags & > > + MEDIA_PAD_FL_INTERNAL_SOURCE) || > > + WARN_ON(source->pads[source_pad].flags & > > + MEDIA_PAD_FL_INTERNAL_SOURCE)) > > + 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 edb8dfef9eba..0e2577e8b425 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_SOURCE (1U << 3) > > > > struct media_pad_desc { > > __u32 entity; /* entity ID */ >
diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst index 0ffeece1e0c8..c724139ad46c 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-SOURCE: .. flat-table:: Media pad flags :header-rows: 0 @@ -382,6 +383,12 @@ 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_SOURCE`` + - This flag indicates an internal pad that has no external + connections. Such a pad may not be connected with a link. The internal + flag indicates that the stream either starts or ends in the + entity. For a given pad, the INTERNAL_SOURCE flag may not be set if + either SINK or SOURCE flags is set. 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 ef34ddd715c9..ed99160a2487 100644 --- a/drivers/media/mc/mc-entity.c +++ b/drivers/media/mc/mc-entity.c @@ -1062,7 +1062,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_SOURCE)) != pad_type) continue; if (entity->pads[i].sig_type == sig_type) @@ -1087,6 +1088,11 @@ 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(sink->pads[sink_pad].flags & + MEDIA_PAD_FL_INTERNAL_SOURCE) || + WARN_ON(source->pads[source_pad].flags & + MEDIA_PAD_FL_INTERNAL_SOURCE)) + 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 edb8dfef9eba..0e2577e8b425 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_SOURCE (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> --- .../userspace-api/media/mediactl/media-types.rst | 7 +++++++ drivers/media/mc/mc-entity.c | 8 +++++++- include/uapi/linux/media.h | 1 + 3 files changed, 15 insertions(+), 1 deletion(-)