[PATCH v2 0/4] Add MEDIA_PAD_FL_MUST_CONNECT pad flag, check it

[PATCH v2 0/4] Add MEDIA_PAD_FL_MUST_CONNECT pad flag, check it

[Sakari Ailus]

Hi all,

This is the second version of the patchset that adds a new media pad flag to
denote that the pad must be connected by an enabled link for the entity to
be able to stream.

The previous version of the set is available here:

<URL:http://www.spinics.net/lists/linux-media/msg68105.html>

What has changed is

- Indentation of the pad flags for the omap3isp driver and assignment of pad
  in media_entity_pipeline_start() and

- Improved the pad flag documentation.

Laurent: unless there further comments to the patches, could you take them
to your tree as they contain changes to the Media controller framework and
the omap3isp driver?

-- 
Kind regards,
Sakari

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

[Sakari Ailus]

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(+)

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

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

[Sakari Ailus]

Do not allow streaming if a pad with MEDIA_PAD_FL_MUST_CONNECT flag is not
connected by an active link.

This patch makes it possible to avoid drivers having to check for the most
common case of link state validation: a sink pad that must be connected.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Tested-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/media-entity.c |   41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 2c286c3..37c334e 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -235,6 +235,8 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
 	media_entity_graph_walk_start(&graph, entity);
 
 	while ((entity = media_entity_graph_walk_next(&graph))) {
+		DECLARE_BITMAP(active, entity->num_pads);
+		DECLARE_BITMAP(has_no_links, entity->num_pads);
 		unsigned int i;
 
 		entity->stream_count++;
@@ -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;
+
+			/* Mark that a pad is connected by a link. */
+			bitmap_clear(has_no_links, pad->index, 1);
+
+			/*
+			 * Pads that either do not need to connect or
+			 * are connected through an enabled link are
+			 * fine.
+			 */
+			if (!(pad->flags & MEDIA_PAD_FL_MUST_CONNECT) ||
+			    link->flags & MEDIA_LNK_FL_ENABLED)
+				bitmap_set(active, pad->index, 1);
+
+			/*
+			 * Link validation will only take place for
+			 * sink ends of the link that are enabled.
+			 */
+			if (link->sink != pad ||
+			    !(link->flags & MEDIA_LNK_FL_ENABLED))
 				continue;
 
 			ret = entity->ops->link_validate(link);
 			if (ret < 0 && ret != -ENOIOCTLCMD)
 				goto error;
 		}
+
+		/* Either no links or validated links are fine. */
+		bitmap_or(active, active, has_no_links, entity->num_pads);
+
+		if (!bitmap_full(active, entity->num_pads)) {
+			ret = -EPIPE;
+			goto error;
+		}
 	}
 
 	mutex_unlock(&mdev->graph_mutex);
-- 
1.7.10.4

[PATCH v2 3/4] omap3isp: Mark which pads must connect

[Sakari Ailus]

Mark pads that must be connected.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/ispccdc.c    |    3 ++-
 drivers/media/platform/omap3isp/ispccp2.c    |    3 ++-
 drivers/media/platform/omap3isp/ispcsi2.c    |    3 ++-
 drivers/media/platform/omap3isp/isppreview.c |    3 ++-
 drivers/media/platform/omap3isp/ispresizer.c |    3 ++-
 drivers/media/platform/omap3isp/ispstat.c    |    2 +-
 drivers/media/platform/omap3isp/ispvideo.c   |    6 ++++--
 7 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 907a205..561c991 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -2484,7 +2484,8 @@ static int ccdc_init_entities(struct isp_ccdc_device *ccdc)
 	v4l2_set_subdevdata(sd, ccdc);
 	sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_HAS_DEVNODE;
 
-	pads[CCDC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+	pads[CCDC_PAD_SINK].flags = MEDIA_PAD_FL_SINK
+				    | MEDIA_PAD_FL_MUST_CONNECT;
 	pads[CCDC_PAD_SOURCE_VP].flags = MEDIA_PAD_FL_SOURCE;
 	pads[CCDC_PAD_SOURCE_OF].flags = MEDIA_PAD_FL_SOURCE;
 
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index e716514..e84fe05 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1076,7 +1076,8 @@ static int ccp2_init_entities(struct isp_ccp2_device *ccp2)
 	v4l2_set_subdevdata(sd, ccp2);
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
-	pads[CCP2_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+	pads[CCP2_PAD_SINK].flags = MEDIA_PAD_FL_SINK
+				    | MEDIA_PAD_FL_MUST_CONNECT;
 	pads[CCP2_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
 
 	me->ops = &ccp2_media_ops;
diff --git a/drivers/media/platform/omap3isp/ispcsi2.c b/drivers/media/platform/omap3isp/ispcsi2.c
index 6db245d..6205608 100644
--- a/drivers/media/platform/omap3isp/ispcsi2.c
+++ b/drivers/media/platform/omap3isp/ispcsi2.c
@@ -1245,7 +1245,8 @@ static int csi2_init_entities(struct isp_csi2_device *csi2)
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
 	pads[CSI2_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
-	pads[CSI2_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+	pads[CSI2_PAD_SINK].flags = MEDIA_PAD_FL_SINK
+				    | MEDIA_PAD_FL_MUST_CONNECT;
 
 	me->ops = &csi2_media_ops;
 	ret = media_entity_init(me, CSI2_PADS_NUM, pads, 0);
diff --git a/drivers/media/platform/omap3isp/isppreview.c b/drivers/media/platform/omap3isp/isppreview.c
index cd8831a..1c776c1 100644
--- a/drivers/media/platform/omap3isp/isppreview.c
+++ b/drivers/media/platform/omap3isp/isppreview.c
@@ -2283,7 +2283,8 @@ static int preview_init_entities(struct isp_prev_device *prev)
 	v4l2_ctrl_handler_setup(&prev->ctrls);
 	sd->ctrl_handler = &prev->ctrls;
 
-	pads[PREV_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+	pads[PREV_PAD_SINK].flags = MEDIA_PAD_FL_SINK
+				    | MEDIA_PAD_FL_MUST_CONNECT;
 	pads[PREV_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
 
 	me->ops = &preview_media_ops;
diff --git a/drivers/media/platform/omap3isp/ispresizer.c b/drivers/media/platform/omap3isp/ispresizer.c
index d11fb26..fa35f2c 100644
--- a/drivers/media/platform/omap3isp/ispresizer.c
+++ b/drivers/media/platform/omap3isp/ispresizer.c
@@ -1701,7 +1701,8 @@ static int resizer_init_entities(struct isp_res_device *res)
 	v4l2_set_subdevdata(sd, res);
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
-	pads[RESZ_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+	pads[RESZ_PAD_SINK].flags = MEDIA_PAD_FL_SINK
+				    | MEDIA_PAD_FL_MUST_CONNECT;
 	pads[RESZ_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
 
 	me->ops = &resizer_media_ops;
diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c
index 61e17f9..a75407c 100644
--- a/drivers/media/platform/omap3isp/ispstat.c
+++ b/drivers/media/platform/omap3isp/ispstat.c
@@ -1067,7 +1067,7 @@ static int isp_stat_init_entities(struct ispstat *stat, const char *name,
 	subdev->flags |= V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_HAS_DEVNODE;
 	v4l2_set_subdevdata(subdev, stat);
 
-	stat->pad.flags = MEDIA_PAD_FL_SINK;
+	stat->pad.flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
 	me->ops = NULL;
 
 	return media_entity_init(me, 1, &stat->pad, 0);
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index a908d00..d45af5c 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1335,11 +1335,13 @@ int omap3isp_video_init(struct isp_video *video, const char *name)
 	switch (video->type) {
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
 		direction = "output";
-		video->pad.flags = MEDIA_PAD_FL_SINK;
+		video->pad.flags = MEDIA_PAD_FL_SINK
+				   | MEDIA_PAD_FL_MUST_CONNECT;
 		break;
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
 		direction = "input";
-		video->pad.flags = MEDIA_PAD_FL_SOURCE;
+		video->pad.flags = MEDIA_PAD_FL_SOURCE
+				   | MEDIA_PAD_FL_MUST_CONNECT;
 		video->video.vfl_dir = VFL_DIR_TX;
 		break;
 
-- 
1.7.10.4

[PATCH v2 4/4] omap3isp: Add resizer data rate configuration to resizer_link_validate

[Sakari Ailus]

The configuration of many other blocks depend on resizer maximum data rate.
Get the value from resizer at link validation time.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/ispresizer.c |   15 +++++++
 drivers/media/platform/omap3isp/ispvideo.c   |   54 --------------------------
 2 files changed, 15 insertions(+), 54 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispresizer.c b/drivers/media/platform/omap3isp/ispresizer.c
index fa35f2c..0d36b8b 100644
--- a/drivers/media/platform/omap3isp/ispresizer.c
+++ b/drivers/media/platform/omap3isp/ispresizer.c
@@ -1532,6 +1532,20 @@ static int resizer_set_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 	return 0;
 }
 
+static int resizer_link_validate(struct v4l2_subdev *sd,
+				 struct media_link *link,
+				 struct v4l2_subdev_format *source_fmt,
+				 struct v4l2_subdev_format *sink_fmt)
+{
+	struct isp_res_device *res = v4l2_get_subdevdata(sd);
+	struct isp_pipeline *pipe = to_isp_pipeline(&sd->entity);
+
+	omap3isp_resizer_max_rate(res, &pipe->max_rate);
+
+	return v4l2_subdev_link_validate_default(sd, link,
+						 source_fmt, sink_fmt);
+}
+
 /*
  * resizer_init_formats - Initialize formats on all pads
  * @sd: ISP resizer V4L2 subdevice
@@ -1570,6 +1584,7 @@ static const struct v4l2_subdev_pad_ops resizer_v4l2_pad_ops = {
 	.set_fmt = resizer_set_format,
 	.get_selection = resizer_get_selection,
 	.set_selection = resizer_set_selection,
+	.link_validate = resizer_link_validate,
 };
 
 /* subdev operations */
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index d45af5c..9319e4d 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -278,55 +278,6 @@ static int isp_video_get_graph_data(struct isp_video *video,
 	return 0;
 }
 
-/*
- * Validate a pipeline by checking both ends of all links for format
- * discrepancies.
- *
- * Compute the minimum time per frame value as the maximum of time per frame
- * limits reported by every block in the pipeline.
- *
- * Return 0 if all formats match, or -EPIPE if at least one link is found with
- * different formats on its two ends or if the pipeline doesn't start with a
- * video source (either a subdev with no input pad, or a non-subdev entity).
- */
-static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
-{
-	struct isp_device *isp = pipe->output->isp;
-	struct media_pad *pad;
-	struct v4l2_subdev *subdev;
-
-	subdev = isp_video_remote_subdev(pipe->output, NULL);
-	if (subdev == NULL)
-		return -EPIPE;
-
-	while (1) {
-		/* Retrieve the sink format */
-		pad = &subdev->entity.pads[0];
-		if (!(pad->flags & MEDIA_PAD_FL_SINK))
-			break;
-
-		/* Update the maximum frame rate */
-		if (subdev == &isp->isp_res.subdev)
-			omap3isp_resizer_max_rate(&isp->isp_res,
-						  &pipe->max_rate);
-
-		/* Retrieve the source format. Return an error if no source
-		 * entity can be found, and stop checking the pipeline if the
-		 * source entity isn't a subdev.
-		 */
-		pad = media_entity_remote_pad(pad);
-		if (pad == NULL)
-			return -EPIPE;
-
-		if (media_entity_type(pad->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
-			break;
-
-		subdev = media_entity_to_v4l2_subdev(pad->entity);
-	}
-
-	return 0;
-}
-
 static int
 __isp_video_get_format(struct isp_video *video, struct v4l2_format *format)
 {
@@ -1054,11 +1005,6 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	if (ret < 0)
 		goto err_check_format;
 
-	/* Validate the pipeline and update its state. */
-	ret = isp_video_validate_pipeline(pipe);
-	if (ret < 0)
-		goto err_check_format;
-
 	pipe->error = false;
 
 	spin_lock_irqsave(&pipe->lock, flags);
-- 
1.7.10.4

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

[Laurent Pinchart]

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 */
-- 
Regards,

Laurent Pinchart

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

[Sylwester Nawrocki]

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


-- 
Sylwester Nawrocki
Samsung R&D Institute Poland

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

[Sakari Ailus]

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

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

[Laurent Pinchart]

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.
-- 
Regards,

Laurent Pinchart

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

[Sylwester Nawrocki]

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

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

[Sakari Ailus]

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 <s.nawrocki@samsung.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 */
-- 
1.7.10.4

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

[Sakari Ailus]

Do not allow streaming if a pad with MEDIA_PAD_FL_MUST_CONNECT flag is not
connected by an active link.

This patch makes it possible to avoid drivers having to check for the most
common case of link state validation: a sink pad that must be connected.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Since v2:

- Change Sylwester's e-mail address

 .../DocBook/media/v4l/media-ioc-enum-links.xml     |    3 +-
 drivers/media/media-entity.c                       |   41 ++++++++++++++++----
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
index e357dc9..cf85485 100644
--- a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
+++ b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
@@ -141,8 +141,7 @@
 	    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>
+	    flag doesn't imply there is none.</entry>
 	  </row>
 	</tbody>
       </tgroup>
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 2c286c3..37c334e 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -235,6 +235,8 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
 	media_entity_graph_walk_start(&graph, entity);
 
 	while ((entity = media_entity_graph_walk_next(&graph))) {
+		DECLARE_BITMAP(active, entity->num_pads);
+		DECLARE_BITMAP(has_no_links, entity->num_pads);
 		unsigned int i;
 
 		entity->stream_count++;
@@ -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;
+
+			/* Mark that a pad is connected by a link. */
+			bitmap_clear(has_no_links, pad->index, 1);
+
+			/*
+			 * Pads that either do not need to connect or
+			 * are connected through an enabled link are
+			 * fine.
+			 */
+			if (!(pad->flags & MEDIA_PAD_FL_MUST_CONNECT) ||
+			    link->flags & MEDIA_LNK_FL_ENABLED)
+				bitmap_set(active, pad->index, 1);
+
+			/*
+			 * Link validation will only take place for
+			 * sink ends of the link that are enabled.
+			 */
+			if (link->sink != pad ||
+			    !(link->flags & MEDIA_LNK_FL_ENABLED))
 				continue;
 
 			ret = entity->ops->link_validate(link);
 			if (ret < 0 && ret != -ENOIOCTLCMD)
 				goto error;
 		}
+
+		/* Either no links or validated links are fine. */
+		bitmap_or(active, active, has_no_links, entity->num_pads);
+
+		if (!bitmap_full(active, entity->num_pads)) {
+			ret = -EPIPE;
+			goto error;
+		}
 	}
 
 	mutex_unlock(&mdev->graph_mutex);
-- 
1.7.10.4

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

[Sakari Ailus]

On Sun, Oct 13, 2013 at 01:58:43PM +0300, Sakari Ailus wrote:
> Pads that set this flag must be connected by an active link for the entity
> to stream.

Oh --- btw. what has changed since v2:

- Rmoved the last sentence of MEDIA_PAD_FL_MUST_CONNECT documentation. The
  sentence was about the flag having no effect on pads w/o links.

- Change Sylwester's e-mail address

-- 
Kind regards,

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

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

[Laurent Pinchart]

Hi Sakari,

On Sunday 13 October 2013 14:03:13 Sakari Ailus wrote:
> On Sun, Oct 13, 2013 at 01:58:43PM +0300, Sakari Ailus wrote:
> > Pads that set this flag must be connected by an active link for the entity
> > to stream.
> 
> Oh --- btw. what has changed since v2:
> 
> - Rmoved the last sentence of MEDIA_PAD_FL_MUST_CONNECT documentation. The
>   sentence was about the flag having no effect on pads w/o links.

That change is part of 2/4. I believe that's a mistake.

> - Change Sylwester's e-mail address

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

On Tue, Oct 15, 2013 at 05:22:50PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Sunday 13 October 2013 14:03:13 Sakari Ailus wrote:
> > On Sun, Oct 13, 2013 at 01:58:43PM +0300, Sakari Ailus wrote:
> > > Pads that set this flag must be connected by an active link for the entity
> > > to stream.
> > 
> > Oh --- btw. what has changed since v2:
> > 
> > - Rmoved the last sentence of MEDIA_PAD_FL_MUST_CONNECT documentation. The
> >   sentence was about the flag having no effect on pads w/o links.
> 
> That change is part of 2/4. I believe that's a mistake.

Indeed. I'll resend in a moment.

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

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

[Sakari Ailus]

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 <s.nawrocki@samsung.com>
---
 Documentation/DocBook/media/v4l/media-ioc-enum-links.xml |    9 +++++++++
 include/uapi/linux/media.h                               |    1 +
 2 files changed, 10 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..cf85485 100644
--- a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
+++ b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
@@ -134,6 +134,15 @@
 	    <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.</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 */
-- 
1.7.10.4

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

[Sakari Ailus]

Do not allow streaming if a pad with MEDIA_PAD_FL_MUST_CONNECT flag is not
connected by an active link.

This patch makes it possible to avoid drivers having to check for the most
common case of link state validation: a sink pad that must be connected.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/media-entity.c |   41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 2c286c3..37c334e 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -235,6 +235,8 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
 	media_entity_graph_walk_start(&graph, entity);
 
 	while ((entity = media_entity_graph_walk_next(&graph))) {
+		DECLARE_BITMAP(active, entity->num_pads);
+		DECLARE_BITMAP(has_no_links, entity->num_pads);
 		unsigned int i;
 
 		entity->stream_count++;
@@ -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;
+
+			/* Mark that a pad is connected by a link. */
+			bitmap_clear(has_no_links, pad->index, 1);
+
+			/*
+			 * Pads that either do not need to connect or
+			 * are connected through an enabled link are
+			 * fine.
+			 */
+			if (!(pad->flags & MEDIA_PAD_FL_MUST_CONNECT) ||
+			    link->flags & MEDIA_LNK_FL_ENABLED)
+				bitmap_set(active, pad->index, 1);
+
+			/*
+			 * Link validation will only take place for
+			 * sink ends of the link that are enabled.
+			 */
+			if (link->sink != pad ||
+			    !(link->flags & MEDIA_LNK_FL_ENABLED))
 				continue;
 
 			ret = entity->ops->link_validate(link);
 			if (ret < 0 && ret != -ENOIOCTLCMD)
 				goto error;
 		}
+
+		/* Either no links or validated links are fine. */
+		bitmap_or(active, active, has_no_links, entity->num_pads);
+
+		if (!bitmap_full(active, entity->num_pads)) {
+			ret = -EPIPE;
+			goto error;
+		}
 	}
 
 	mutex_unlock(&mdev->graph_mutex);
-- 
1.7.10.4

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

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Wednesday 16 October 2013 00:46:57 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 <s.nawrocki@samsung.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  Documentation/DocBook/media/v4l/media-ioc-enum-links.xml |    9 +++++++++
>  include/uapi/linux/media.h                               |    1 +
>  2 files changed, 10 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..cf85485 100644
> --- a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> +++ b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
> @@ -134,6 +134,15 @@
>  	    <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.</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 */
-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Wednesday 16 October 2013 00:48:17 Sakari Ailus wrote:
> Do not allow streaming if a pad with MEDIA_PAD_FL_MUST_CONNECT flag is not
> connected by an active link.
> 
> This patch makes it possible to avoid drivers having to check for the most
> common case of link state validation: a sink pad that must be connected.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I've taken the whole series in my tree and will push it upstream for v3.14.

> ---
>  drivers/media/media-entity.c |   41 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 2c286c3..37c334e 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -235,6 +235,8 @@ __must_check int media_entity_pipeline_start(struct
> media_entity *entity, media_entity_graph_walk_start(&graph, entity);
> 
>  	while ((entity = media_entity_graph_walk_next(&graph))) {
> +		DECLARE_BITMAP(active, entity->num_pads);
> +		DECLARE_BITMAP(has_no_links, entity->num_pads);
>  		unsigned int i;
> 
>  		entity->stream_count++;
> @@ -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;
> +
> +			/* Mark that a pad is connected by a link. */
> +			bitmap_clear(has_no_links, pad->index, 1);
> +
> +			/*
> +			 * Pads that either do not need to connect or
> +			 * are connected through an enabled link are
> +			 * fine.
> +			 */
> +			if (!(pad->flags & MEDIA_PAD_FL_MUST_CONNECT) ||
> +			    link->flags & MEDIA_LNK_FL_ENABLED)
> +				bitmap_set(active, pad->index, 1);
> +
> +			/*
> +			 * Link validation will only take place for
> +			 * sink ends of the link that are enabled.
> +			 */
> +			if (link->sink != pad ||
> +			    !(link->flags & MEDIA_LNK_FL_ENABLED))
>  				continue;
> 
>  			ret = entity->ops->link_validate(link);
>  			if (ret < 0 && ret != -ENOIOCTLCMD)
>  				goto error;
>  		}
> +
> +		/* Either no links or validated links are fine. */
> +		bitmap_or(active, active, has_no_links, entity->num_pads);
> +
> +		if (!bitmap_full(active, entity->num_pads)) {
> +			ret = -EPIPE;
> +			goto error;
> +		}
>  	}
> 
>  	mutex_unlock(&mdev->graph_mutex);
-- 
Regards,

Laurent Pinchart