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

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

[Sakari Ailus]

Hi all,

This is a small RFC patchset which adds a new pad flag
MEDIA_PAD_FL_MUST_CONNECT. Pads that have this flag are required to be
connected through an enabled link for the entity to be able to stream. Both
sink and source pads may have this flag, compared to my old patch which
required all sink pads to be connected.

One of the additional benefits is that the users will also know which pads
must be connected which is better than the ah-so-informative -EPIPE. More
complex cases are still left for the driver to implement, though.

The omap3isp driver gets these flags to all of its sink pads. Other drivers
would likely need to be changed by the driver authors since I have little
knowledge of their requirements.

Consequently, an additional loop over the media graph can be avoided in the
omap3isp driver (4th patch) since the driver no longer has the
responsibility to check that its pads are connected.

-- 
Kind regards,
Sakari

[RFC 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>
---
 Documentation/DocBook/media/v4l/media-ioc-enum-links.xml |    5 +++++
 include/uapi/linux/media.h                               |    1 +
 2 files changed, 6 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..4725a99 100644
--- a/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
+++ b/Documentation/DocBook/media/v4l/media-ioc-enum-links.xml
@@ -134,6 +134,11 @@
 	    <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 entity to be able to stream.</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

[RFC 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>
---
 drivers/media/media-entity.c |   34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index cb30ffb..4e58f8a 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -20,6 +20,7 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#include <linux/bitmap.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <media/media-entity.h>
@@ -227,6 +228,7 @@ __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);
 		unsigned int i;
 
 		entity->stream_count++;
@@ -240,21 +242,39 @@ __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);
+
 		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;
+
+			/*
+			 * 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;
 		}
+
+		if (!bitmap_full(active, entity->num_pads)) {
+			ret = -EPIPE;
+			goto error;
+		}
 	}
 
 	mutex_unlock(&mdev->graph_mutex);
-- 
1.7.10.4

[RFC 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>
---
 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..a99dd0a 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..2c652d3 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..58e40b9 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..bdb8fd7 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..6509d66 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..1b0311c 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

[RFC 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 6509d66..336f96a 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 1b0311c..a86ccb8 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: [RFC 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 Friday 19 July 2013 20:55:07 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>
> ---
>  drivers/media/media-entity.c |   34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index cb30ffb..4e58f8a 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -20,6 +20,7 @@
>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 
> USA */
> 
> +#include <linux/bitmap.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <media/media-entity.h>
> @@ -227,6 +228,7 @@ __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);
>  		unsigned int i;
> 
>  		entity->stream_count++;
> @@ -240,21 +242,39 @@ __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);
> +
>  		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;
> +
> +			/*
> +			 * 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;
>  		}
> +
> +		if (!bitmap_full(active, entity->num_pads)) {
> +			ret = -EPIPE;
> +			goto error;
> +		}

I'm afraid that won't work if one of the pads has no links. In that case the 
bitmap won't be full. I think you will have to iterate separately on the links 
to validate them, and on the pads to check the flags.

>  	}
> 
>  	mutex_unlock(&mdev->graph_mutex);
-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

On Fri, Aug 09, 2013 at 01:34:46AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday 19 July 2013 20:55:07 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>
> > ---
> >  drivers/media/media-entity.c |   34 +++++++++++++++++++++++++++-------
> >  1 file changed, 27 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index cb30ffb..4e58f8a 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -20,6 +20,7 @@
> >   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 
> > USA */
> > 
> > +#include <linux/bitmap.h>
> >  #include <linux/module.h>
> >  #include <linux/slab.h>
> >  #include <media/media-entity.h>
> > @@ -227,6 +228,7 @@ __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);
> >  		unsigned int i;
> > 
> >  		entity->stream_count++;
> > @@ -240,21 +242,39 @@ __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);
> > +
> >  		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;
> > +
> > +			/*
> > +			 * 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;
> >  		}
> > +
> > +		if (!bitmap_full(active, entity->num_pads)) {
> > +			ret = -EPIPE;
> > +			goto error;
> > +		}
> 
> I'm afraid that won't work if one of the pads has no links. In that case the 
> bitmap won't be full. I think you will have to iterate separately on the links 
> to validate them, and on the pads to check the flags.

Good point.

I could as well add another bitmap or perhaps rather keep the information in
struct media_pad, separately from flags since this is something the user can
find through other means --- that way we don't add practically any runtime
overhead.

What do you think?

-- 
Kind regards,

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

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

[Laurent Pinchart]

Hi Sakari,

On Saturday 10 August 2013 15:16:29 Sakari Ailus wrote:
> On Fri, Aug 09, 2013 at 01:34:46AM +0200, Laurent Pinchart wrote:
> > On Friday 19 July 2013 20:55:07 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>
> > > ---
> > > 
> > >  drivers/media/media-entity.c |   34 +++++++++++++++++++++++++++-------
> > >  1 file changed, 27 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > > index cb30ffb..4e58f8a 100644
> > > --- a/drivers/media/media-entity.c
> > > +++ b/drivers/media/media-entity.c
> > > @@ -20,6 +20,7 @@
> > >   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> > > USA */
> > > 
> > > +#include <linux/bitmap.h>
> > >  #include <linux/module.h>
> > >  #include <linux/slab.h>
> > >  #include <media/media-entity.h>
> > > @@ -227,6 +228,7 @@ __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);
> > >  		unsigned int i;
> > >  		
> > >  		entity->stream_count++;
> > > 
> > > @@ -240,21 +242,39 @@ __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);
> > > +
> > >  		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;
> > > +
> > > +			/*
> > > +			 * 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;
> > >  		}
> > > +
> > > +		if (!bitmap_full(active, entity->num_pads)) {
> > > +			ret = -EPIPE;
> > > +			goto error;
> > > +		}
> > 
> > I'm afraid that won't work if one of the pads has no links. In that case
> > the bitmap won't be full. I think you will have to iterate separately on
> > the links to validate them, and on the pads to check the flags.
> 
> Good point.
> 
> I could as well add another bitmap or perhaps rather keep the information in
> struct media_pad, separately from flags since this is something the user can
> find through other means --- that way we don't add practically any runtime
> overhead.
> 
> What do you think?

Given the number of pads we expect on an entity, is it really worth the micro-
optimization ? I think I would just loop over the links and pads separately, 
especially given that you need to validate pads even if the entity has no 
link_validate operation, which is checked before the for loop over the links.

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

On Wed, Aug 21, 2013 at 05:22:45AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Saturday 10 August 2013 15:16:29 Sakari Ailus wrote:
> > On Fri, Aug 09, 2013 at 01:34:46AM +0200, Laurent Pinchart wrote:
> > > On Friday 19 July 2013 20:55:07 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>
> > > > ---
> > > > 
> > > >  drivers/media/media-entity.c |   34 +++++++++++++++++++++++++++-------
> > > >  1 file changed, 27 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > > > index cb30ffb..4e58f8a 100644
> > > > --- a/drivers/media/media-entity.c
> > > > +++ b/drivers/media/media-entity.c
> > > > @@ -20,6 +20,7 @@
> > > >   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> > > > USA */
> > > > 
> > > > +#include <linux/bitmap.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/slab.h>
> > > >  #include <media/media-entity.h>
> > > > @@ -227,6 +228,7 @@ __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);
> > > >  		unsigned int i;
> > > >  		
> > > >  		entity->stream_count++;
> > > > 
> > > > @@ -240,21 +242,39 @@ __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);
> > > > +
> > > >  		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;
> > > > +
> > > > +			/*
> > > > +			 * 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;
> > > >  		}
> > > > +
> > > > +		if (!bitmap_full(active, entity->num_pads)) {
> > > > +			ret = -EPIPE;
> > > > +			goto error;
> > > > +		}
> > > 
> > > I'm afraid that won't work if one of the pads has no links. In that case
> > > the bitmap won't be full. I think you will have to iterate separately on
> > > the links to validate them, and on the pads to check the flags.
> > 
> > Good point.
> > 
> > I could as well add another bitmap or perhaps rather keep the information in
> > struct media_pad, separately from flags since this is something the user can
> > find through other means --- that way we don't add practically any runtime
> > overhead.
> > 
> > What do you think?
> 
> Given the number of pads we expect on an entity, is it really worth the micro-
> optimization ? I think I would just loop over the links and pads separately, 
> especially given that you need to validate pads even if the entity has no 
> link_validate operation, which is checked before the for loop over the links.

I'm fine with that. Even if the information was kept in struct media_pad, a
loop would be needed to dig it out. Alternatively a bitmap could be put to
struct media_entity but that'd be less pretty.

I'll just add another bitmap here. It can later be moved to struct
media_entity if needed.

-- 
Cheers,

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

[PATCH v1.1 3/5] media: Pads that are not connected by even a disabled link are fine

[Sakari Ailus]

Do not require a connected link to a pad if a pad has no links connected to
it.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
Hi Laurent,

This goes on top of patch 2/4. I can combine the two in the end but I think
this is cleaner as a separate change.

 drivers/media/media-entity.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index a99396b..2ad291f 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -236,6 +236,7 @@ __must_check int media_entity_pipeline_start(struct media_entity *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++;
@@ -250,6 +251,7 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
 			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];
@@ -257,6 +259,11 @@ __must_check int media_entity_pipeline_start(struct media_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.
@@ -278,6 +285,9 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
 				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;
-- 
1.7.10.4

Re: [PATCH v1.1 3/5] media: Pads that are not connected by even a disabled link are fine

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Saturday 31 August 2013 19:28:06 Sakari Ailus wrote:
> Do not require a connected link to a pad if a pad has no links connected to
> it.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
> Hi Laurent,
> 
> This goes on top of patch 2/4. I can combine the two in the end but I think
> this is cleaner as a separate change.

Merging the patches separately could result in a bisection breakage, so I'd 
rather combine the patches if that's OK. What about the following commit 
message ?

"media: Check for active links on pads with MEDIA_PAD_FL_MUST_CONNECT flag

Do not allow streaming if a pad with MEDIA_PAD_FL_MUST_CONNECT flag is 
connected by links that are all inactive.

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

>  drivers/media/media-entity.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index a99396b..2ad291f 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -236,6 +236,7 @@ __must_check int media_entity_pipeline_start(struct
> media_entity *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++;
> @@ -250,6 +251,7 @@ __must_check int media_entity_pipeline_start(struct
> media_entity *entity, 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];
> @@ -257,6 +259,11 @@ __must_check int media_entity_pipeline_start(struct
> media_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.
> @@ -278,6 +285,9 @@ __must_check int media_entity_pipeline_start(struct
> media_entity *entity, 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;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v1.1 3/5] media: Pads that are not connected by even a disabled link are fine

[Sakari Ailus]

Hi Laurent,

Thanks for the comments!!

On Tue, Sep 03, 2013 at 08:07:43PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Saturday 31 August 2013 19:28:06 Sakari Ailus wrote:
> > Do not require a connected link to a pad if a pad has no links connected to
> > it.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> > Hi Laurent,
> > 
> > This goes on top of patch 2/4. I can combine the two in the end but I think
> > this is cleaner as a separate change.
> 
> Merging the patches separately could result in a bisection breakage, so I'd 
> rather combine the patches if that's OK. What about the following commit 
> message ?

I'm certainly fine with that. Let me resend the patch.

> "media: Check for active links on pads with MEDIA_PAD_FL_MUST_CONNECT flag
> 
> Do not allow streaming if a pad with MEDIA_PAD_FL_MUST_CONNECT flag is 
> connected by links that are all inactive.
> 
> 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."

-- 
Kind regards,

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

[RFC v1.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
connected by links that are all inactive.

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>
---
 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..567a171 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: [RFC v1.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 04 September 2013 03:09:42 Sakari Ailus wrote:
> Do not allow streaming if a pad with MEDIA_PAD_FL_MUST_CONNECT flag is
> connected by links that are all inactive.
> 
> 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>
> ---
>  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..567a171 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)

With the || moved on the previous line (here and below),

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

If that's fine with you there's no need to resent, I'll take the patch in my 
tree with that modification.

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

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

[Sakari Ailus]

Hi Laurent,

Thanks for the reply.

On Fri, Sep 06, 2013 at 06:30:47PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wednesday 04 September 2013 03:09:42 Sakari Ailus wrote:
> > Do not allow streaming if a pad with MEDIA_PAD_FL_MUST_CONNECT flag is
> > connected by links that are all inactive.
> > 
> > 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>
> > ---
> >  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..567a171 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)
> 
> With the || moved on the previous line (here and below),

I'd like to claim the above is more clear and conforms to GNU coding
standards whereas the kernel CodingStyle doesn't suggest either way:

<URL:http://www.gnu.org/prep/standards/standards.html#Formatting>

I feel we've had this discussion before. :-)

I have to admit I disagree with Stallman's liberal use of parentheses and a
funny habit of having a space between a function name and the parentheses
enclosing its arguments. ;-)

> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> If that's fine with you there's no need to resent, I'll take the patch in my 
> tree with that modification.

I won't say no, but I would prefer getting an ack from someone else, too,
for the two first patches. A less permissive variant of this was dismissed
around a year ago, potentially perhaps there was no pressing need for that
kind of a change. The earlier set required all sink pads must have a
connected link (which indeed was too restrictive, I agree).

<URL:https://linuxtv.org/patch/15205/>

As seen in the fourth patch of this set, this makes it possible to avoid
another loop (done in the driver) over the pipeline, which was my motivation
for this patch --- so I think this is the right thing to do.

Other drivers could be changed the same way but I think this is up to the
driver authors. There could be other reasons for the pad to need connecting;
tha absence of the flag won't say there aren't.

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

-- 
Kind regards,

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

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

[Sakari Ailus]

Ping.

On Fri, Sep 06, 2013 at 08:10:12PM +0300, Sakari Ailus wrote:
> Hi Laurent,
> 
> Thanks for the reply.
> 
> On Fri, Sep 06, 2013 at 06:30:47PM +0200, Laurent Pinchart wrote:
> > Hi Sakari,
> > 
> > Thank you for the patch.
> > 
> > On Wednesday 04 September 2013 03:09:42 Sakari Ailus wrote:
> > > Do not allow streaming if a pad with MEDIA_PAD_FL_MUST_CONNECT flag is
> > > connected by links that are all inactive.
> > > 
> > > 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>
> > > ---
> > >  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..567a171 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)
> > 
> > With the || moved on the previous line (here and below),
> 
> I'd like to claim the above is more clear and conforms to GNU coding
> standards whereas the kernel CodingStyle doesn't suggest either way:
> 
> <URL:http://www.gnu.org/prep/standards/standards.html#Formatting>
> 
> I feel we've had this discussion before. :-)
> 
> I have to admit I disagree with Stallman's liberal use of parentheses and a
> funny habit of having a space between a function name and the parentheses
> enclosing its arguments. ;-)
> 
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > If that's fine with you there's no need to resent, I'll take the patch in my 
> > tree with that modification.
> 
> I won't say no, but I would prefer getting an ack from someone else, too,
> for the two first patches. A less permissive variant of this was dismissed
> around a year ago, potentially perhaps there was no pressing need for that
> kind of a change. The earlier set required all sink pads must have a
> connected link (which indeed was too restrictive, I agree).
> 
> <URL:https://linuxtv.org/patch/15205/>
> 
> As seen in the fourth patch of this set, this makes it possible to avoid
> another loop (done in the driver) over the pipeline, which was my motivation
> for this patch --- so I think this is the right thing to do.
> 
> Other drivers could be changed the same way but I think this is up to the
> driver authors. There could be other reasons for the pad to need connecting;
> tha absence of the flag won't say there aren't.
> 
> > > +				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);
> 
> -- 
> Kind regards,
> 
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

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