Re: [RFC 1/7] media: mc: Add INTERNAL_SOURCE pad type flag

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

 



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@xxxxxxxxxxxxxxx>
> > ---
> >  .../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



[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