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

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

[Sakari Ailus]

Hi Laurent,

On Fri, Sep 20, 2013 at 11:08:47PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.

Thanks for the review! :)

> On Thursday 19 September 2013 01:01:05 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>
> > ---
> >  Documentation/DocBook/media/v4l/media-ioc-enum-links.xml |    8 ++++++++
> >  include/uapi/linux/media.h                               |    1 +
> >  2 files changed, 9 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..59b212a 100644
> > --- a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > +++ b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > @@ -134,6 +134,14 @@
> >  	    <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>A pad must be connected with an enabled link for the
> 
> s/A pad/The pad/ ?

Fixed.

> > +	    entity to be able to stream. There could be temporary reasons
> > +	    (e.g. device configuration dependent) for the pad to need
> > +	    connecting; the absence of the flag won't say there
> > +	    may not be any.</entry>
> 
> I believe the description doesn't make it very explicit that a MUST_CONNECT 
> pad with no existing link is valid, as opposed to existing links with no 
> enabled link, which would be invalid. Do you think we should fix that ?

Yes. I propose to add this: "The flag has no effect on pads without
connected links."

> > +	  </row>
> >  	</tbody>
> >        </tgroup>
> >      </table>
> > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > index ed49574..d847c76 100644
> > --- a/include/uapi/linux/media.h
> > +++ b/include/uapi/linux/media.h
> > @@ -98,6 +98,7 @@ struct media_entity_desc {
> > 
> >  #define MEDIA_PAD_FL_SINK		(1 << 0)
> >  #define MEDIA_PAD_FL_SOURCE		(1 << 1)
> > +#define MEDIA_PAD_FL_MUST_CONNECT	(1 << 2)
> > 
> >  struct media_pad_desc {
> >  	__u32 entity;		/* entity ID */

-- 
Kind regards,

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

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

[Laurent Pinchart]

Hi Sakari,

On Tuesday 01 October 2013 02:08:47 Sakari Ailus wrote:
> On Fri, Sep 20, 2013 at 11:08:47PM +0200, Laurent Pinchart wrote:
> > On Thursday 19 September 2013 01:01:05 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>
> > > ---
> > > 
> > >  Documentation/DocBook/media/v4l/media-ioc-enum-links.xml |    8 +++++++
> > >  include/uapi/linux/media.h                               |    1 +
> > >  2 files changed, 9 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..59b212a 100644
> > > --- a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > > +++ b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > > @@ -134,6 +134,14 @@
> > >  	    <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>A pad must be connected with an enabled link for the
> > 
> > s/A pad/The pad/ ?
> 
> Fixed.
> 
> > > +	    entity to be able to stream. There could be temporary reasons
> > > +	    (e.g. device configuration dependent) for the pad to need
> > > +	    connecting; the absence of the flag won't say there
> > > +	    may not be any.</entry>
> > 
> > I believe the description doesn't make it very explicit that a
> > MUST_CONNECT pad with no existing link is valid, as opposed to existing
> > links with no enabled link, which would be invalid. Do you think we should
> > fix that ?
> 
> Yes. I propose to add this: "The flag has no effect on pads without
> connected links."

What about

If the pad is linked to any other pad, at least one of the 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; the 
absence of the flag doesn't imply there is none. The flag has no effect on
pads without connected links.

> > > +	  </row>
> > >  	</tbody>
> > >        </tgroup>
> > >      </table>

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

Thnanks for the comments. A few more below.

On Tue, Oct 01, 2013 at 01:21:58AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday 01 October 2013 02:08:47 Sakari Ailus wrote:
> > On Fri, Sep 20, 2013 at 11:08:47PM +0200, Laurent Pinchart wrote:
> > > On Thursday 19 September 2013 01:01:05 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>
> > > > ---
> > > > 
> > > >  Documentation/DocBook/media/v4l/media-ioc-enum-links.xml |    8 +++++++
> > > >  include/uapi/linux/media.h                               |    1 +
> > > >  2 files changed, 9 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..59b212a 100644
> > > > --- a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > > > +++ b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> > > > @@ -134,6 +134,14 @@
> > > >  	    <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>A pad must be connected with an enabled link for the
> > > 
> > > s/A pad/The pad/ ?
> > 
> > Fixed.
> > 
> > > > +	    entity to be able to stream. There could be temporary reasons
> > > > +	    (e.g. device configuration dependent) for the pad to need
> > > > +	    connecting; the absence of the flag won't say there
> > > > +	    may not be any.</entry>
> > > 
> > > I believe the description doesn't make it very explicit that a
> > > MUST_CONNECT pad with no existing link is valid, as opposed to existing
> > > links with no enabled link, which would be invalid. Do you think we should
> > > fix that ?
> > 
> > Yes. I propose to add this: "The flag has no effect on pads without
> > connected links."
> 
> What about
> 
> If the pad is linked to any other pad, at least one of the 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; the 
> absence of the flag doesn't imply there is none. The flag has no effect on
> pads without connected links.

Thinking about this again, I'd add before the comma: "and this flag is set".

And if you put it like that then the last sentence is redundat --- I'd drop
it.

What do you think?

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

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

[Laurent Pinchart]

Hi Sakari,

On Tuesday 01 October 2013 02:28:23 Sakari Ailus wrote:
> On Tue, Oct 01, 2013 at 01:21:58AM +0200, Laurent Pinchart wrote:
> > On Tuesday 01 October 2013 02:08:47 Sakari Ailus wrote:
> > > On Fri, Sep 20, 2013 at 11:08:47PM +0200, Laurent Pinchart wrote:
> > > > On Thursday 19 September 2013 01:01:05 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>

[snip]

> > What about
> > 
> > If the pad is linked to any other pad, at least one of the 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; the absence of the flag doesn't imply there is none. The flag has
> > no effect on pads without connected links.
> 
> Thinking about this again, I'd add before the comma: "and this flag is set".
> 
> And if you put it like that then the last sentence is redundat --- I'd drop
> it.
> 
> What do you think?

What about

"When this flag is set, if 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."

Feel free to drop the last sentence.

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

On Tue, Oct 01, 2013 at 10:55:04AM +0200, Laurent Pinchart wrote:
> On Tuesday 01 October 2013 02:28:23 Sakari Ailus wrote:
> > On Tue, Oct 01, 2013 at 01:21:58AM +0200, Laurent Pinchart wrote:
> > > On Tuesday 01 October 2013 02:08:47 Sakari Ailus wrote:
> > > > On Fri, Sep 20, 2013 at 11:08:47PM +0200, Laurent Pinchart wrote:
> > > > > On Thursday 19 September 2013 01:01:05 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>
> 
> [snip]
> 
> > > What about
> > > 
> > > If the pad is linked to any other pad, at least one of the 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; the absence of the flag doesn't imply there is none. The flag has
> > > no effect on pads without connected links.
> > 
> > Thinking about this again, I'd add before the comma: "and this flag is set".
> > 
> > And if you put it like that then the last sentence is redundat --- I'd drop
> > it.
> > 
> > What do you think?
> 
> What about
> 
> "When this flag is set, if the pad is linked to any other pad then at least 

How about:

"If this flag is set and the pad is linked to any other pad, then"...

I think it's cleaner like that.

> 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."
> 
> Feel free to drop the last sentence.

Thinking about it again, I'm fine keeping it. :-)

-- 
Kind regards,

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

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

[Laurent Pinchart]

Hi Sakari,

On Tuesday 01 October 2013 12:17:21 Sakari Ailus wrote:
> On Tue, Oct 01, 2013 at 10:55:04AM +0200, Laurent Pinchart wrote:
> > On Tuesday 01 October 2013 02:28:23 Sakari Ailus wrote:
> > > On Tue, Oct 01, 2013 at 01:21:58AM +0200, Laurent Pinchart wrote:
> > > > On Tuesday 01 October 2013 02:08:47 Sakari Ailus wrote:
> > > > > On Fri, Sep 20, 2013 at 11:08:47PM +0200, Laurent Pinchart wrote:
> > > > > > On Thursday 19 September 2013 01:01:05 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>
> > 
> > [snip]
> > 
> > > > What about
> > > > 
> > > > If the pad is linked to any other pad, at least one of the 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; the absence of the flag doesn't imply there is
> > > > none. The flag has no effect on pads without connected links.
> > > 
> > > Thinking about this again, I'd add before the comma: "and this flag is
> > > set".
> > > 
> > > And if you put it like that then the last sentence is redundat --- I'd
> > > drop it.
> > > 
> > > What do you think?
> > 
> > What about
> > 
> > "When this flag is set, if the pad is linked to any other pad then at
> > least
> 
> How about:
> 
> "If this flag is set and the pad is linked to any other pad, then"...
> 
> I think it's cleaner like that.

Fine with me.

> > 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."
> > 
> > Feel free to drop the last sentence.
> 
> Thinking about it again, I'm fine keeping it. :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] media: Check for active links on pads with MEDIA_PAD_FL_MUST_CONNECT flag

[Laurent Pinchart]

Hi Sakari,

On Monday 23 September 2013 22:57:02 Sakari Ailus wrote:
> On Fri, Sep 20, 2013 at 10:54:22PM +0200, Laurent Pinchart wrote:
> > > @@ -248,21 +250,46 @@ __must_check int
> > > media_entity_pipeline_start(struct media_entity *entity,
> > >  		if (!entity->ops || !entity->ops->link_validate)
> > >  			continue;
> > > 
> > > +		bitmap_zero(active, entity->num_pads);
> > > +		bitmap_fill(has_no_links, entity->num_pads);
> > > +
> > >  		for (i = 0; i < entity->num_links; i++) {
> > >  			struct media_link *link = &entity->links[i];
> > > -
> > > -			/* Is this pad part of an enabled link? */
> > > -			if (!(link->flags & MEDIA_LNK_FL_ENABLED))
> > > -				continue;
> > > -
> > > -			/* Are we the sink or not? */
> > > -			if (link->sink->entity != entity)
> > > +			struct media_pad *pad = link->sink->entity == entity
> > > +				? link->sink : link->source;
> > 
> > What about aligning the ? to the = ? With that change,
> 
> How about to the beginning of the next operand rather than "="?
> 
> (Think of adding parentheses around the rvalue of "=".)
> 
> I think it's fine as it was, but if it's to be changed then it should be
> aligned to link->sink->entity IMHO. :-)

My preference goes for aligning the ? under the =, but I agree it's not 
logical from an rvalue point of view :-) I would favor aligning the ? under 
the l of link, but enough bikeshedding for now, please pick whichever solution 
you prefer :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] media: Check for active links on pads with MEDIA_PAD_FL_MUST_CONNECT flag

[Sakari Ailus]

Hi Laurent,

On Tue, Oct 01, 2013 at 02:39:14PM +0200, Laurent Pinchart wrote:
> On Monday 23 September 2013 22:57:02 Sakari Ailus wrote:
> > On Fri, Sep 20, 2013 at 10:54:22PM +0200, Laurent Pinchart wrote:
> > > > @@ -248,21 +250,46 @@ __must_check int
> > > > media_entity_pipeline_start(struct media_entity *entity,
> > > >  		if (!entity->ops || !entity->ops->link_validate)
> > > >  			continue;
> > > > 
> > > > +		bitmap_zero(active, entity->num_pads);
> > > > +		bitmap_fill(has_no_links, entity->num_pads);
> > > > +
> > > >  		for (i = 0; i < entity->num_links; i++) {
> > > >  			struct media_link *link = &entity->links[i];
> > > > -
> > > > -			/* Is this pad part of an enabled link? */
> > > > -			if (!(link->flags & MEDIA_LNK_FL_ENABLED))
> > > > -				continue;
> > > > -
> > > > -			/* Are we the sink or not? */
> > > > -			if (link->sink->entity != entity)
> > > > +			struct media_pad *pad = link->sink->entity == entity
> > > > +				? link->sink : link->source;
> > > 
> > > What about aligning the ? to the = ? With that change,
> > 
> > How about to the beginning of the next operand rather than "="?
> > 
> > (Think of adding parentheses around the rvalue of "=".)
> > 
> > I think it's fine as it was, but if it's to be changed then it should be
> > aligned to link->sink->entity IMHO. :-)
> 
> My preference goes for aligning the ? under the =, but I agree it's not 
> logical from an rvalue point of view :-) I would favor aligning the ? under 
> the l of link, but enough bikeshedding for now, please pick whichever solution 
> you prefer :-)

? goes under l of the link; agreed. I'll resend the set.

-- 
Cheers,

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