Re: [PATCH 8/8] vivid: add pixel_array test control

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

Hi Hans,

Thank you for the patch.

On Tue, Jun 28, 2022 at 02:05:23PM +0200, Hans Verkuil wrote:
> This control will change dimensions according to the source resolution.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/test-drivers/vivid/vivid-core.h    |  1 +
>  drivers/media/test-drivers/vivid/vivid-ctrls.c   | 14 ++++++++++++++
>  drivers/media/test-drivers/vivid/vivid-vid-cap.c |  4 ++++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/drivers/media/test-drivers/vivid/vivid-core.h b/drivers/media/test-drivers/vivid/vivid-core.h
> index 176b72cb143b..e7b23ebc705e 100644
> --- a/drivers/media/test-drivers/vivid/vivid-core.h
> +++ b/drivers/media/test-drivers/vivid/vivid-core.h
> @@ -227,6 +227,7 @@ struct vivid_dev {
>  	struct v4l2_ctrl		*bitmask;
>  	struct v4l2_ctrl		*int_menu;
>  	struct v4l2_ctrl		*ro_int32;
> +	struct v4l2_ctrl		*pixel_array;
>  	struct v4l2_ctrl		*test_pattern;
>  	struct v4l2_ctrl		*colorspace;
>  	struct v4l2_ctrl		*rgb_range_cap;
> diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
> index a78d676575bc..f98a651842ce 100644
> --- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
> +++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
> @@ -35,6 +35,7 @@
>  #define VIVID_CID_AREA			(VIVID_CID_CUSTOM_BASE + 11)
>  #define VIVID_CID_RO_INTEGER		(VIVID_CID_CUSTOM_BASE + 12)
>  #define VIVID_CID_U32_DYN_ARRAY		(VIVID_CID_CUSTOM_BASE + 13)
> +#define VIVID_CID_U8_PIXEL_ARRAY	(VIVID_CID_CUSTOM_BASE + 14)
>  
>  #define VIVID_CID_VIVID_BASE		(0x00f00000 | 0xf000)
>  #define VIVID_CID_VIVID_CLASS		(0x00f00000 | 1)
> @@ -228,6 +229,18 @@ static const struct v4l2_ctrl_config vivid_ctrl_u8_4d_array = {
>  	.dims = { 2, 3, 4, 5 },
>  };
>  
> +static const struct v4l2_ctrl_config vivid_ctrl_u8_pixel_array = {
> +	.ops = &vivid_user_gen_ctrl_ops,
> +	.id = VIVID_CID_U8_PIXEL_ARRAY,
> +	.name = "U8 Pixel Array",
> +	.type = V4L2_CTRL_TYPE_U8,
> +	.def = 0x80,
> +	.min = 0x00,
> +	.max = 0xff,
> +	.step = 1,
> +	.dims = { 640, 360 },
> +};
> +
>  static const char * const vivid_ctrl_menu_strings[] = {
>  	"Menu Item 0 (Skipped)",
>  	"Menu Item 1",
> @@ -1642,6 +1655,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL);
>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u16_matrix, NULL);
>  	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_4d_array, NULL);
> +	dev->pixel_array = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_pixel_array, NULL);
>  
>  	if (dev->has_vid_cap) {
>  		/* Image Processing Controls */
> diff --git a/drivers/media/test-drivers/vivid/vivid-vid-cap.c b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
> index b9caa4b26209..6dc4091fcabb 100644
> --- a/drivers/media/test-drivers/vivid/vivid-vid-cap.c
> +++ b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
> @@ -381,6 +381,7 @@ static enum tpg_pixel_aspect vivid_get_pixel_aspect(const struct vivid_dev *dev)
>  void vivid_update_format_cap(struct vivid_dev *dev, bool keep_controls)
>  {
>  	struct v4l2_bt_timings *bt = &dev->dv_timings_cap[dev->input].bt;
> +	u32 dims[V4L2_CTRL_MAX_DIMS] = {};
>  	unsigned size;
>  	u64 pixelclock;
>  
> @@ -459,6 +460,9 @@ void vivid_update_format_cap(struct vivid_dev *dev, bool keep_controls)
>  	tpg_s_video_aspect(&dev->tpg, vivid_get_video_aspect(dev));
>  	tpg_s_pixel_aspect(&dev->tpg, vivid_get_pixel_aspect(dev));
>  	tpg_update_mv_step(&dev->tpg);
> +	dims[0] = dev->src_rect.width;
> +	dims[1] = dev->src_rect.height;
> +	v4l2_ctrl_modify_dimensions(dev->pixel_array, dims);

The implementation looks fine, but calling the init function (twice) on
each element will make vivid_update_format_cap() pretty slow. How about
going for a downsampled resolution here ?

Also, this made me wonder if v4l2_ctrl_modify_dimensions() should return
without doing anything if the new dimensions are identical to the
current ones.

>  }
>  
>  /* Map the field to something that is valid for the current input */

-- 
Regards,

Laurent Pinchart