Re: [PATCH v2 1/4] media: Add pad flag MEDIA_PAD_FL_MUST_CONNECT

Sakari Ailus
2013-10-03T08:43:07Z

Hi Sylwester,

On Thu, Oct 03, 2013 at 11:16:59AM +0900, Sylwester Nawrocki wrote:
> Hi Sakari,
>
> On 10/03/2013 08:29 AM, Laurent Pinchart wrote:
> > Hi Sakari,
> >
> > Thank you for the patch.
> >
> > On Thursday 03 October 2013 02:17:50 Sakari Ailus wrote:
> >> Pads that set this flag must be connected by an active link for the entity
> >> to stream.
> >>
> >> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> >> Acked-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> >
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Looks good, I would just like to ask for changing my e-mail address on those
> patches to s.nawrocki@samsung.com, sorry for not mentioning it earlier.
> Just one doubt below regarding name of the flag.

Will do.

> >> ---
> >> Documentation/DocBook/media/v4l/media-ioc-enum-links.xml | 10 ++++++++++
> >> include/uapi/linux/media.h | 1 +
> >> 2 files changed, 11 insertions(+)
> >>
> >> diff --git a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> >> b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml index
> >> 355df43..e357dc9 100644
> >> --- a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> >> +++ b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> >> @@ -134,6 +134,16 @@
> >> <entry>Output pad, relative to the entity. Output pads source data
> >> and are origins of links.</entry>
> >> </row>
> >> + <row>
> >> + <entry><constant>MEDIA_PAD_FL_MUST_CONNECT</constant></entry>
> >> + <entry>If this flag is set and the pad is linked to any other
> >> + pad, then at least one of those links must be enabled for the
> >> + entity to be able to stream. There could be temporary reasons
> >> + (e.g. device configuration dependent) for the pad to need
> >> + enabled links even when this flag isn't set; the absence of the
> >> + flag doesn't imply there is none. The flag has no effect on pads
> >> + without connected links.</entry>
>
> Probably MEDIA_PAD_FL_MUST_CONNECT name is fine, but isn't it more something
> like MEDIA_PAD_FL_NEED_ACTIVE_LINK ? Or presumably MEDIA_PAD_FL_MUST_CONNECT
> just doesn't make sense on pads without connected links and should never be
> set on such pads ? From the last sentence it feels the situation where a pad
> without a connected link has this flags set is allowed and a valid
> configuration.
>
> Perhaps the last sentence should be something like:
>
> "The flag should not be used on pads without connected links and has no effect
> on such pads."

Hmm. Good question. My assumption was that such cases might appear when an
external entity could be connected to the pad, but receivers typically have
just a single pad. So streaming would be out of question in such case
anyway. But my thought was that we shouldn't burden drivers by forcing them
to unset the flag if there happens to be nothing connected to an entity.

How about just that I remove the last sentence, and s/ and the pad is linked
to any other pad, then at least one of those links must be enabled/, the pad
must be connected by an enabled link/? :-)

The purpose is two-fold: to allow automated pipeline validation and also
hint the user what are the conditions for that configuration.

--
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk