From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: linux-media@vger.kernel.org, laurent.pinchart@ideasonboard.com,
tomi.valkeinen@ideasonboard.com, bingbu.cao@intel.com,
hongju.wang@intel.com
Subject: Re: [RFC 1/7] media: mc: Add INTERNAL_SOURCE pad type flag
Date: Fri, 9 Jun 2023 12:44:39 +0000 [thread overview]
Message-ID: <ZIMetwrhQcn3GgF8@kekkonen.localdomain> (raw)
In-Reply-To: <8b315fe6-0d34-2f93-bf0f-7f3be0b37dbc@xs4all.nl>
Hi Hans,
Thanks for the review.
On Thu, Jun 08, 2023 at 09:59:48AM +0200, Hans Verkuil wrote:
> On 05/05/2023 23: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
>
> 'internal pad' or 'internal source pad'?
>
> > + 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.
>
> This sentence makes no sense. The flag is for a pad, but then this sentence
> talks about streams and entities and doesn't relate that to the pad. Also,
Streams are between pads, but are visible on V4L2 only while this is MC
related. Documentation-wise this is a bit awkward.
> the flag is INTERNAL_SOURCE, and reading 'stream ends' suggests that it
> should be INTERNAL_SINK in that case.
When it comes to routing, it uses fields related to the sink pad, but this
is a source.
Based on review of the other patches, it seems both Tomi and Laurent would
prefer a plain INTERNAL flag which then would be used in conjunction with
the existing SINK flag, to note an internal source. Routing-wise this is a
cleaner option as this is associated with the sink related fields in any
case.
>
> I think this needs to be more elaborate and ideally add some examples
> of the use-case(s) (or refer to an example elsewhere).
>
> 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``
>
> and 'MEDIA_PAD_FL_INTERNAL_SOURCE' as well?
>
> > 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 */
>
> Regards,
>
> Hans
--
Kind regards,
Sakari Ailus
next prev parent reply other threads:[~2023-06-09 12:44 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-05 21:52 [RFC 0/7] Generic line based metadata support, internal pads Sakari Ailus
2023-05-05 21:52 ` [RFC 1/7] media: mc: Add INTERNAL_SOURCE pad type flag Sakari Ailus
2023-05-08 9:52 ` Tomi Valkeinen
2023-05-08 12:04 ` Sakari Ailus
2023-05-08 12:07 ` Tomi Valkeinen
2023-05-08 12:28 ` Sakari Ailus
2023-06-02 9:18 ` Laurent Pinchart
2023-06-02 15:05 ` Sakari Ailus
2023-06-08 7:59 ` Hans Verkuil
2023-06-09 12:44 ` Sakari Ailus [this message]
2023-05-05 21:52 ` [RFC 2/7] media: v4l: subdev: Support INTERNAL_SOURCE pads in routing IOCTLs Sakari Ailus
2023-05-08 10:14 ` Tomi Valkeinen
2023-05-08 12:24 ` Sakari Ailus
2023-06-02 9:44 ` Laurent Pinchart
2023-06-02 9:46 ` Laurent Pinchart
2023-06-02 13:10 ` Sakari Ailus
2023-06-04 14:26 ` Laurent Pinchart
2023-06-05 8:06 ` Sakari Ailus
2023-06-05 8:23 ` Laurent Pinchart
2023-06-08 8:06 ` Hans Verkuil
2023-05-05 21:52 ` [RFC 3/7] media: uapi: v4l: Document source routes Sakari Ailus
2023-05-08 10:33 ` Tomi Valkeinen
2023-05-08 16:26 ` Sakari Ailus
2023-05-08 16:35 ` Tomi Valkeinen
2023-05-08 17:41 ` Sakari Ailus
2023-06-02 9:56 ` Laurent Pinchart
2023-06-02 9:56 ` Laurent Pinchart
2023-06-09 12:55 ` Sakari Ailus
2023-06-08 8:20 ` Hans Verkuil
2023-05-05 21:52 ` [RFC 4/7] media: mc: Check pad flag validity Sakari Ailus
2023-06-02 9:58 ` Laurent Pinchart
2023-06-09 14:41 ` Sakari Ailus
2023-05-05 21:52 ` [RFC 5/7] media: uapi: Add generic serial metadata mbus formats Sakari Ailus
2023-06-02 10:36 ` Laurent Pinchart
2023-06-09 14:45 ` Sakari Ailus
2023-06-08 8:35 ` Hans Verkuil
2023-06-09 13:34 ` Sakari Ailus
2023-06-08 8:46 ` Hans Verkuil
2023-06-09 13:38 ` Sakari Ailus
2023-05-05 21:52 ` [RFC 6/7] media: uapi: Add generic 8-bit metadata format definitions Sakari Ailus
2023-06-08 8:54 ` Hans Verkuil
2023-06-09 14:27 ` Sakari Ailus
2023-05-05 21:52 ` [RFC 7/7] media: v4l: Support line-based metadata capture Sakari Ailus
2023-06-02 10:50 ` Laurent Pinchart
2023-06-09 13:46 ` Sakari Ailus
2023-06-02 7:54 ` [RFC 0/7] Generic line based metadata support, internal pads Naushir Patuck
2023-06-02 8:46 ` Sakari Ailus
2023-06-02 9:35 ` Naushir Patuck
2023-06-02 12:05 ` Sakari Ailus
2023-06-02 9:12 ` Laurent Pinchart
2023-06-02 9:43 ` Naushir Patuck
2023-06-09 13:20 ` Sakari Ailus
2023-06-09 13:59 ` Dave Stevenson
2023-06-09 14:41 ` Sakari Ailus
2023-08-03 22:36 ` Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZIMetwrhQcn3GgF8@kekkonen.localdomain \
--to=sakari.ailus@linux.intel.com \
--cc=bingbu.cao@intel.com \
--cc=hongju.wang@intel.com \
--cc=hverkuil@xs4all.nl \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=tomi.valkeinen@ideasonboard.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).