Date:   Thu, 2 Mar 2017 18:02:57 +0200
From:   Sakari Ailus <sakari.ailus@....fi>
To:     Steve Longerbeam <slongerbeam@...il.com>
Cc:     robh+dt@...nel.org, mark.rutland@....com, shawnguo@...nel.org,
        kernel@...gutronix.de, fabio.estevam@....com,
        linux@...linux.org.uk, mchehab@...nel.org, hverkuil@...all.nl,
        nick@...anahar.org, markus.heiser@...marIT.de,
        p.zabel@...gutronix.de, laurent.pinchart+renesas@...asonboard.com,
        bparrot@...com, geert@...ux-m68k.org, arnd@...db.de,
        sudipm.mukherjee@...il.com, minghsiu.tsai@...iatek.com,
        tiffany.lin@...iatek.com, jean-christophe.trotin@...com,
        horms+renesas@...ge.net.au, niklas.soderlund+renesas@...natech.se,
        robert.jarzmik@...e.fr, songjun.wu@...rochip.com,
        andrew-ct.chen@...iatek.com, gregkh@...uxfoundation.org,
        shuah@...nel.org, sakari.ailus@...ux.intel.com, pavel@....cz,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-media@...r.kernel.org,
        devel@...verdev.osuosl.org,
        Steve Longerbeam <steve_longerbeam@...tor.com>
Subject: Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit
 controls from a pipeline

Hi Steve,

On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote:
> v4l2_pipeline_inherit_controls() will add the v4l2 controls from
> all subdev entities in a pipeline to a given video device.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@...tor.com>
> ---
>  drivers/media/v4l2-core/v4l2-mc.c | 48 +++++++++++++++++++++++++++++++++++++++
>  include/media/v4l2-mc.h           | 25 ++++++++++++++++++++
>  2 files changed, 73 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> index 303980b..09d4d97 100644
> --- a/drivers/media/v4l2-core/v4l2-mc.c
> +++ b/drivers/media/v4l2-core/v4l2-mc.c
> @@ -22,6 +22,7 @@
>  #include <linux/usb.h>
>  #include <media/media-device.h>
>  #include <media/media-entity.h>
> +#include <media/v4l2-ctrls.h>
>  #include <media/v4l2-fh.h>
>  #include <media/v4l2-mc.h>
>  #include <media/v4l2-subdev.h>
> @@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct vb2_queue *q)
>  }
>  EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source);
>  
> +int __v4l2_pipeline_inherit_controls(struct video_device *vfd,
> +				     struct media_entity *start_entity)

I have a few concerns / questions:

- What's the purpose of this patch? Why not to access the sub-device node
  directly?

- This implementation is only workable as long as you do not modify the
  pipeline. Once you disable a link along the pipeline, a device where the
  control was inherited from may no longer be a part of the pipeline.
  Depending on the hardware, it could be a part of another pipeline, in
  which case it certainly must not be accessible through an unrelated video
  node. As the function is added to the framework, I would expect it to
  handle such a case correctly.

- I assume it is the responsibility of the caller of this function to ensure
  the device in question will not be powered off whilst the video node is
  used as another user space interface to such a sub-device. If the driver
  uses the generic PM functions in the same file, this works, but it still
  has to be documented.

> +{
> +	struct media_device *mdev = start_entity->graph_obj.mdev;
> +	struct media_entity *entity;
> +	struct media_graph graph;
> +	struct v4l2_subdev *sd;
> +	int ret;
> +
> +	ret = media_graph_walk_init(&graph, mdev);
> +	if (ret)
> +		return ret;
> +
> +	media_graph_walk_start(&graph, start_entity);
> +
> +	while ((entity = media_graph_walk_next(&graph))) {
> +		if (!is_media_entity_v4l2_subdev(entity))
> +			continue;
> +
> +		sd = media_entity_to_v4l2_subdev(entity);
> +
> +		ret = v4l2_ctrl_add_handler(vfd->ctrl_handler,
> +					    sd->ctrl_handler,
> +					    NULL);
> +		if (ret)
> +			break;
> +	}
> +
> +	media_graph_walk_cleanup(&graph);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(__v4l2_pipeline_inherit_controls);
> +
> +int v4l2_pipeline_inherit_controls(struct video_device *vfd,
> +				   struct media_entity *start_entity)
> +{
> +	struct media_device *mdev = start_entity->graph_obj.mdev;
> +	int ret;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	ret = __v4l2_pipeline_inherit_controls(vfd, start_entity);
> +	mutex_unlock(&mdev->graph_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_pipeline_inherit_controls);
> +
>  /* -----------------------------------------------------------------------------
>   * Pipeline power management
>   *
> diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
> index 2634d9d..9848e77 100644
> --- a/include/media/v4l2-mc.h
> +++ b/include/media/v4l2-mc.h
> @@ -171,6 +171,17 @@ void v4l_disable_media_source(struct video_device *vdev);
>   */
>  int v4l_vb2q_enable_media_source(struct vb2_queue *q);
>  
> +/**
> + * v4l2_pipeline_inherit_controls - Add the v4l2 controls from all
> + *				    subdev entities in a pipeline to
> + *				    the given video device.
> + * @vfd: the video device
> + * @start_entity: Starting entity
> + */
> +int __v4l2_pipeline_inherit_controls(struct video_device *vfd,
> +				     struct media_entity *start_entity);
> +int v4l2_pipeline_inherit_controls(struct video_device *vfd,
> +				   struct media_entity *start_entity);
>  
>  /**
>   * v4l2_pipeline_pm_use - Update the use count of an entity
> @@ -231,6 +242,20 @@ static inline int v4l_vb2q_enable_media_source(struct vb2_queue *q)
>  	return 0;
>  }
>  
> +static inline int __v4l2_pipeline_inherit_controls(
> +	struct video_device *vfd,
> +	struct media_entity *start_entity)
> +{
> +	return 0;
> +}
> +
> +static inline int v4l2_pipeline_inherit_controls(
> +	struct video_device *vfd,
> +	struct media_entity *start_entity)
> +{
> +	return 0;
> +}
> +
>  static inline int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
>  {
>  	return 0;

-- 
Kind regards,

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

Powered by blists - more mailing lists