[REVIEW PATCH v3 0/2] Media link_notify behaviour change and exynos4-is updates

[REVIEW PATCH v3 0/2] Media link_notify behaviour change and exynos4-is updates

[Sylwester Nawrocki]

Hi All,

This is an updated version of the patch set [1], I've dropped patches 
01/11...09/11, which I've already queued for 3.11. This series sits on 
top of my for-next branch [2].

This iteration attempts to address issues pointed out by Sakari, 
thanks! The changes since v2 were:

 - link_notify callback 'flags' argument's type changed to u32,
 - in the omap3isp driver link->flags checked instead of the passed 
   flags argument of the link_notify handler to see if the pipeline 
   should be powered off,
 - in the exynos4-is driver link->flags checked instead of the flags 
   argument of the fimc_md_link_notify() handler to see if the pipelines 
   should be powered on.

Thanks,
Sylwester

[1] http://www.spinics.net/lists/linux-media/msg64258.html
[2] http://git.linuxtv.org/snawrocki/samsung.git/for-next

Sylwester Nawrocki (2):
  media: Change media device link_notify behaviour
  exynos4-is: Extend link_notify handler to support fimc-is/lite
    pipelines

 drivers/media/media-entity.c                  |   18 +---
 drivers/media/platform/exynos4-is/media-dev.c |  101 ++++++++++++++++++++-----
 drivers/media/platform/omap3isp/isp.c         |   41 ++++++----
 include/media/media-device.h                  |    9 ++-
 4 files changed, 118 insertions(+), 51 deletions(-)

-- 
1.7.4.1

[REVIEW PATCH v3 1/2] media: Change media device link_notify behaviour

[Sylwester Nawrocki]

Currently the media device link_notify callback is invoked before the
actual change of state of a link when the link is being enabled, and
after the actual change of state when the link is being disabled.

This doesn't allow a media device driver to perform any operations
on a full graph before a link is disabled, as well as performing
any tasks on a modified graph right after a link's state is changed.

This patch modifies signature of the link_notify callback. This
callback is now called always before and after a link's state change.
To distinguish the notifications a 'notification' argument is added
to the link_notify callback: MEDIA_DEV_NOTIFY_PRE_LINK_CH indicates
notification before link's state change and
MEDIA_DEV_NOTIFY_POST_LINK_CH corresponds to a notification after
link flags change.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes since v1:
 - link_notify callback 'flags' argument's type changed to u32,
 - in the omap3isp driver check link->flags instead of the passed flags
   argument of the link_notify handler to see if pipeline should be
   powered off,
-  use link->flags to check link's state in the fimc_md_link_notify()
   handler instead of link_notify 'flags' argument.
---
 drivers/media/media-entity.c                  |   18 +++--------
 drivers/media/platform/exynos4-is/media-dev.c |   18 ++++++-----
 drivers/media/platform/omap3isp/isp.c         |   41 +++++++++++++++----------
 include/media/media-device.h                  |    9 ++++-
 4 files changed, 47 insertions(+), 39 deletions(-)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index e1cd132..7004cb0 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -496,25 +496,17 @@ int __media_entity_setup_link(struct media_link *link, u32 flags)
 
 	mdev = source->parent;
 
-	if ((flags & MEDIA_LNK_FL_ENABLED) && mdev->link_notify) {
-		ret = mdev->link_notify(link->source, link->sink,
-					MEDIA_LNK_FL_ENABLED);
+	if (mdev->link_notify) {
+		ret = mdev->link_notify(link, flags,
+					MEDIA_DEV_NOTIFY_PRE_LINK_CH);
 		if (ret < 0)
 			return ret;
 	}
 
 	ret = __media_entity_setup_link_notify(link, flags);
-	if (ret < 0)
-		goto err;
 
-	if (!(flags & MEDIA_LNK_FL_ENABLED) && mdev->link_notify)
-		mdev->link_notify(link->source, link->sink, 0);
-
-	return 0;
-
-err:
-	if ((flags & MEDIA_LNK_FL_ENABLED) && mdev->link_notify)
-		mdev->link_notify(link->source, link->sink, 0);
+	if (mdev->link_notify)
+		mdev->link_notify(link, flags, MEDIA_DEV_NOTIFY_POST_LINK_CH);
 
 	return ret;
 }
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index 424ff92..045a6ae 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -1287,34 +1287,36 @@ int fimc_md_set_camclk(struct v4l2_subdev *sd, bool on)
 	return __fimc_md_set_camclk(fmd, si, on);
 }
 
-static int fimc_md_link_notify(struct media_pad *source,
-			       struct media_pad *sink, u32 flags)
+static int fimc_md_link_notify(struct media_link *link, u32 flags,
+						unsigned int notification)
 {
+	struct media_entity *sink = link->sink->entity;
 	struct exynos_video_entity *ve;
 	struct video_device *vdev;
 	struct fimc_pipeline *pipeline;
 	int i, ret = 0;
 
-	if (media_entity_type(sink->entity) != MEDIA_ENT_T_DEVNODE_V4L)
+	if (media_entity_type(sink) != MEDIA_ENT_T_DEVNODE_V4L ||
+	    notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH)
 		return 0;
 
-	vdev = media_entity_to_video_device(sink->entity);
+	vdev = media_entity_to_video_device(sink);
 	ve = vdev_to_exynos_video_entity(vdev);
 	pipeline = to_fimc_pipeline(ve->pipe);
 
-	if (!(flags & MEDIA_LNK_FL_ENABLED) && pipeline->subdevs[IDX_SENSOR]) {
-		if (sink->entity->use_count > 0)
+	if (!(link->flags & MEDIA_LNK_FL_ENABLED) && pipeline->subdevs[IDX_SENSOR]) {
+		if (sink->use_count > 0)
 			ret = __fimc_pipeline_close(ve->pipe);
 
 		for (i = 0; i < IDX_MAX; i++)
 			pipeline->subdevs[i] = NULL;
-	} else if (sink->entity->use_count > 0) {
+	} else if (sink->use_count > 0) {
 		/*
 		 * Link activation. Enable power of pipeline elements only if
 		 * the pipeline is already in use, i.e. its video node is open.
 		 * Recreate the controls destroyed during the link deactivation.
 		 */
-		ret = __fimc_pipeline_open(ve->pipe, sink->entity, true);
+		ret = __fimc_pipeline_open(ve->pipe, sink, true);
 	}
 
 	return ret ? -EPIPE : ret;
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 1d7dbd5..1a2d25c 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -792,9 +792,9 @@ int omap3isp_pipeline_pm_use(struct media_entity *entity, int use)
 
 /*
  * isp_pipeline_link_notify - Link management notification callback
- * @source: Pad at the start of the link
- * @sink: Pad at the end of the link
+ * @link: The link
  * @flags: New link flags that will be applied
+ * @notification: The link's state change notification type (MEDIA_DEV_NOTIFY_*)
  *
  * React to link management on powered pipelines by updating the use count of
  * all entities in the source and sink sides of the link. Entities are powered
@@ -804,29 +804,38 @@ int omap3isp_pipeline_pm_use(struct media_entity *entity, int use)
  * off is assumed to never fail. This function will not fail for disconnection
  * events.
  */
-static int isp_pipeline_link_notify(struct media_pad *source,
-				    struct media_pad *sink, u32 flags)
+static int isp_pipeline_link_notify(struct media_link *link, u32 flags,
+				    unsigned int notification)
 {
-	int source_use = isp_pipeline_pm_use_count(source->entity);
-	int sink_use = isp_pipeline_pm_use_count(sink->entity);
+	struct media_entity *source = link->source->entity;
+	struct media_entity *sink = link->sink->entity;
+	int source_use = isp_pipeline_pm_use_count(source);
+	int sink_use = isp_pipeline_pm_use_count(sink);
 	int ret;
 
-	if (!(flags & MEDIA_LNK_FL_ENABLED)) {
+	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
+	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
 		/* Powering off entities is assumed to never fail. */
-		isp_pipeline_pm_power(source->entity, -sink_use);
-		isp_pipeline_pm_power(sink->entity, -source_use);
+		isp_pipeline_pm_power(source, -sink_use);
+		isp_pipeline_pm_power(sink, -source_use);
 		return 0;
 	}
 
-	ret = isp_pipeline_pm_power(source->entity, sink_use);
-	if (ret < 0)
-		return ret;
+	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
+		(flags & MEDIA_LNK_FL_ENABLED)) {
 
-	ret = isp_pipeline_pm_power(sink->entity, source_use);
-	if (ret < 0)
-		isp_pipeline_pm_power(source->entity, -sink_use);
+		ret = isp_pipeline_pm_power(source, sink_use);
+		if (ret < 0)
+			return ret;
 
-	return ret;
+		ret = isp_pipeline_pm_power(sink, source_use);
+		if (ret < 0)
+			isp_pipeline_pm_power(source, -sink_use);
+
+		return ret;
+	}
+
+	return 0;
 }
 
 /* -----------------------------------------------------------------------------
diff --git a/include/media/media-device.h b/include/media/media-device.h
index eaade98..12155a9 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -45,6 +45,7 @@ struct device;
  * @entities:	List of registered entities
  * @lock:	Entities list lock
  * @graph_mutex: Entities graph operation lock
+ * @link_notify: Link state change notification callback
  *
  * This structure represents an abstract high-level media device. It allows easy
  * access to entities and provides basic media device-level support. The
@@ -75,10 +76,14 @@ struct media_device {
 	/* Serializes graph operations. */
 	struct mutex graph_mutex;
 
-	int (*link_notify)(struct media_pad *source,
-			   struct media_pad *sink, u32 flags);
+	int (*link_notify)(struct media_link *link, u32 flags,
+			   unsigned int notification);
 };
 
+/* Supported link_notify @notification values. */
+#define MEDIA_DEV_NOTIFY_PRE_LINK_CH	0
+#define MEDIA_DEV_NOTIFY_POST_LINK_CH	1
+
 /* media_devnode to media_device */
 #define to_media_device(node) container_of(node, struct media_device, devnode)
 
-- 
1.7.4.1

Re: [REVIEW PATCH v3 1/2] media: Change media device link_notify behaviour

[Laurent Pinchart]

Hi Sylwester,

Thanks for the patch.

On Sunday 09 June 2013 22:14:37 Sylwester Nawrocki wrote:
> Currently the media device link_notify callback is invoked before the
> actual change of state of a link when the link is being enabled, and
> after the actual change of state when the link is being disabled.
> 
> This doesn't allow a media device driver to perform any operations
> on a full graph before a link is disabled, as well as performing
> any tasks on a modified graph right after a link's state is changed.
> 
> This patch modifies signature of the link_notify callback. This
> callback is now called always before and after a link's state change.
> To distinguish the notifications a 'notification' argument is added
> to the link_notify callback: MEDIA_DEV_NOTIFY_PRE_LINK_CH indicates
> notification before link's state change and
> MEDIA_DEV_NOTIFY_POST_LINK_CH corresponds to a notification after
> link flags change.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

This looks good to me. For the media controller core and omap3isp driver,

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

I'd like to have Sakari's ack as well.

> ---
> Changes since v1:
>  - link_notify callback 'flags' argument's type changed to u32,
>  - in the omap3isp driver check link->flags instead of the passed flags
>    argument of the link_notify handler to see if pipeline should be
>    powered off,
> -  use link->flags to check link's state in the fimc_md_link_notify()
>    handler instead of link_notify 'flags' argument.
> ---
>  drivers/media/media-entity.c                  |   18 +++--------
>  drivers/media/platform/exynos4-is/media-dev.c |   18 ++++++-----
>  drivers/media/platform/omap3isp/isp.c         |   41 ++++++++++++----------
>  include/media/media-device.h                  |    9 ++++-
>  4 files changed, 47 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index e1cd132..7004cb0 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -496,25 +496,17 @@ int __media_entity_setup_link(struct media_link *link,
> u32 flags)
> 
>  	mdev = source->parent;
> 
> -	if ((flags & MEDIA_LNK_FL_ENABLED) && mdev->link_notify) {
> -		ret = mdev->link_notify(link->source, link->sink,
> -					MEDIA_LNK_FL_ENABLED);
> +	if (mdev->link_notify) {
> +		ret = mdev->link_notify(link, flags,
> +					MEDIA_DEV_NOTIFY_PRE_LINK_CH);
>  		if (ret < 0)
>  			return ret;
>  	}
> 
>  	ret = __media_entity_setup_link_notify(link, flags);
> -	if (ret < 0)
> -		goto err;
> 
> -	if (!(flags & MEDIA_LNK_FL_ENABLED) && mdev->link_notify)
> -		mdev->link_notify(link->source, link->sink, 0);
> -
> -	return 0;
> -
> -err:
> -	if ((flags & MEDIA_LNK_FL_ENABLED) && mdev->link_notify)
> -		mdev->link_notify(link->source, link->sink, 0);
> +	if (mdev->link_notify)
> +		mdev->link_notify(link, flags, MEDIA_DEV_NOTIFY_POST_LINK_CH);
> 
>  	return ret;
>  }
> diff --git a/drivers/media/platform/exynos4-is/media-dev.c
> b/drivers/media/platform/exynos4-is/media-dev.c index 424ff92..045a6ae
> 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -1287,34 +1287,36 @@ int fimc_md_set_camclk(struct v4l2_subdev *sd, bool
> on) return __fimc_md_set_camclk(fmd, si, on);
>  }
> 
> -static int fimc_md_link_notify(struct media_pad *source,
> -			       struct media_pad *sink, u32 flags)
> +static int fimc_md_link_notify(struct media_link *link, u32 flags,
> +						unsigned int notification)
>  {
> +	struct media_entity *sink = link->sink->entity;
>  	struct exynos_video_entity *ve;
>  	struct video_device *vdev;
>  	struct fimc_pipeline *pipeline;
>  	int i, ret = 0;
> 
> -	if (media_entity_type(sink->entity) != MEDIA_ENT_T_DEVNODE_V4L)
> +	if (media_entity_type(sink) != MEDIA_ENT_T_DEVNODE_V4L ||
> +	    notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH)
>  		return 0;
> 
> -	vdev = media_entity_to_video_device(sink->entity);
> +	vdev = media_entity_to_video_device(sink);
>  	ve = vdev_to_exynos_video_entity(vdev);
>  	pipeline = to_fimc_pipeline(ve->pipe);
> 
> -	if (!(flags & MEDIA_LNK_FL_ENABLED) && pipeline->subdevs[IDX_SENSOR]) {
> -		if (sink->entity->use_count > 0)
> +	if (!(link->flags & MEDIA_LNK_FL_ENABLED) &&
> pipeline->subdevs[IDX_SENSOR]) { +		if (sink->use_count > 0)
>  			ret = __fimc_pipeline_close(ve->pipe);
> 
>  		for (i = 0; i < IDX_MAX; i++)
>  			pipeline->subdevs[i] = NULL;
> -	} else if (sink->entity->use_count > 0) {
> +	} else if (sink->use_count > 0) {
>  		/*
>  		 * Link activation. Enable power of pipeline elements only if
>  		 * the pipeline is already in use, i.e. its video node is open.
>  		 * Recreate the controls destroyed during the link deactivation.
>  		 */
> -		ret = __fimc_pipeline_open(ve->pipe, sink->entity, true);
> +		ret = __fimc_pipeline_open(ve->pipe, sink, true);
>  	}
> 
>  	return ret ? -EPIPE : ret;
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 1d7dbd5..1a2d25c 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -792,9 +792,9 @@ int omap3isp_pipeline_pm_use(struct media_entity
> *entity, int use)
> 
>  /*
>   * isp_pipeline_link_notify - Link management notification callback
> - * @source: Pad at the start of the link
> - * @sink: Pad at the end of the link
> + * @link: The link
>   * @flags: New link flags that will be applied
> + * @notification: The link's state change notification type
> (MEDIA_DEV_NOTIFY_*) *
>   * React to link management on powered pipelines by updating the use count
> of * all entities in the source and sink sides of the link. Entities are
> powered @@ -804,29 +804,38 @@ int omap3isp_pipeline_pm_use(struct
> media_entity *entity, int use) * off is assumed to never fail. This
> function will not fail for disconnection * events.
>   */
> -static int isp_pipeline_link_notify(struct media_pad *source,
> -				    struct media_pad *sink, u32 flags)
> +static int isp_pipeline_link_notify(struct media_link *link, u32 flags,
> +				    unsigned int notification)
>  {
> -	int source_use = isp_pipeline_pm_use_count(source->entity);
> -	int sink_use = isp_pipeline_pm_use_count(sink->entity);
> +	struct media_entity *source = link->source->entity;
> +	struct media_entity *sink = link->sink->entity;
> +	int source_use = isp_pipeline_pm_use_count(source);
> +	int sink_use = isp_pipeline_pm_use_count(sink);
>  	int ret;
> 
> -	if (!(flags & MEDIA_LNK_FL_ENABLED)) {
> +	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> +	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
>  		/* Powering off entities is assumed to never fail. */
> -		isp_pipeline_pm_power(source->entity, -sink_use);
> -		isp_pipeline_pm_power(sink->entity, -source_use);
> +		isp_pipeline_pm_power(source, -sink_use);
> +		isp_pipeline_pm_power(sink, -source_use);
>  		return 0;
>  	}
> 
> -	ret = isp_pipeline_pm_power(source->entity, sink_use);
> -	if (ret < 0)
> -		return ret;
> +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
> +		(flags & MEDIA_LNK_FL_ENABLED)) {
> 
> -	ret = isp_pipeline_pm_power(sink->entity, source_use);
> -	if (ret < 0)
> -		isp_pipeline_pm_power(source->entity, -sink_use);
> +		ret = isp_pipeline_pm_power(source, sink_use);
> +		if (ret < 0)
> +			return ret;
> 
> -	return ret;
> +		ret = isp_pipeline_pm_power(sink, source_use);
> +		if (ret < 0)
> +			isp_pipeline_pm_power(source, -sink_use);
> +
> +		return ret;
> +	}
> +
> +	return 0;
>  }
> 
>  /*
> ---------------------------------------------------------------------------
> -- diff --git a/include/media/media-device.h b/include/media/media-device.h
> index eaade98..12155a9 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -45,6 +45,7 @@ struct device;
>   * @entities:	List of registered entities
>   * @lock:	Entities list lock
>   * @graph_mutex: Entities graph operation lock
> + * @link_notify: Link state change notification callback
>   *
>   * This structure represents an abstract high-level media device. It allows
> easy * access to entities and provides basic media device-level support.
> The @@ -75,10 +76,14 @@ struct media_device {
>  	/* Serializes graph operations. */
>  	struct mutex graph_mutex;
> 
> -	int (*link_notify)(struct media_pad *source,
> -			   struct media_pad *sink, u32 flags);
> +	int (*link_notify)(struct media_link *link, u32 flags,
> +			   unsigned int notification);
>  };
> 
> +/* Supported link_notify @notification values. */
> +#define MEDIA_DEV_NOTIFY_PRE_LINK_CH	0
> +#define MEDIA_DEV_NOTIFY_POST_LINK_CH	1
> +
>  /* media_devnode to media_device */
>  #define to_media_device(node) container_of(node, struct media_device,
> devnode)
-- 
Regards,

Laurent Pinchart

Re: [REVIEW PATCH v3 1/2] media: Change media device link_notify behaviour

[Sakari Ailus]

Hi Sylwester,

Thanks for the update.

Sylwester Nawrocki wrote:
...
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 1d7dbd5..1a2d25c 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -792,9 +792,9 @@ int omap3isp_pipeline_pm_use(struct media_entity *entity, int use)
>
>   /*
>    * isp_pipeline_link_notify - Link management notification callback
> - * @source: Pad at the start of the link
> - * @sink: Pad at the end of the link
> + * @link: The link
>    * @flags: New link flags that will be applied
> + * @notification: The link's state change notification type (MEDIA_DEV_NOTIFY_*)
>    *
>    * React to link management on powered pipelines by updating the use count of
>    * all entities in the source and sink sides of the link. Entities are powered
> @@ -804,29 +804,38 @@ int omap3isp_pipeline_pm_use(struct media_entity *entity, int use)
>    * off is assumed to never fail. This function will not fail for disconnection
>    * events.
>    */
> -static int isp_pipeline_link_notify(struct media_pad *source,
> -				    struct media_pad *sink, u32 flags)
> +static int isp_pipeline_link_notify(struct media_link *link, u32 flags,
> +				    unsigned int notification)
>   {
> -	int source_use = isp_pipeline_pm_use_count(source->entity);
> -	int sink_use = isp_pipeline_pm_use_count(sink->entity);
> +	struct media_entity *source = link->source->entity;
> +	struct media_entity *sink = link->sink->entity;
> +	int source_use = isp_pipeline_pm_use_count(source);
> +	int sink_use = isp_pipeline_pm_use_count(sink);
>   	int ret;
>
> -	if (!(flags & MEDIA_LNK_FL_ENABLED)) {
> +	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> +	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
>   		/* Powering off entities is assumed to never fail. */
> -		isp_pipeline_pm_power(source->entity, -sink_use);
> -		isp_pipeline_pm_power(sink->entity, -source_use);
> +		isp_pipeline_pm_power(source, -sink_use);
> +		isp_pipeline_pm_power(sink, -source_use);
>   		return 0;
>   	}
>
> -	ret = isp_pipeline_pm_power(source->entity, sink_use);
> -	if (ret < 0)
> -		return ret;
> +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
> +		(flags & MEDIA_LNK_FL_ENABLED)) {

You could return zero here if the opposite was true, and unindent the 
rest. Up to you --- the patch is fine.

Acked-by: Sakari Ailus <sakari.ailus@iki.fi>

> -	ret = isp_pipeline_pm_power(sink->entity, source_use);
> -	if (ret < 0)
> -		isp_pipeline_pm_power(source->entity, -sink_use);
> +		ret = isp_pipeline_pm_power(source, sink_use);
> +		if (ret < 0)
> +			return ret;
>
> -	return ret;
> +		ret = isp_pipeline_pm_power(sink, source_use);
> +		if (ret < 0)
> +			isp_pipeline_pm_power(source, -sink_use);
> +
> +		return ret;
> +	}
> +
> +	return 0;
>   }
>
>   /* -----------------------------------------------------------------------------

-- 
Kind regards,

Sakari Ailus
sakari.ailus@iki.fi

Re: [REVIEW PATCH v3 1/2] media: Change media device link_notify behaviour

[Laurent Pinchart]

Hi Sylwester,

Should I take the series in my tree, or would you like to push it yourself to 
avoid conflicts with other Exynos patches ?

On Monday 10 June 2013 01:10:30 Sakari Ailus wrote:
> Hi Sylwester,
> 
> Thanks for the update.
> 
> Sylwester Nawrocki wrote:
> ...
> 
> > diff --git a/drivers/media/platform/omap3isp/isp.c
> > b/drivers/media/platform/omap3isp/isp.c index 1d7dbd5..1a2d25c 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -792,9 +792,9 @@ int omap3isp_pipeline_pm_use(struct media_entity
> > *entity, int use)> 
> >   /*
> >   
> >    * isp_pipeline_link_notify - Link management notification callback
> > 
> > - * @source: Pad at the start of the link
> > - * @sink: Pad at the end of the link
> > + * @link: The link
> > 
> >    * @flags: New link flags that will be applied
> > 
> > + * @notification: The link's state change notification type
> > (MEDIA_DEV_NOTIFY_*)> 
> >    *
> >    * React to link management on powered pipelines by updating the use
> >    count of * all entities in the source and sink sides of the link.
> >    Entities are powered> 
> > @@ -804,29 +804,38 @@ int omap3isp_pipeline_pm_use(struct media_entity
> > *entity, int use)> 
> >    * off is assumed to never fail. This function will not fail for
> >    disconnection * events.
> >    */
> > 
> > -static int isp_pipeline_link_notify(struct media_pad *source,
> > -				    struct media_pad *sink, u32 flags)
> > +static int isp_pipeline_link_notify(struct media_link *link, u32 flags,
> > +				    unsigned int notification)
> > 
> >   {
> > 
> > -	int source_use = isp_pipeline_pm_use_count(source->entity);
> > -	int sink_use = isp_pipeline_pm_use_count(sink->entity);
> > +	struct media_entity *source = link->source->entity;
> > +	struct media_entity *sink = link->sink->entity;
> > +	int source_use = isp_pipeline_pm_use_count(source);
> > +	int sink_use = isp_pipeline_pm_use_count(sink);
> > 
> >   	int ret;
> > 
> > -	if (!(flags & MEDIA_LNK_FL_ENABLED)) {
> > +	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> > +	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
> > 
> >   		/* Powering off entities is assumed to never fail. */
> > 
> > -		isp_pipeline_pm_power(source->entity, -sink_use);
> > -		isp_pipeline_pm_power(sink->entity, -source_use);
> > +		isp_pipeline_pm_power(source, -sink_use);
> > +		isp_pipeline_pm_power(sink, -source_use);
> > 
> >   		return 0;
> >   	
> >   	}
> > 
> > -	ret = isp_pipeline_pm_power(source->entity, sink_use);
> > -	if (ret < 0)
> > -		return ret;
> > +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
> > +		(flags & MEDIA_LNK_FL_ENABLED)) {
> 
> You could return zero here if the opposite was true, and unindent the
> rest. Up to you --- the patch is fine.
> 
> Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
> 
> > -	ret = isp_pipeline_pm_power(sink->entity, source_use);
> > -	if (ret < 0)
> > -		isp_pipeline_pm_power(source->entity, -sink_use);
> > +		ret = isp_pipeline_pm_power(source, sink_use);
> > +		if (ret < 0)
> > +			return ret;
> > 
> > -	return ret;
> > +		ret = isp_pipeline_pm_power(sink, source_use);
> > +		if (ret < 0)
> > +			isp_pipeline_pm_power(source, -sink_use);
> > +
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > 
> >   }
> >   
> >   /*
> >   -----------------------------------------------------------------------
> >   ------
-- 
Regards,

Laurent Pinchart

Re: [REVIEW PATCH v3 1/2] media: Change media device link_notify behaviour

[Sylwester Nawrocki]

Hi Laurent,

On 06/10/2013 11:46 AM, Laurent Pinchart wrote:
> Hi Sylwester,
> 
> Should I take the series in my tree, or would you like to push it yourself to 
> avoid conflicts with other Exynos patches ?

My plan was to handle this series together with the other Exynos patches
it depends on. I would send a pull request today. I guess there would be
the least conflicts this way. Sorry about embedding this core patch in my
series.

Regards,
Sylwester

Re: [REVIEW PATCH v3 1/2] media: Change media device link_notify behaviour

[Laurent Pinchart]

On Monday 10 June 2013 12:20:15 Sylwester Nawrocki wrote:
> On 06/10/2013 11:46 AM, Laurent Pinchart wrote:
> > Hi Sylwester,
> > 
> > Should I take the series in my tree, or would you like to push it yourself
> > to avoid conflicts with other Exynos patches ?
> 
> My plan was to handle this series together with the other Exynos patches
> it depends on. I would send a pull request today. I guess there would be
> the least conflicts this way. Sorry about embedding this core patch in my
> series.

No worries, I'm fine with that. That's one less pull request for me to handle 
:-)

-- 
Regards,

Laurent Pinchart

Re: [REVIEW PATCH v3 1/2] media: Change media device link_notify behaviour

[Sylwester Nawrocki]

Hi Sakari,

On 06/10/2013 12:10 AM, Sakari Ailus wrote:
> Sylwester Nawrocki wrote:
> ...
>> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
>> index 1d7dbd5..1a2d25c 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -792,9 +792,9 @@ int omap3isp_pipeline_pm_use(struct media_entity *entity, int use)
>>
>>   /*
>>    * isp_pipeline_link_notify - Link management notification callback
>> - * @source: Pad at the start of the link
>> - * @sink: Pad at the end of the link
>> + * @link: The link
>>    * @flags: New link flags that will be applied
>> + * @notification: The link's state change notification type (MEDIA_DEV_NOTIFY_*)
>>    *
>>    * React to link management on powered pipelines by updating the use count of
>>    * all entities in the source and sink sides of the link. Entities are powered
>> @@ -804,29 +804,38 @@ int omap3isp_pipeline_pm_use(struct media_entity *entity, int use)
>>    * off is assumed to never fail. This function will not fail for disconnection
>>    * events.
>>    */
>> -static int isp_pipeline_link_notify(struct media_pad *source,
>> -				    struct media_pad *sink, u32 flags)
>> +static int isp_pipeline_link_notify(struct media_link *link, u32 flags,
>> +				    unsigned int notification)
>>   {
>> -	int source_use = isp_pipeline_pm_use_count(source->entity);
>> -	int sink_use = isp_pipeline_pm_use_count(sink->entity);
>> +	struct media_entity *source = link->source->entity;
>> +	struct media_entity *sink = link->sink->entity;
>> +	int source_use = isp_pipeline_pm_use_count(source);
>> +	int sink_use = isp_pipeline_pm_use_count(sink);
>>   	int ret;
>>
>> -	if (!(flags & MEDIA_LNK_FL_ENABLED)) {
>> +	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
>> +	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
>>   		/* Powering off entities is assumed to never fail. */
>> -		isp_pipeline_pm_power(source->entity, -sink_use);
>> -		isp_pipeline_pm_power(sink->entity, -source_use);
>> +		isp_pipeline_pm_power(source, -sink_use);
>> +		isp_pipeline_pm_power(sink, -source_use);
>>   		return 0;
>>   	}
>>
>> -	ret = isp_pipeline_pm_power(source->entity, sink_use);
>> -	if (ret < 0)
>> -		return ret;
>> +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
>> +		(flags & MEDIA_LNK_FL_ENABLED)) {
> 
> You could return zero here if the opposite was true, and unindent the 
> rest. Up to you --- the patch is fine.

All right, thanks for the Ack. An updated patch to follow.

> Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
> 
>> -	ret = isp_pipeline_pm_power(sink->entity, source_use);
>> -	if (ret < 0)
>> -		isp_pipeline_pm_power(source->entity, -sink_use);
>> +		ret = isp_pipeline_pm_power(source, sink_use);
>> +		if (ret < 0)
>> +			return ret;
>>
>> -	return ret;
>> +		ret = isp_pipeline_pm_power(sink, source_use);
>> +		if (ret < 0)
>> +			isp_pipeline_pm_power(source, -sink_use);
>> +
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>>   }

Regards,
Sylwester

Re: [REVIEW PATCH v3 1/2] media: Change media device link_notify behaviour

[Laurent Pinchart]

On Monday 10 June 2013 12:47:02 Sylwester Nawrocki wrote:
> On 06/10/2013 12:10 AM, Sakari Ailus wrote:
> > Sylwester Nawrocki wrote:
> > ...
> > 
> >> diff --git a/drivers/media/platform/omap3isp/isp.c
> >> b/drivers/media/platform/omap3isp/isp.c index 1d7dbd5..1a2d25c 100644
> >> --- a/drivers/media/platform/omap3isp/isp.c
> >> +++ b/drivers/media/platform/omap3isp/isp.c
> >> @@ -792,9 +792,9 @@ int omap3isp_pipeline_pm_use(struct media_entity
> >> *entity, int use)>> 
> >>   /*
> >>   
> >>    * isp_pipeline_link_notify - Link management notification callback
> >> 
> >> - * @source: Pad at the start of the link
> >> - * @sink: Pad at the end of the link
> >> + * @link: The link
> >> 
> >>    * @flags: New link flags that will be applied
> >> 
> >> + * @notification: The link's state change notification type
> >> (MEDIA_DEV_NOTIFY_*)>> 
> >>    *
> >>    * React to link management on powered pipelines by updating the use
> >>    count of * all entities in the source and sink sides of the link.
> >>    Entities are powered>> 
> >> @@ -804,29 +804,38 @@ int omap3isp_pipeline_pm_use(struct media_entity
> >> *entity, int use)>> 
> >>    * off is assumed to never fail. This function will not fail for
> >>    disconnection * events.
> >>    */
> >> 
> >> -static int isp_pipeline_link_notify(struct media_pad *source,
> >> -				    struct media_pad *sink, u32 flags)
> >> +static int isp_pipeline_link_notify(struct media_link *link, u32 flags,
> >> +				    unsigned int notification)
> >> 
> >>   {
> >> 
> >> -	int source_use = isp_pipeline_pm_use_count(source->entity);
> >> -	int sink_use = isp_pipeline_pm_use_count(sink->entity);
> >> +	struct media_entity *source = link->source->entity;
> >> +	struct media_entity *sink = link->sink->entity;
> >> +	int source_use = isp_pipeline_pm_use_count(source);
> >> +	int sink_use = isp_pipeline_pm_use_count(sink);
> >> 
> >>   	int ret;
> >> 
> >> -	if (!(flags & MEDIA_LNK_FL_ENABLED)) {
> >> +	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> >> +	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
> >> 
> >>   		/* Powering off entities is assumed to never fail. */
> >> 
> >> -		isp_pipeline_pm_power(source->entity, -sink_use);
> >> -		isp_pipeline_pm_power(sink->entity, -source_use);
> >> +		isp_pipeline_pm_power(source, -sink_use);
> >> +		isp_pipeline_pm_power(sink, -source_use);
> >> 
> >>   		return 0;
> >>   	
> >>   	}
> >> 
> >> -	ret = isp_pipeline_pm_power(source->entity, sink_use);
> >> -	if (ret < 0)
> >> -		return ret;
> >> +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
> >> +		(flags & MEDIA_LNK_FL_ENABLED)) {
> > 
> > You could return zero here if the opposite was true, and unindent the
> > rest. Up to you --- the patch is fine.

I would personally keep the code as-is, to keep the symmetry, but I'm fine 
with both :-)

> All right, thanks for the Ack. An updated patch to follow.
> 
> > Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
> > 
> >> -	ret = isp_pipeline_pm_power(sink->entity, source_use);
> >> -	if (ret < 0)
> >> -		isp_pipeline_pm_power(source->entity, -sink_use);
> >> +		ret = isp_pipeline_pm_power(source, sink_use);
> >> +		if (ret < 0)
> >> +			return ret;
> >> 
> >> -	return ret;
> >> +		ret = isp_pipeline_pm_power(sink, source_use);
> >> +		if (ret < 0)
> >> +			isp_pipeline_pm_power(source, -sink_use);
> >> +
> >> +		return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> 
> >>   }

-- 
Regards,

Laurent Pinchart

Re: [REVIEW PATCH v3 1/2] media: Change media device link_notify behaviour

[Sylwester Nawrocki]

On 06/10/2013 12:49 PM, Laurent Pinchart wrote:
>>>> -static int isp_pipeline_link_notify(struct media_pad *source,
>>>> > >> -				    struct media_pad *sink, u32 flags)
>>>> > >> +static int isp_pipeline_link_notify(struct media_link *link, u32 flags,
>>>> > >> +				    unsigned int notification)
>>>> > >> 
>>>> > >>   {
>>>> > >> 
>>>> > >> -	int source_use = isp_pipeline_pm_use_count(source->entity);
>>>> > >> -	int sink_use = isp_pipeline_pm_use_count(sink->entity);
>>>> > >> +	struct media_entity *source = link->source->entity;
>>>> > >> +	struct media_entity *sink = link->sink->entity;
>>>> > >> +	int source_use = isp_pipeline_pm_use_count(source);
>>>> > >> +	int sink_use = isp_pipeline_pm_use_count(sink);
>>>> > >> 
>>>> > >>   	int ret;
>>>> > >> 
>>>> > >> -	if (!(flags & MEDIA_LNK_FL_ENABLED)) {
>>>> > >> +	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
>>>> > >> +	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
>>>> > >> 
>>>> > >>   		/* Powering off entities is assumed to never fail. */
>>>> > >> 
>>>> > >> -		isp_pipeline_pm_power(source->entity, -sink_use);
>>>> > >> -		isp_pipeline_pm_power(sink->entity, -source_use);
>>>> > >> +		isp_pipeline_pm_power(source, -sink_use);
>>>> > >> +		isp_pipeline_pm_power(sink, -source_use);
>>>> > >> 
>>>> > >>   		return 0;
>>>> > >>   	
>>>> > >>   	}
>>>> > >> 
>>>> > >> -	ret = isp_pipeline_pm_power(source->entity, sink_use);
>>>> > >> -	if (ret < 0)
>>>> > >> -		return ret;
>>>> > >> +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
>>>> > >> +		(flags & MEDIA_LNK_FL_ENABLED)) {
>>> > > 
>>> > > You could return zero here if the opposite was true, and unindent the
>>> > > rest. Up to you --- the patch is fine.
>
> I would personally keep the code as-is, to keep the symmetry, but I'm fine 
> with both :-)

I had also an impression that it looks more symmetric as-is. I would leave it
unchanged then. ;)

Regards,
Sylwester

[REVIEW PATCH v3 2/2] exynos4-is: Extend link_notify handler to support fimc-is/lite pipelines

[Sylwester Nawrocki]

This patch corrects the link_notify handler to support more complex
pipelines, including fimc-lite and fimc-is entities.

After the FIMC-IS driver addition the assumptions made in the link_notify
callback are no longer valid, e.g. the link between fimc-lite subdev and
its video node is not immutable any more and there is more subdevs than
just sensor, MIPI-CSIS and FIMC(-LITE).

The graph is now walked and for each video node found a media pipeline
which ends at this node is disabled/enabled (the subdevs are powered
on/off).

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes since v1:
 - check link->flags instead of the flags argument of link_notify handler
   to see if the pipelines' should be powered back on.
---
 drivers/media/platform/exynos4-is/media-dev.c |  103 +++++++++++++++++++-----
 1 files changed, 81 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index 045a6ae..ec79ebb 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -1287,39 +1287,98 @@ int fimc_md_set_camclk(struct v4l2_subdev *sd, bool on)
 	return __fimc_md_set_camclk(fmd, si, on);
 }
 
-static int fimc_md_link_notify(struct media_link *link, u32 flags,
-						unsigned int notification)
+static int __fimc_md_modify_pipeline(struct media_entity *entity, bool enable)
 {
-	struct media_entity *sink = link->sink->entity;
 	struct exynos_video_entity *ve;
+	struct fimc_pipeline *p;
 	struct video_device *vdev;
-	struct fimc_pipeline *pipeline;
-	int i, ret = 0;
+	int ret;
 
-	if (media_entity_type(sink) != MEDIA_ENT_T_DEVNODE_V4L ||
-	    notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH)
+	vdev = media_entity_to_video_device(entity);
+	if (vdev->entity.use_count == 0)
 		return 0;
 
-	vdev = media_entity_to_video_device(sink);
 	ve = vdev_to_exynos_video_entity(vdev);
-	pipeline = to_fimc_pipeline(ve->pipe);
+	p = to_fimc_pipeline(ve->pipe);
+	/*
+	 * Nothing to do if we are disabling the pipeline, some link
+	 * has been disconnected and p->subdevs array is cleared now.
+	 */
+	if (!enable && p->subdevs[IDX_SENSOR] == NULL)
+		return 0;
 
-	if (!(link->flags & MEDIA_LNK_FL_ENABLED) && pipeline->subdevs[IDX_SENSOR]) {
-		if (sink->use_count > 0)
-			ret = __fimc_pipeline_close(ve->pipe);
+	if (enable)
+		ret = __fimc_pipeline_open(ve->pipe, entity, true);
+	else
+		ret = __fimc_pipeline_close(ve->pipe);
 
-		for (i = 0; i < IDX_MAX; i++)
-			pipeline->subdevs[i] = NULL;
-	} else if (sink->use_count > 0) {
-		/*
-		 * Link activation. Enable power of pipeline elements only if
-		 * the pipeline is already in use, i.e. its video node is open.
-		 * Recreate the controls destroyed during the link deactivation.
-		 */
-		ret = __fimc_pipeline_open(ve->pipe, sink, true);
+	if (ret == 0 && !enable)
+		memset(p->subdevs, 0, sizeof(p->subdevs));
+
+	return ret;
+}
+
+/* Locking: called with entity->parent->graph_mutex mutex held. */
+static int __fimc_md_modify_pipelines(struct media_entity *entity, bool enable)
+{
+	struct media_entity *entity_err = entity;
+	struct media_entity_graph graph;
+	int ret;
+
+	/*
+	 * Walk current graph and call the pipeline open/close routine for each
+	 * opened video node that belongs to the graph of entities connected
+	 * through active links. This is needed as we cannot power on/off the
+	 * subdevs in random order.
+	 */
+	media_entity_graph_walk_start(&graph, entity);
+
+	while ((entity = media_entity_graph_walk_next(&graph))) {
+		if (media_entity_type(entity) != MEDIA_ENT_T_DEVNODE)
+			continue;
+
+		ret  = __fimc_md_modify_pipeline(entity, enable);
+
+		if (ret < 0)
+			goto err;
+	}
+
+	return 0;
+ err:
+	media_entity_graph_walk_start(&graph, entity_err);
+
+	while ((entity_err = media_entity_graph_walk_next(&graph))) {
+		if (media_entity_type(entity_err) != MEDIA_ENT_T_DEVNODE)
+			continue;
+
+		__fimc_md_modify_pipeline(entity_err, !enable);
+
+		if (entity_err == entity)
+			break;
+	}
+
+	return ret;
+}
+
+static int fimc_md_link_notify(struct media_link *link, unsigned int flags,
+				unsigned int notification)
+{
+	struct media_entity *sink = link->sink->entity;
+	int ret = 0;
+
+	/* Before link disconnection */
+	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH) {
+		if (!(flags & MEDIA_LNK_FL_ENABLED))
+			ret = __fimc_md_modify_pipelines(sink, false);
+		else
+			; /* TODO: Link state change validation */
+	/* After link activation */
+	} else if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
+		   (link->flags & MEDIA_LNK_FL_ENABLED)) {
+		ret = __fimc_md_modify_pipelines(sink, true);
 	}
 
-	return ret ? -EPIPE : ret;
+	return ret ? -EPIPE : 0;
 }
 
 static ssize_t fimc_md_sysfs_show(struct device *dev,
-- 
1.7.4.1