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@xxxxxxxxxxxxxxx>
> > ---
> > 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 */
>
--
Sakari Ailus
