| Message ID | 20231012071329.2542003-1-yuji2.ishikawa@toshiba.co.jp (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Toshiba Visconti Video Input Interface driver | expand |
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. 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 */
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 */ > >
On Tue, Nov 14, 2023 at 10:10:50AM +0100, Hans Verkuil wrote: > 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. Actually, I think a better option is to use parameters buffers instead of controls, like other ISP driver do. > > 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. > > > >> }; > >> > >> /* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
Hello Hans, Thank you for the review > -----Original Message----- > From: Hans Verkuil <hverkuil@xs4all.nl> > Sent: Tuesday, November 14, 2023 6:02 PM > To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) > <yuji2.ishikawa@toshiba.co.jp>; Laurent Pinchart > <laurent.pinchart@ideasonboard.com>; Mauro Carvalho Chehab > <mchehab@kernel.org>; Rob Herring <robh+dt@kernel.org>; Krzysztof > Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > <conor+dt@kernel.org>; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ > ＯＳＴ) <nobuhiro1.iwamatsu@toshiba.co.jp> > Cc: linux-media@vger.kernel.org; devicetree@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v9 3/5] media: platform: visconti: add V4L2 vendor > specific control handlers > > 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. > I'll put changes for the core library into a separate patch. > > 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 understand. > 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. I'll apply pahole tool. > 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 */ Regards, Yuji
Hello Hans, > -----Original Message----- > From: Hans Verkuil <hverkuil@xs4all.nl> > Sent: Tuesday, November 14, 2023 6:11 PM > To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) > <yuji2.ishikawa@toshiba.co.jp>; Laurent Pinchart > <laurent.pinchart@ideasonboard.com>; Mauro Carvalho Chehab > <mchehab@kernel.org>; Rob Herring <robh+dt@kernel.org>; Krzysztof > Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > <conor+dt@kernel.org>; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ > ＯＳＴ) <nobuhiro1.iwamatsu@toshiba.co.jp> > Cc: linux-media@vger.kernel.org; devicetree@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v9 3/5] media: platform: visconti: add V4L2 vendor > specific control handlers > > 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. I checked drivers/media/platform/nxp/dw100/dw100.c and found that V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP handles a 2D array of U32 parameters, which is covered by standard v4l2_ctrl_type_op_xxxx() APIs. I suppose that controls of the VIIF driver, which handle control specific structs, would not be implemented in the same way and something else may be needed. > 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 */ > > > > Regards, Yuji
Hello Laurent, Thank you for your comment. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: Tuesday, November 14, 2023 6:54 PM > To: Hans Verkuil <hverkuil@xs4all.nl> > Cc: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) > <yuji2.ishikawa@toshiba.co.jp>; Mauro Carvalho Chehab > <mchehab@kernel.org>; Rob Herring <robh+dt@kernel.org>; Krzysztof > Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > <conor+dt@kernel.org>; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ > ＯＳＴ) <nobuhiro1.iwamatsu@toshiba.co.jp>; linux-media@vger.kernel.org; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH v9 3/5] media: platform: visconti: add V4L2 vendor > specific control handlers > > On Tue, Nov 14, 2023 at 10:10:50AM +0100, Hans Verkuil wrote: > > 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. > > Actually, I think a better option is to use parameters buffers instead of controls, > like other ISP driver do. > I'll quickly make an experimental impl of parameters buffers of the VIIF driver and check if it works well. I'm still not for sure passing parameter via parameters buffers satisfies the timing constraint of the HW. > > > 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. > > > > > >> }; > > >> > > >> /* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */ > > -- > Regards, > > Laurent Pinchart Regards, Yuji
On 27/11/2023 01:45, yuji2.ishikawa@toshiba.co.jp wrote: > Hello Hans, > >> -----Original Message----- >> From: Hans Verkuil <hverkuil@xs4all.nl> >> Sent: Tuesday, November 14, 2023 6:11 PM >> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) >> <yuji2.ishikawa@toshiba.co.jp>; Laurent Pinchart >> <laurent.pinchart@ideasonboard.com>; Mauro Carvalho Chehab >> <mchehab@kernel.org>; Rob Herring <robh+dt@kernel.org>; Krzysztof >> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley >> <conor+dt@kernel.org>; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ >> ＯＳＴ) <nobuhiro1.iwamatsu@toshiba.co.jp> >> Cc: linux-media@vger.kernel.org; devicetree@vger.kernel.org; >> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v9 3/5] media: platform: visconti: add V4L2 vendor >> specific control handlers >> >> 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. > > I checked drivers/media/platform/nxp/dw100/dw100.c and found that > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP handles a 2D array of U32 parameters, > which is covered by standard v4l2_ctrl_type_op_xxxx() APIs. > > I suppose that controls of the VIIF driver, which handle control specific structs, > would not be implemented in the same way and something else may be needed. The key takeaway here is that you don't want to have to add support for a driver-specific type to the core control framework. It has to remain driver specific. In this case that means you need to provide your own custom type ops in the driver, just like dw100 does. The actual code there will of course be different, since you are dealing with a compound type, not an array. But the principle is the same. Regards, Hans > >> 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 */ >>> >>> > > Regards, > Yuji
On 27/11/2023 01:46, yuji2.ishikawa@toshiba.co.jp wrote: > Hello Laurent, > > Thank you for your comment. > >> -----Original Message----- >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Sent: Tuesday, November 14, 2023 6:54 PM >> To: Hans Verkuil <hverkuil@xs4all.nl> >> Cc: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) >> <yuji2.ishikawa@toshiba.co.jp>; Mauro Carvalho Chehab >> <mchehab@kernel.org>; Rob Herring <robh+dt@kernel.org>; Krzysztof >> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley >> <conor+dt@kernel.org>; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ >> ＯＳＴ) <nobuhiro1.iwamatsu@toshiba.co.jp>; linux-media@vger.kernel.org; >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >> linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v9 3/5] media: platform: visconti: add V4L2 vendor >> specific control handlers >> >> On Tue, Nov 14, 2023 at 10:10:50AM +0100, Hans Verkuil wrote: >>> 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. >> >> Actually, I think a better option is to use parameters buffers instead of controls, >> like other ISP driver do. >> > > I'll quickly make an experimental impl of parameters buffers of the VIIF driver and check if it works well. > I'm still not for sure passing parameter via parameters buffers satisfies the timing constraint of the HW. This is the preferred method over using controls. Note that the 32 bit vs 64 bit requirement still stands, so use pahole to make sure that whatever data you exchange over the parameter buffer is the same for both architectures. 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. >>>> >>>>> }; >>>>> >>>>> /* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */ >> >> -- >> Regards, >> >> Laurent Pinchart > > > Regards, > Yuji
Hello Laurent, > -----Original Message----- > From: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) > Sent: Monday, November 27, 2023 9:47 AM > To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Hans Verkuil > <hverkuil@xs4all.nl> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ＯＳＴ) > <nobuhiro1.iwamatsu@toshiba.co.jp>; linux-media@vger.kernel.org; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org > Subject: RE: [PATCH v9 3/5] media: platform: visconti: add V4L2 vendor > specific control handlers > > Hello Laurent, > > Thank you for your comment. > > > -----Original Message----- > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Sent: Tuesday, November 14, 2023 6:54 PM > > To: Hans Verkuil <hverkuil@xs4all.nl> > > Cc: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) > > <yuji2.ishikawa@toshiba.co.jp>; Mauro Carvalho Chehab > > <mchehab@kernel.org>; Rob Herring <robh+dt@kernel.org>; Krzysztof > > Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > > <conor+dt@kernel.org>; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ > > ＯＳＴ) <nobuhiro1.iwamatsu@toshiba.co.jp>; > linux-media@vger.kernel.org; > > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > linux-kernel@vger.kernel.org > > Subject: Re: [PATCH v9 3/5] media: platform: visconti: add V4L2 vendor > > specific control handlers > > > > On Tue, Nov 14, 2023 at 10:10:50AM +0100, Hans Verkuil wrote: > > > 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. > > > > Actually, I think a better option is to use parameters buffers instead > > of controls, like other ISP driver do. > > > > I'll quickly make an experimental impl of parameters buffers of the VIIF driver > and check if it works well. > I'm still not for sure passing parameter via parameters buffers satisfies the > timing constraint of the HW. I made an experimental implementation of parameters buffers based on the RKISP1 driver. The experimental driver seems working well, but there's a concern. Due to HW spec, there is a minium 2-frame delay before the enqueued parameters are reflected in the resulting image, while most ISPs should reflect it with a minium 1-frame delay. Our in-house AE/AG middleware will be affected by increased latency. Does libcamera have specific requirements for control latency?, or it can handle the problem by properly using parameter interface and statistics interface? ===== The Visconti5's ISP captures register values automatically at VSync, not manually triggered by register operations (as most ISPs do). The extra 1 frame occurs as shown below. [Visconti5 ISP requires 2 frames to reflect] 0: user application issues ioctl(QBUF) of new parameters 1: VSYNC ISR pops parameters and sets them to ISP registers 2: ISP captures register values at VSYNC. ISP reflects. [Most ISPs require 1 frame to reflect] 0: user application issues ioctl(QBUF) of new parameters 1: VSYNC ISR pops parameters, sets them to ISP registers and notify the change. ISP reflects. > > > > 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. > > > > > > > >> }; > > > >> > > > >> /* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */ > > > > -- > > Regards, > > > > Laurent Pinchart > > > Regards, > Yuji Regards, Yuji
This series is the Video Input Interface driver for Toshiba's ARM SoC, Visconti. This provides DT binding documentation, device driver, documentation and MAINTAINER files. A visconti VIIF driver instance exposes 1 media control device file and 3 video device files for a VIIF hardware. Detailed HW/SW are described in documentation directory. The VIIF hardware has CSI2 receiver, image signal processor and DMAC inside. The subdevice for image signal processor provides vendor specific V4L2 controls. The device driver depends on two other drivers under development; clock framework driver and IOMMU driver. Corresponding features will be added later. Best regards, Yuji Changelog v2: - Resend v1 because a patch exceeds size limit. Changelog v3: - Add documentation to describe SW and HW - 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 a patch exceeds size limit - fix dt-bindings document - stop specifying ID numbers for driver instance explicitly at device tree - use pm_runtime to trigger initialization of HW along with open/close of device files. - add a entry for a header file at MAINTAINERS file Changelog v5: - Fix coding style problem in viif.c (patch 2/6) Changelog v6: - add register definition of BUS-IF and MPU in dt-bindings - add CSI2RX subdevice (separeted from ISP subdevice) - change directory layout (moved to media/platform/toshiba/visconti) - change source file layout (removed hwd_xxxx.c) - pointer to userland memory is removed from uAPI parameters - change register access (from struct style to macro style) - remove unused macros Changelog v7: - remove redundant "bindings" from header and description text - fix multiline text of "description" - change "compatible" to "visconti5-viif" - explicitly define allowed properties for port::endpoint - remove unused variables - update kerneldoc comments - update references to headers Changelog v8: - rename bindings description file - remove/simplify items in bindings - update operations around v4l2_async_notifier - use v4l2_async_connection instead of v4l2_async_subdev - use dev_err_probe() - better error handling at probe - remove redundant mutex - add V4L2_CTRL_TYPE_VISCONTI_ISP constant Changelog v9: - dictionary ordering of dt-bindings properties - applied sparce checker - call div64_u64 for 64bit division - rebase to media_staging tree - fix warning for cast between ptr and dma_addr_t Yuji Ishikawa (5): dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input Interface media: platform: visconti: Add Toshiba Visconti Video Input Interface driver media: platform: visconti: add V4L2 vendor specific control handlers documentation: media: add documentation for Toshiba Visconti Video Input Interface driver MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface .../media/toshiba,visconti5-viif.yaml | 105 + .../driver-api/media/drivers/index.rst | 1 + .../media/drivers/visconti-viif.rst | 462 +++ .../media/v4l/vidioc-g-ext-ctrls.rst | 4 + .../media/v4l/vidioc-queryctrl.rst | 5 + .../media/videodev2.h.rst.exceptions | 1 + MAINTAINERS | 4 + drivers/media/platform/Kconfig | 1 + drivers/media/platform/Makefile | 1 + drivers/media/platform/toshiba/Kconfig | 6 + drivers/media/platform/toshiba/Makefile | 2 + .../media/platform/toshiba/visconti/Kconfig | 18 + .../media/platform/toshiba/visconti/Makefile | 8 + .../media/platform/toshiba/visconti/viif.c | 650 ++++ .../media/platform/toshiba/visconti/viif.h | 374 ++ .../platform/toshiba/visconti/viif_capture.c | 1483 +++++++ .../platform/toshiba/visconti/viif_capture.h | 22 + .../platform/toshiba/visconti/viif_common.c | 199 + .../platform/toshiba/visconti/viif_common.h | 38 + .../platform/toshiba/visconti/viif_controls.c | 3395 +++++++++++++++++ .../platform/toshiba/visconti/viif_controls.h | 18 + .../platform/toshiba/visconti/viif_csi2rx.c | 695 ++++ .../platform/toshiba/visconti/viif_csi2rx.h | 24 + .../toshiba/visconti/viif_csi2rx_regs.h | 102 + .../platform/toshiba/visconti/viif_isp.c | 1259 ++++++ .../platform/toshiba/visconti/viif_isp.h | 24 + .../platform/toshiba/visconti/viif_regs.h | 716 ++++ drivers/media/v4l2-core/v4l2-ctrls-core.c | 7 +- include/uapi/linux/v4l2-controls.h | 6 + include/uapi/linux/videodev2.h | 2 + include/uapi/linux/visconti_viif.h | 1800 +++++++++ 31 files changed, 11431 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti5-viif.yaml create mode 100644 Documentation/driver-api/media/drivers/visconti-viif.rst create mode 100644 drivers/media/platform/toshiba/Kconfig create mode 100644 drivers/media/platform/toshiba/Makefile create mode 100644 drivers/media/platform/toshiba/visconti/Kconfig create mode 100644 drivers/media/platform/toshiba/visconti/Makefile create mode 100644 drivers/media/platform/toshiba/visconti/viif.c create mode 100644 drivers/media/platform/toshiba/visconti/viif.h create mode 100644 drivers/media/platform/toshiba/visconti/viif_capture.c create mode 100644 drivers/media/platform/toshiba/visconti/viif_capture.h create mode 100644 drivers/media/platform/toshiba/visconti/viif_common.c create mode 100644 drivers/media/platform/toshiba/visconti/viif_common.h create mode 100644 drivers/media/platform/toshiba/visconti/viif_controls.c create mode 100644 drivers/media/platform/toshiba/visconti/viif_controls.h create mode 100644 drivers/media/platform/toshiba/visconti/viif_csi2rx.c create mode 100644 drivers/media/platform/toshiba/visconti/viif_csi2rx.h create mode 100644 drivers/media/platform/toshiba/visconti/viif_csi2rx_regs.h create mode 100644 drivers/media/platform/toshiba/visconti/viif_isp.c create mode 100644 drivers/media/platform/toshiba/visconti/viif_isp.h create mode 100644 drivers/media/platform/toshiba/visconti/viif_regs.h create mode 100644 include/uapi/linux/visconti_viif.h