Re: [PATCH v6 01/28] media: mc: Add INTERNAL pad flag

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

On Fri, Oct 27, 2023 at 03:48:04PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Oct 25, 2023 at 09:17:15PM +0000, Sakari Ailus wrote:
> > On Thu, Oct 05, 2023 at 01:16:27PM +0200, Hans Verkuil wrote:
> > > On 05/10/2023 12:22, Sakari Ailus wrote:
> > > > On Thu, Oct 05, 2023 at 11:52:08AM +0200, Hans Verkuil wrote:
> > > >> On 03/10/2023 13: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 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.
> > > >>
> > > >> Hmm, I think this is a bit confusing. I think it would be better to replace
> > > >> "from source to sink" with "from the initial source to the final sink".
> > > > 
> > > > Seems fine to me. I'll use that in v7.
> > > > 
> > > >>
> > > >> The original text doesn't make it clear that there can be many hops in
> > > >> between.
> > > >>
> > > >> So also: "A source" -> "An initial source", and "a sink" -> "a final sink".
> > > >>
> > > >> Note that "media pipeline" isn't defined either. Should that be added?
> > > > 
> > > > I can add that, too...
> > > > 
> > > >>
> > > >> Finally, I think this should be a separate patch as it has nothing to do with
> > > >> adding the INTERNAL pad flag.
> > > > 
> > > > I agree, will split in v7.
> > > > 
> > > >>
> > > >>> +
> > > >>>      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.
> > > >>
> > > >> This suggests that INTERNAL can be used for both sinks and sources, but...
> > > > 
> > > > Right. The INTERNAL flag in UAPI shouldn't be limited to sources, but at
> > > > the same time we don't have use for it in sinks. I'd prefer to leave this
> > > > open in the future. We could of course say that explicitly.
> > > 
> > > I think it is better to mention that it is for streams that start in the
> > > entity only, but that in the future it might be extended to support streams
> > > that end in the entity.
> > > 
> > > When we add support for that, we need to update the documentation as well,
> > > at minimum with one or more examples of how that would be used.
> > 
> > I'll use this instead of the original sentence:
> > 
> > 	  The internal flag may currently be present only in a source pad only
> 
> Double "only". 
> 
> 	The internal flag may currently be present only in source pads, where
> 	...

Thanks for spotting this, I'll address it in v7.

> 
> > 	  where it indicates that the :ref:``stream <media-glossary-stream>``
> > 	  originates from within the entity.
> 

-- 
Regards,

Sakari Ailus



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux