Re: [RFC PATCH 6/8] v4l2-subdev: add v4l2_subdev_create_pad_configs

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

Thank you for the patch.

On Thursday 04 December 2014 10:54:57 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> When a new subdevice is registered this new function is called to allocate
> and initialize a pad_config array. This allows bridge drivers to use the
> pad ops that require such an array as argument.

While I agree that a v4l2_subdev_create_pad_configs() function is needed, I 
don't think the pad configs should be stored in the subdev structure. This 
would defeat the whole point of having a try mechanism based on private 
configs. Bridge drivers should instead create the configs on demand, use them, 
and delete them afterwards.

> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/v4l2-device.c | 11 ++++++++++-
>  drivers/media/v4l2-core/v4l2-subdev.c | 36 ++++++++++++++++++++++++++++++++
>  include/media/v4l2-subdev.h           |  4 ++++
>  3 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-device.c
> b/drivers/media/v4l2-core/v4l2-device.c index 015f92a..5c8d402 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -183,12 +183,16 @@ int v4l2_device_register_subdev(struct v4l2_device
> *v4l2_dev, if (err)
>  		goto error_unregister;
> 
> +	err = v4l2_subdev_create_pad_configs(sd);
> +	if (err)
> +		goto error_unregister;
> +

I don't thin

>  #if defined(CONFIG_MEDIA_CONTROLLER)
>  	/* Register the entity. */
>  	if (v4l2_dev->mdev) {
>  		err = media_device_register_entity(v4l2_dev->mdev, entity);
>  		if (err < 0)
> -			goto error_unregister;
> +			goto error_free_pad_configs;
>  	}
>  #endif
> 
> @@ -198,6 +202,9 @@ int v4l2_device_register_subdev(struct v4l2_device
> *v4l2_dev,
> 
>  	return 0;
> 
> +error_free_pad_configs:
> +	kfree(sd->pad_configs);
> +	sd->pad_configs = NULL;
>  error_unregister:
>  	if (sd->internal_ops && sd->internal_ops->unregistered)
>  		sd->internal_ops->unregistered(sd);
> @@ -282,6 +289,8 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev
> *sd)
> 
>  	if (sd->internal_ops && sd->internal_ops->unregistered)
>  		sd->internal_ops->unregistered(sd);
> +	kfree(sd->pad_configs);
> +	sd->pad_configs = NULL;
>  	sd->v4l2_dev = NULL;
> 
>  #if defined(CONFIG_MEDIA_CONTROLLER)
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
> b/drivers/media/v4l2-core/v4l2-subdev.c index 3c8b198..a7b874e 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -560,6 +560,41 @@ int v4l2_subdev_link_validate(struct media_link *link)
>  EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
>  #endif /* CONFIG_MEDIA_CONTROLLER */
> 
> +int v4l2_subdev_create_pad_configs(struct v4l2_subdev *sd)
> +{
> +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> +	struct v4l2_subdev_format fmt = {
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +	};
> +	unsigned pads = sd->entity.num_pads;
> +	unsigned pad;
> +	int err;
> +
> +	if (pads == 0)
> +		return 0;
> +	sd->pad_configs = kcalloc(pads, sizeof(*sd->pad_configs), GFP_KERNEL);
> +	if (sd->pad_configs == NULL)
> +		return -ENOMEM;
> +	for (pad = 0; pad < pads; pad++) {
> +		fmt.pad = pad;
> +		err = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
> +
> +		if (err && err != -ENOIOCTLCMD) {
> +			kfree(sd->pad_configs);
> +			sd->pad_configs = NULL;
> +			return err;
> +		}
> +		sd->pad_configs[pad].try_fmt = fmt.format;
> +		sd->pad_configs[pad].try_crop.width = fmt.format.width;
> +		sd->pad_configs[pad].try_crop.height = fmt.format.height;
> +		sd->pad_configs[pad].try_compose.width = fmt.format.width;
> +		sd->pad_configs[pad].try_compose.height = fmt.format.height;
> +	}
> +#endif
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_create_pad_configs);
> +
>  void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops
> *ops) {
>  	INIT_LIST_HEAD(&sd->list);
> @@ -571,6 +606,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const
> struct v4l2_subdev_ops *ops) sd->grp_id = 0;
>  	sd->dev_priv = NULL;
>  	sd->host_priv = NULL;
> +	sd->pad_configs = NULL;
>  #if defined(CONFIG_MEDIA_CONTROLLER)
>  	sd->entity.name = sd->name;
>  	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV;
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 175dc4f..e9e8b15 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -611,6 +611,8 @@ struct v4l2_subdev {
>  	char name[V4L2_SUBDEV_NAME_SIZE];
>  	/* can be used to group similar subdevs, value is driver-specific */
>  	u32 grp_id;
> +	/* Used by bridge drivers to allocate a pad config array */
> +	struct v4l2_subdev_pad_config *pad_configs;
>  	/* pointer to private data */
>  	void *dev_priv;
>  	void *host_priv;
> @@ -689,6 +691,8 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev
> *sd, struct v4l2_subdev_format *sink_fmt);
>  int v4l2_subdev_link_validate(struct media_link *link);
>  #endif /* CONFIG_MEDIA_CONTROLLER */
> +
> +int v4l2_subdev_create_pad_configs(struct v4l2_subdev *sd);
>  void v4l2_subdev_init(struct v4l2_subdev *sd,
>  		      const struct v4l2_subdev_ops *ops);

-- 
Regards,

Laurent Pinchart