On Thu, Aug 03, 2023 at 11:24:46AM +0300, Tomi Valkeinen wrote:
> On 03/08/2023 11:19, Sakari Ailus wrote:
> > Moi,
> >
> > Thanks for the review.
> >
> > On Thu, Aug 03, 2023 at 10:56:35AM +0300, Tomi Valkeinen wrote:
> > > On 03/08/2023 00:45, 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@xxxxxxxxxxxxxxx>
> > > > ---
> > > > 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))
> > > > + return -EINVAL;
> > >
> > > What does this check?
> > >
> > > Shouldn't we check here if either sink or source is INTERNAL, then fail?
> >
> > We shouldn't have any source pads that have the internal flag set, that was
> > the intention here (but you can drop the source flag check). Also both
> > pads can't have the internal flag, I'll add that for v3.
>
> The function is media_create_pad_link(), isn't that about creating links
> between entities? Internal pads shouldn't be allowed on either side.
Uh, yes, indeed. I'll address this in v3.
--
Sakari Ailus
