Re: [PATCH v9 3/5] media: platform: visconti: add V4L2 vendor specific control handlers

[Hans Verkuil]

On 12/10/2023 09:13, Yuji Ishikawa wrote:
> Add support to Image Signal Processors of Visconti's Video Input Interface.
> This patch adds vendor specific compound controls
> to configure the image signal processor.
> 
> Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@xxxxxxxxxxxxx>
> ---
> Changelog v2:
> - Resend v1 because a patch exceeds size limit.
> 
> Changelog v3:
> - Adapted to media control framework
> - Introduced ISP subdevice, capture device
> - Remove private IOCTLs and add vendor specific V4L2 controls
> - Change function name avoiding camelcase and uppercase letters
> 
> Changelog v4:
> - Split patches because the v3 patch exceeds size limit
> - Stop using ID number to identify driver instance:
>   - Use dynamically allocated structure to hold HW specific context,
>     instead of static one.
>   - Call HW layer functions with the context structure instead of ID number
> 
> Changelog v5:
> - no change
> 
> Changelog v6:
> - remove unused macros
> - removed hwd_ and HWD_ prefix
> - update source code documentation
> - Suggestion from Hans Verkuil
>   - pointer to userland memory is removed from uAPI arguments
>     - style of structure is now "nested" instead of "chained by pointer";
>   - use div64_u64 for 64bit division
>   - vendor specific controls support TRY_EXT_CTRLS
>   - add READ_ONLY flag to GET_CALIBRATION_STATUS control and similar ones
>   - human friendry control names for vendor specific controls
>   - add initial value to each vendor specific control
>   - GET_LAST_CAPTURE_STATUS control is updated asyncnously from workqueue
>   - remove EXECUTE_ON_WRITE flag of vendor specific control
>   - uAPI: return value of GET_CALIBRATION_STATUS follows common rules of error codes
>   - applied v4l2-compliance
> - Suggestion from Sakari Ailus
>   - use div64_u64 for 64bit division
>   - update copyright's year
>   - remove redandunt cast
>   - use bool instead of HWD_VIIF_ENABLE/DISABLE
>   - simplify comparison to 0
>   - simplify statements with trigram operator
>   - remove redundant local variables
>   - use general integer types instead of u32/s32
> - Suggestion from Laurent Pinchart
>   - moved VIIF driver to driver/platform/toshiba/visconti
>   - change register access: struct-style to macro-style
>   - remove unused type definitions
>   - define enums instead of successive macro constants
>   - remove redundant parenthesis of macro constant
>   - embed struct hwd_res into struct viif_device
>   - use xxx_dma instead of xxx_paddr for variable names of IOVA
>   - literal value: just 0 instead of 0x0
>   - use literal 1 or 0 instead of HWD_VIIF_ENABLE, DISABLE for register access
>   - use true or false instead of HWD_VIIF_ENABLE, DISABLE for function calls
>   - uAPI: return value of GET_CALIBRATION_STATUS follows common rules of error codes
> 
> Changelog v7:
> - remove unused variables
> - split long statements which have multiple logical-OR and trigram operators
> 
> Changelog v8:
> - define constant V4L2_CTRL_TYPE_VISCONTI_ISP for datatype
>   of Visconti specific controls
> - Suggestion from Hans Verkuil
>   - remove pr_info()
>   - use pm_runtime_get_if_in_use() to get power status
> 
> Changelog v9:
> - fix warning for cast between ptr and dma_addr_t
> 
>  .../media/platform/toshiba/visconti/Makefile  |    2 +-
>  .../media/platform/toshiba/visconti/viif.c    |   10 +-
>  .../platform/toshiba/visconti/viif_controls.c | 3395 +++++++++++++++++
>  .../platform/toshiba/visconti/viif_controls.h |   18 +
>  .../platform/toshiba/visconti/viif_isp.c      |   15 +-
>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |    7 +-
>  include/uapi/linux/videodev2.h                |    2 +
>  7 files changed, 3431 insertions(+), 18 deletions(-)
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_controls.c
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_controls.h
> 

<snip>

These core changes below should be in a separate patch, not mixed in with
the driver.

> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index a662fb60f73f..0c4df9fffbe0 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -367,7 +367,9 @@ void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
>  	case V4L2_CTRL_TYPE_AV1_FILM_GRAIN:
>  		pr_cont("AV1_FILM_GRAIN");
>  		break;
> -
> +	case V4L2_CTRL_TYPE_VISCONTI_ISP:
> +		pr_cont("VISCONTI_ISP");
> +		break;
>  	default:
>  		pr_cont("unknown type %d", ctrl->type);
>  		break;
> @@ -1163,6 +1165,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  	case V4L2_CTRL_TYPE_AV1_FILM_GRAIN:
>  		return validate_av1_film_grain(p);
>  
> +	case V4L2_CTRL_TYPE_VISCONTI_ISP:
> +		break;
> +
>  	case V4L2_CTRL_TYPE_AREA:
>  		area = p;
>  		if (!area->width || !area->height)
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index c3d4e490ce7c..bbc3cd3efa65 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1915,6 +1915,8 @@ enum v4l2_ctrl_type {
>  	V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY = 0x281,
>  	V4L2_CTRL_TYPE_AV1_FRAME	    = 0x282,
>  	V4L2_CTRL_TYPE_AV1_FILM_GRAIN	    = 0x283,
> +
> +	V4L2_CTRL_TYPE_VISCONTI_ISP = 0x290,

I see you are using the same V4L2_CTRL_TYPE_VISCONTI_ISP for all the compound
controls. But that's not allowed: the V4L2_CTRL_TYPE_ defines determine the
control type, so each struct used by a control needs its own type.

I also noticed looking through include/uapi/linux/visconti_viif.h that some
of the struct have holes. I really want to avoid holes in structs used by
controls, it is bad practice.

The pahole utility is very useful for testing this. It is also highly
recommended to check for both 32 and 64 bit compilation: the struct layout
must be the same, otherwise you would run into problems if a 32 bit application
is used with a 64 bit kernel.

Finally, Laurent and/or Sakari will also take a look at this driver, for some
reason this driver has been mostly reviewed by me, but I am not really the
expert on ISPs.

Regards,

	Hans

>  };
>  
>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */