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.
--
Kind regards,
Sakari Ailus
