diff mbox

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

Message ID 1380755873-25835-2-git-send-email-sakari.ailus@iki.fi (mailing list archive)
State New, archived
Headers show

Commit Message

Sakari Ailus Oct. 2, 2013, 11:17 p.m. UTC 
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 |   10 ++++++++++
 include/uapi/linux/media.h                               |    1 +
 2 files changed, 11 insertions(+)

Comments

Laurent Pinchart Oct. 2, 2013, 11:29 p.m. UTC | #1 
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>

> ---
>  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>
> +	  </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 */
Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Oct. 3, 2013, 2:16 a.m. UTC | #2 
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.

>> ---
>>  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." 

Regards,
Sylwester

>> +	  </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 */
Sakari Ailus Oct. 3, 2013, 8:43 a.m. UTC | #3 
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.
Laurent Pinchart Oct. 3, 2013, 9:40 a.m. UTC | #4 
On Thursday 03 October 2013 11:43:01 Sakari Ailus wrote:
> On Thu, Oct 03, 2013 at 11:16:59AM +0900, Sylwester Nawrocki wrote:
> > On 10/03/2013 08:29 AM, Laurent Pinchart wrote:
> > > 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.

If I'm not mistaken, that's a valid configuration. The flag merely says that, 
if a pad has any link, then one of them must be active (Sakari, please correct 
me if I'm wrong).

> > 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.
Sylwester Nawrocki Oct. 3, 2013, 10:13 p.m. UTC | #5 
Hi,

On 10/03/2013 11:40 AM, Laurent Pinchart wrote:
>>>>> 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.
>
> If I'm not mistaken, that's a valid configuration. The flag merely says that,
> if a pad has any link, then one of them must be active (Sakari, please correct
> me if I'm wrong).

It may be valid but it just sounds odd to me. Since the pad without a link
cannot be connected to anything, how could setting MUST_CONNECT flag on 
it be
logical ? :) I think it's more about name of the flag, since its semantics
seems very well described.

>>> >  >  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/?:-)

I guess removing the last sentence could be enough, IMHO it's pretty 
clear also
without this sentence the MEDIA_PAD_FL_MUST_CONNECT flag is meaningless 
on a pad
without links.

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

Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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>
+	  </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 */