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

[Hans Verkuil <hverkuil@xs4all.nl>]

On 14/11/2023 10:02, Hans Verkuil wrote:
> 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@toshiba.co.jp>
>> ---
>> 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.

Actually, you don't want to add such a type at all. This is all driver specific,
so support like this belongs in the driver.

A good example of that is V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP in
drivers/media/platform/nxp/dw100/dw100.c: there all the handling is done in
the driver, and it adds init/validate/log/equal ops as well.

Regards,

	Hans

> 
> 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 */
> 
>