| Message ID | 20220503095127.48710-1-eugen.hristev@microchip.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: atmel: atmel-isc: implement media controller | expand |
On 5/3/22 12:51 PM, Eugen Hristev wrote: > This series is a split from the series : > [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller > and it includes the media controller part. > previous fixes were sent on a different patch series. > > As discussed on the ML, moving forward with having the media link validate at > start/stop streaming call. > I will test the patch : > [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops > afterwards, but that patch requires moving my logic to the new vb2 callbacks. > > Full series history: > > Changes in v10: > -> split the series into this first fixes part. > -> moved IO_MC addition from first patch to the second patch on the driver changes > -> edited commit messages > -> DT nodes now disabled by default. > > Changes in v9: > -> kernel robot reported isc_link_validate is not static, changed to static. > > Changes in v8: > -> scaler: modified crop bounds to have the exact source size > > Changes in v7: > -> scaler: modified crop bounds to have maximum isc size > -> format propagation: did small changes as per Jacopo review > > > Changes in v6: > -> worked a bit on scaler, added try crop and other changes as per Jacopo review > -> worked on isc-base enum_fmt , reworked as per Jacopo review > > Changes in v5: > -> removed patch that removed the 'stop' variable as it was still required > -> added two new trivial patches > -> reworked some parts of the scaler and format propagation after discussions with Jacopo > > > Changes in v4: > -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked > one patch that was using it > -> as reviewed by Jacopo, reworked some parts of the media controller implementation > > > Changes in v3: > - change in bindings, small fixes in csi2dc driver and conversion to mc > for the isc-base. > - removed some MAINTAINERS patches and used patterns in MAINTAINERS > > Changes in v2: > - integrated many changes suggested by Jacopo in the review of the v1 series. > - add a few new patches > > Eugen Hristev (5): > media: atmel: atmel-isc: prepare for media controller support > media: atmel: atmel-isc: implement media controller > ARM: dts: at91: sama7g5: add nodes for video capture > ARM: configs: at91: sama7: add xisc and csi2dc > ARM: multi_v7_defconfig: add atmel video pipeline modules > > arch/arm/boot/dts/sama7g5.dtsi | 51 ++ > arch/arm/configs/multi_v7_defconfig | 3 + > arch/arm/configs/sama7_defconfig | 2 + > drivers/media/platform/atmel/Makefile | 2 +- > drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- > .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ > drivers/media/platform/atmel/atmel-isc.h | 50 +- > .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- > .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- > 9 files changed, 685 insertions(+), 241 deletions(-) > create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c > Hello Hans, What do you think about this series, does it require more work or changes until it could move further ? Anything in particular you would like me to try or test out ? Thanks, Eugen
Hi Eugen, On 6/15/22 13:06, Eugen.Hristev@microchip.com wrote: > On 5/3/22 12:51 PM, Eugen Hristev wrote: >> This series is a split from the series : >> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller >> and it includes the media controller part. >> previous fixes were sent on a different patch series. >> >> As discussed on the ML, moving forward with having the media link validate at >> start/stop streaming call. >> I will test the patch : >> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops >> afterwards, but that patch requires moving my logic to the new vb2 callbacks. >> >> Full series history: >> >> Changes in v10: >> -> split the series into this first fixes part. >> -> moved IO_MC addition from first patch to the second patch on the driver changes >> -> edited commit messages >> -> DT nodes now disabled by default. >> >> Changes in v9: >> -> kernel robot reported isc_link_validate is not static, changed to static. >> >> Changes in v8: >> -> scaler: modified crop bounds to have the exact source size >> >> Changes in v7: >> -> scaler: modified crop bounds to have maximum isc size >> -> format propagation: did small changes as per Jacopo review >> >> >> Changes in v6: >> -> worked a bit on scaler, added try crop and other changes as per Jacopo review >> -> worked on isc-base enum_fmt , reworked as per Jacopo review >> >> Changes in v5: >> -> removed patch that removed the 'stop' variable as it was still required >> -> added two new trivial patches >> -> reworked some parts of the scaler and format propagation after discussions with Jacopo >> >> >> Changes in v4: >> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked >> one patch that was using it >> -> as reviewed by Jacopo, reworked some parts of the media controller implementation >> >> >> Changes in v3: >> - change in bindings, small fixes in csi2dc driver and conversion to mc >> for the isc-base. >> - removed some MAINTAINERS patches and used patterns in MAINTAINERS >> >> Changes in v2: >> - integrated many changes suggested by Jacopo in the review of the v1 series. >> - add a few new patches >> >> Eugen Hristev (5): >> media: atmel: atmel-isc: prepare for media controller support >> media: atmel: atmel-isc: implement media controller >> ARM: dts: at91: sama7g5: add nodes for video capture >> ARM: configs: at91: sama7: add xisc and csi2dc >> ARM: multi_v7_defconfig: add atmel video pipeline modules >> >> arch/arm/boot/dts/sama7g5.dtsi | 51 ++ >> arch/arm/configs/multi_v7_defconfig | 3 + >> arch/arm/configs/sama7_defconfig | 2 + >> drivers/media/platform/atmel/Makefile | 2 +- >> drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- >> .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ >> drivers/media/platform/atmel/atmel-isc.h | 50 +- >> .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- >> .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- >> 9 files changed, 685 insertions(+), 241 deletions(-) >> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c >> > > > Hello Hans, > > What do you think about this series, does it require more work or > changes until it could move further ? Anything in particular you would > like me to try or test out ? It's high on my todo list to look at this and see if anything else needs to be done. I hope to get to this this week. Regards, Hans > > Thanks, > Eugen
Hi Eugen, On 03/05/2022 11:51, Eugen Hristev wrote: > This series is a split from the series : > [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller > and it includes the media controller part. > previous fixes were sent on a different patch series. > > As discussed on the ML, moving forward with having the media link validate at > start/stop streaming call. > I will test the patch : > [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops > afterwards, but that patch requires moving my logic to the new vb2 callbacks. I'm looking at merging this series, but I would like to have the output of 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all correct. And one more question which may have been answered already in the past: Changing to the MC will break existing applications, doesn't it? Or did I miss something? Regards, Hans > > Full series history: > > Changes in v10: > -> split the series into this first fixes part. > -> moved IO_MC addition from first patch to the second patch on the driver changes > -> edited commit messages > -> DT nodes now disabled by default. > > Changes in v9: > -> kernel robot reported isc_link_validate is not static, changed to static. > > Changes in v8: > -> scaler: modified crop bounds to have the exact source size > > Changes in v7: > -> scaler: modified crop bounds to have maximum isc size > -> format propagation: did small changes as per Jacopo review > > > Changes in v6: > -> worked a bit on scaler, added try crop and other changes as per Jacopo review > -> worked on isc-base enum_fmt , reworked as per Jacopo review > > Changes in v5: > -> removed patch that removed the 'stop' variable as it was still required > -> added two new trivial patches > -> reworked some parts of the scaler and format propagation after discussions with Jacopo > > > Changes in v4: > -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked > one patch that was using it > -> as reviewed by Jacopo, reworked some parts of the media controller implementation > > > Changes in v3: > - change in bindings, small fixes in csi2dc driver and conversion to mc > for the isc-base. > - removed some MAINTAINERS patches and used patterns in MAINTAINERS > > Changes in v2: > - integrated many changes suggested by Jacopo in the review of the v1 series. > - add a few new patches > > Eugen Hristev (5): > media: atmel: atmel-isc: prepare for media controller support > media: atmel: atmel-isc: implement media controller > ARM: dts: at91: sama7g5: add nodes for video capture > ARM: configs: at91: sama7: add xisc and csi2dc > ARM: multi_v7_defconfig: add atmel video pipeline modules > > arch/arm/boot/dts/sama7g5.dtsi | 51 ++ > arch/arm/configs/multi_v7_defconfig | 3 + > arch/arm/configs/sama7_defconfig | 2 + > drivers/media/platform/atmel/Makefile | 2 +- > drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- > .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ > drivers/media/platform/atmel/atmel-isc.h | 50 +- > .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- > .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- > 9 files changed, 685 insertions(+), 241 deletions(-) > create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c >
On 6/22/22 2:53 PM, Hans Verkuil wrote: > Hi Eugen, > > On 03/05/2022 11:51, Eugen Hristev wrote: >> This series is a split from the series : >> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller >> and it includes the media controller part. >> previous fixes were sent on a different patch series. >> >> As discussed on the ML, moving forward with having the media link validate at >> start/stop streaming call. >> I will test the patch : >> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops >> afterwards, but that patch requires moving my logic to the new vb2 callbacks. > > I'm looking at merging this series, but I would like to have the output of > 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all > correct. Hello Hans, Please have a look at attached file . Unless you want me to add the whole output to the e-mail ? I also added output of media-ctl -p for your convenience. the subdev2 is a device and driver that is not upstream and has some compliance issues, they are reported by the v4l2-compliance tool, but they should not affect this series, it's a synopsys driver that was rejected on mainline a few years ago, I took it for internal usage, but it's not cleaned up nor worked a lot upon. > > And one more question which may have been answered already in the past: > > Changing to the MC will break existing applications, doesn't it? Or did I > miss something? > The existing applications will have to configure the pipeline now. It will no longer work by configuring just the top video node /dev/video0 . They would have to use media-ctl for it, something similar with this set of commands: media-ctl -d /dev/media0 --set-v4l2 '"imx219 1-0010":0[fmt:SRGGB10_1X10/1920x1080]' media-ctl -d /dev/media0 --set-v4l2 '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]' media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]' media-ctl -d /dev/media0 --set-v4l2 '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]' Thank you for taking care of this ! Eugen > Regards, > > Hans > >> >> Full series history: >> >> Changes in v10: >> -> split the series into this first fixes part. >> -> moved IO_MC addition from first patch to the second patch on the driver changes >> -> edited commit messages >> -> DT nodes now disabled by default. >> >> Changes in v9: >> -> kernel robot reported isc_link_validate is not static, changed to static. >> >> Changes in v8: >> -> scaler: modified crop bounds to have the exact source size >> >> Changes in v7: >> -> scaler: modified crop bounds to have maximum isc size >> -> format propagation: did small changes as per Jacopo review >> >> >> Changes in v6: >> -> worked a bit on scaler, added try crop and other changes as per Jacopo review >> -> worked on isc-base enum_fmt , reworked as per Jacopo review >> >> Changes in v5: >> -> removed patch that removed the 'stop' variable as it was still required >> -> added two new trivial patches >> -> reworked some parts of the scaler and format propagation after discussions with Jacopo >> >> >> Changes in v4: >> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked >> one patch that was using it >> -> as reviewed by Jacopo, reworked some parts of the media controller implementation >> >> >> Changes in v3: >> - change in bindings, small fixes in csi2dc driver and conversion to mc >> for the isc-base. >> - removed some MAINTAINERS patches and used patterns in MAINTAINERS >> >> Changes in v2: >> - integrated many changes suggested by Jacopo in the review of the v1 series. >> - add a few new patches >> >> Eugen Hristev (5): >> media: atmel: atmel-isc: prepare for media controller support >> media: atmel: atmel-isc: implement media controller >> ARM: dts: at91: sama7g5: add nodes for video capture >> ARM: configs: at91: sama7: add xisc and csi2dc >> ARM: multi_v7_defconfig: add atmel video pipeline modules >> >> arch/arm/boot/dts/sama7g5.dtsi | 51 ++ >> arch/arm/configs/multi_v7_defconfig | 3 + >> arch/arm/configs/sama7_defconfig | 2 + >> drivers/media/platform/atmel/Makefile | 2 +- >> drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- >> .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ >> drivers/media/platform/atmel/atmel-isc.h | 50 +- >> .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- >> .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- >> 9 files changed, 685 insertions(+), 241 deletions(-) >> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c >> > # media-ctl -p Media controller API version 5.15.32 Media device information ------------------------ driver atmel_isc_commo model microchip,sama7g5-isc serial bus info platform:microchip-sama7g5-xisc hw revision 0x220 driver version 5.15.32 Device topology - entity 1: atmel_isc_scaler (2 pads, 2 links) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev0 pad0: Sink [fmt:SBGGR8_1X8/3264x2464 field:none colorspace:srgb crop.bounds:(0,0)/3264x2464 crop:(0,0)/3264x2464] <- "csi2dc":1 [ENABLED,IMMUTABLE] pad1: Source [fmt:SBGGR8_1X8/3264x2464 field:none colorspace:srgb] -> "atmel_isc_common":0 [ENABLED,IMMUTABLE] - entity 4: csi2dc (2 pads, 2 links) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev1 pad0: Sink [fmt:SRGGB8_1X8/640x480 field:none colorspace:srgb] <- "dw-csi.0":1 [ENABLED] pad1: Source [fmt:SRGGB8_1X8/640x480 field:none colorspace:srgb] -> "atmel_isc_scaler":0 [ENABLED,IMMUTABLE] - entity 7: dw-csi.0 (2 pads, 2 links) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev2 pad0: Sink [fmt:SBGGR8_1X8/0x0] <- "imx219 1-0010":0 [ENABLED] pad1: Source [fmt:SBGGR8_1X8/0x0] -> "csi2dc":0 [ENABLED] - entity 12: imx219 1-0010 (1 pad, 1 link) type V4L2 subdev subtype Sensor flags 0 device node name /dev/v4l-subdev3 pad0: Source [fmt:SRGGB10_1X10/3280x2464 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464] -> "dw-csi.0":0 [ENABLED] - entity 24: atmel_isc_common (1 pad, 1 link) type Node subtype V4L flags 1 device node name /dev/video0 pad0: Sink <- "atmel_isc_scaler":1 [ENABLED,IMMUTABLE] v4l2-compliance 1.22.1, 32 bits, 32-bit time_t Compliance test for atmel_isc_commo device /dev/media0: Media Driver Info: Driver name : atmel_isc_commo Model : microchip,sama7g5-isc Serial : Bus info : platform:microchip-sama7g5-xisc Media version : 5.15.32 Hardware revision: 0x00000220 (544) Driver version : 5.15.32 Required ioctls: test MEDIA_IOC_DEVICE_INFO: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/media0 open: OK test MEDIA_IOC_DEVICE_INFO: OK test for unlimited opens: OK Media Controller ioctls: test MEDIA_IOC_G_TOPOLOGY: OK Entities: 5 Interfaces: 5 Pads: 8 Links: 9 test MEDIA_IOC_ENUM_ENTITIES/LINKS: OK test MEDIA_IOC_SETUP_LINK: OK Total for atmel_isc_commo device /dev/media0: 8, Succeeded: 8, Failed: 0, Warnings: 0 -------------------------------------------------------------------------------- Compliance test for atmel_isc_commo device /dev/v4l-subdev0: Driver Info: Driver version : 5.15.32 Capabilities : 0x00000000 Media Driver Info: Driver name : atmel_isc_commo Model : microchip,sama7g5-isc Serial : Bus info : platform:microchip-sama7g5-xisc Media version : 5.15.32 Hardware revision: 0x00000220 (544) Driver version : 5.15.32 Interface Info: ID : 0x03000010 Type : V4L Sub-Device Entity Info: ID : 0x00000001 (1) Name : atmel_isc_scaler Function : Video Scaler Pad 0x01000002 : 0: Sink Link 0x0200001c: from remote pad 0x1000006 of entity 'csi2dc' (Video Interface Bridge): Data, Enabled, Immutable Pad 0x01000003 : 1: Source Link 0x0200001e: to remote pad 0x1000019 of entity 'atmel_isc_common' (V4L2 I/O): Data, Enabled, Immutable Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_SUDBEV_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/v4l-subdev0 open: OK test VIDIOC_SUBDEV_QUERYCAP: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Sub-Device ioctls (Sink Pad 0): test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK test Try VIDIOC_SUBDEV_G/S_FMT: OK warn: v4l2-test-subdevs.cpp(507): VIDIOC_SUBDEV_G_SELECTION is supported for target 0 but not VIDIOC_SUBDEV_S_SELECTION test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK test Active VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK test Active VIDIOC_SUBDEV_G/S_FMT: OK warn: v4l2-test-subdevs.cpp(507): VIDIOC_SUBDEV_G_SELECTION is supported for target 0 but not VIDIOC_SUBDEV_S_SELECTION test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported) Sub-Device ioctls (Source Pad 1): test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK test Try VIDIOC_SUBDEV_G/S_FMT: OK test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported) test Active VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK test Active VIDIOC_SUBDEV_G/S_FMT: OK test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported) test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported) Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) test VIDIOC_QUERYCTRL: OK (Not Supported) test VIDIOC_G/S_CTRL: OK (Not Supported) test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 0 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported) test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK (Not Supported) test VIDIOC_TRY_FMT: OK (Not Supported) test VIDIOC_S_FMT: OK (Not Supported) test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported) test VIDIOC_EXPBUF: OK (Not Supported) test Requests: OK (Not Supported) test TIME32/64: OK Total for atmel_isc_commo device /dev/v4l-subdev0: 59, Succeeded: 59, Failed: 0, Warnings: 2 -------------------------------------------------------------------------------- Compliance test for device /dev/v4l-subdev1: Driver Info: Driver version : 5.15.32 Capabilities : 0x00000000 Required ioctls: test VIDIOC_SUDBEV_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/v4l-subdev1 open: OK test VIDIOC_SUBDEV_QUERYCAP: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) test VIDIOC_QUERYCTRL: OK (Not Supported) test VIDIOC_G/S_CTRL: OK (Not Supported) test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 0 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported) test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK (Not Supported) test VIDIOC_TRY_FMT: OK (Not Supported) test VIDIOC_S_FMT: OK (Not Supported) test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported) test VIDIOC_EXPBUF: OK (Not Supported) test Requests: OK (Not Supported) test TIME32/64: OK Total for device /dev/v4l-subdev1: 44, Succeeded: 44, Failed: 0, Warnings: 0 -------------------------------------------------------------------------------- Compliance test for atmel_isc_commo device /dev/v4l-subdev2: Driver Info: Driver version : 5.15.32 Capabilities : 0x00000000 Media Driver Info: Driver name : atmel_isc_commo Model : microchip,sama7g5-isc Serial : Bus info : platform:microchip-sama7g5-xisc Media version : 5.15.32 Hardware revision: 0x00000220 (544) Driver version : 5.15.32 Interface Info: ID : 0x03000014 Type : V4L Sub-Device Entity Info: ID : 0x00000007 (7) Name : dw-csi.0 Function : Video Interface Bridge Pad 0x01000008 : 0: Sink Link 0x0200000e: from remote pad 0x100000d of entity 'imx219 1-0010' (Camera Sensor): Data, Enabled Pad 0x01000009 : 1: Source Link 0x0200000a: to remote pad 0x1000005 of entity 'csi2dc' (Video Interface Bridge): Data, Enabled Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_SUDBEV_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/v4l-subdev2 open: OK test VIDIOC_SUBDEV_QUERYCAP: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_LOG_STATUS: OK Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Sub-Device ioctls (Sink Pad 0): test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK fail: v4l2-test-subdevs.cpp(324): fmt.width == 0 || fmt.width > 65536 fail: v4l2-test-subdevs.cpp(369): checkMBusFrameFmt(node, fmt.format) test Try VIDIOC_SUBDEV_G/S_FMT: FAIL test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported) test Active VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK fail: v4l2-test-subdevs.cpp(324): fmt.width == 0 || fmt.width > 65536 fail: v4l2-test-subdevs.cpp(369): checkMBusFrameFmt(node, fmt.format) test Active VIDIOC_SUBDEV_G/S_FMT: FAIL test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported) test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported) Sub-Device ioctls (Source Pad 1): test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK fail: v4l2-test-subdevs.cpp(324): fmt.width == 0 || fmt.width > 65536 fail: v4l2-test-subdevs.cpp(369): checkMBusFrameFmt(node, fmt.format) test Try VIDIOC_SUBDEV_G/S_FMT: FAIL test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported) test Active VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK fail: v4l2-test-subdevs.cpp(324): fmt.width == 0 || fmt.width > 65536 fail: v4l2-test-subdevs.cpp(369): checkMBusFrameFmt(node, fmt.format) test Active VIDIOC_SUBDEV_G/S_FMT: FAIL test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported) test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported) Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) test VIDIOC_QUERYCTRL: OK (Not Supported) test VIDIOC_G/S_CTRL: OK (Not Supported) test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 0 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported) test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK (Not Supported) test VIDIOC_TRY_FMT: OK (Not Supported) test VIDIOC_S_FMT: OK (Not Supported) test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported) test VIDIOC_EXPBUF: OK (Not Supported) test Requests: OK (Not Supported) test TIME32/64: OK Total for atmel_isc_commo device /dev/v4l-subdev2: 59, Succeeded: 55, Failed: 4, Warnings: 0 -------------------------------------------------------------------------------- Compliance test for device /dev/v4l-subdev3: Driver Info: Driver version : 5.15.32 Capabilities : 0x00000000 Required ioctls: test VIDIOC_SUDBEV_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/v4l-subdev3 open: OK test VIDIOC_SUBDEV_QUERYCAP: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 17 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported) test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK (Not Supported) test VIDIOC_TRY_FMT: OK (Not Supported) test VIDIOC_S_FMT: OK (Not Supported) test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported) test VIDIOC_EXPBUF: OK (Not Supported) test Requests: OK (Not Supported) test TIME32/64: OK Total for device /dev/v4l-subdev3: 44, Succeeded: 44, Failed: 0, Warnings: 0 -------------------------------------------------------------------------------- Compliance test for microchip-isc device /dev/video0: Driver Info: Driver name : microchip-isc Card type : Atmel Image Sensor Controller Bus info : platform:microchip-sama7g5-xisc Driver version : 5.15.32 Capabilities : 0xa4200001 Video Capture Streaming Extended Pix Format Device Capabilities Device Caps : 0x24200001 Video Capture Streaming Extended Pix Format Media Driver Info: Driver name : atmel_isc_commo Model : microchip,sama7g5-isc Serial : Bus info : platform:microchip-sama7g5-xisc Media version : 5.15.32 Hardware revision: 0x00000220 (544) Driver version : 5.15.32 Interface Info: ID : 0x0300001a Type : V4L Video Entity Info: ID : 0x00000018 (24) Name : atmel_isc_common Function : V4L2 I/O Flags : default Pad 0x01000019 : 0: Sink Link 0x0200001e: from remote pad 0x1000003 of entity 'atmel_isc_scaler' (Video Scaler): Data, Enabled, Immutable Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/video0 open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls (Input 0): test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 6 Private Controls: 8 Format ioctls (Input 0): test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK Codec ioctls (Input 0): test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls (Input 0): test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK test Requests: OK (Not Supported) test TIME32/64: OK Total for microchip-isc device /dev/video0: 47, Succeeded: 47, Failed: 0, Warnings: 0 Grand Total for atmel_isc_commo device /dev/media0: 261, Succeeded: 257, Failed: 4, Warnings: 2
On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote: > On 6/22/22 2:53 PM, Hans Verkuil wrote: >> Hi Eugen, >> >> On 03/05/2022 11:51, Eugen Hristev wrote: >>> This series is a split from the series : >>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller >>> and it includes the media controller part. >>> previous fixes were sent on a different patch series. >>> >>> As discussed on the ML, moving forward with having the media link validate at >>> start/stop streaming call. >>> I will test the patch : >>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops >>> afterwards, but that patch requires moving my logic to the new vb2 callbacks. >> >> I'm looking at merging this series, but I would like to have the output of >> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all >> correct. > > Hello Hans, > > Please have a look at attached file . Unless you want me to add the > whole output to the e-mail ? > > I also added output of media-ctl -p for your convenience. > the subdev2 is a device and driver that is not upstream and has some > compliance issues, they are reported by the v4l2-compliance tool, but > they should not affect this series, it's a synopsys driver that was > rejected on mainline a few years ago, I took it for internal usage, but > it's not cleaned up nor worked a lot upon. > >> >> And one more question which may have been answered already in the past: >> >> Changing to the MC will break existing applications, doesn't it? Or did I >> miss something? >> > > The existing applications will have to configure the pipeline now. It > will no longer work by configuring just the top video node /dev/video0 . > They would have to use media-ctl for it, something similar with this set > of commands: To add on top of that, actually, the reality is that without the MC support in atmel-isc , some of our platforms do not work at all, because the csi2dc driver which is in the middle of the pipeline, is a MC driver. So it will not work without configuring it with MC anyway. It used to work in a very preliminary version of the csi2dc driver which I sent a few years ago, but that way of handling things was rejected. Hence I changed the csi2dc to being full-MC driver (requested for new drivers) and now I am completing the conversion for the whole pipeline. We are using this MC-centric approach in production for our products to be as close as possible to mainline, and backported it to our 5.15 internal releases, which people are using right now. > > media-ctl -d /dev/media0 --set-v4l2 '"imx219 > 1-0010":0[fmt:SRGGB10_1X10/1920x1080]' > media-ctl -d /dev/media0 --set-v4l2 > '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]' > media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]' > media-ctl -d /dev/media0 --set-v4l2 > '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]' > > Thank you for taking care of this ! > > Eugen > >> Regards, >> >> Hans >> >>> >>> Full series history: >>> >>> Changes in v10: >>> -> split the series into this first fixes part. >>> -> moved IO_MC addition from first patch to the second patch on the driver changes >>> -> edited commit messages >>> -> DT nodes now disabled by default. >>> >>> Changes in v9: >>> -> kernel robot reported isc_link_validate is not static, changed to static. >>> >>> Changes in v8: >>> -> scaler: modified crop bounds to have the exact source size >>> >>> Changes in v7: >>> -> scaler: modified crop bounds to have maximum isc size >>> -> format propagation: did small changes as per Jacopo review >>> >>> >>> Changes in v6: >>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review >>> -> worked on isc-base enum_fmt , reworked as per Jacopo review >>> >>> Changes in v5: >>> -> removed patch that removed the 'stop' variable as it was still required >>> -> added two new trivial patches >>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo >>> >>> >>> Changes in v4: >>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked >>> one patch that was using it >>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation >>> >>> >>> Changes in v3: >>> - change in bindings, small fixes in csi2dc driver and conversion to mc >>> for the isc-base. >>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS >>> >>> Changes in v2: >>> - integrated many changes suggested by Jacopo in the review of the v1 series. >>> - add a few new patches >>> >>> Eugen Hristev (5): >>> media: atmel: atmel-isc: prepare for media controller support >>> media: atmel: atmel-isc: implement media controller >>> ARM: dts: at91: sama7g5: add nodes for video capture >>> ARM: configs: at91: sama7: add xisc and csi2dc >>> ARM: multi_v7_defconfig: add atmel video pipeline modules >>> >>> arch/arm/boot/dts/sama7g5.dtsi | 51 ++ >>> arch/arm/configs/multi_v7_defconfig | 3 + >>> arch/arm/configs/sama7_defconfig | 2 + >>> drivers/media/platform/atmel/Makefile | 2 +- >>> drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- >>> .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ >>> drivers/media/platform/atmel/atmel-isc.h | 50 +- >>> .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- >>> .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- >>> 9 files changed, 685 insertions(+), 241 deletions(-) >>> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c >>> >> >
Hi Eugen, On 22/06/2022 14:25, Eugen.Hristev@microchip.com wrote: > On 6/22/22 2:53 PM, Hans Verkuil wrote: >> Hi Eugen, >> >> On 03/05/2022 11:51, Eugen Hristev wrote: >>> This series is a split from the series : >>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller >>> and it includes the media controller part. >>> previous fixes were sent on a different patch series. >>> >>> As discussed on the ML, moving forward with having the media link validate at >>> start/stop streaming call. >>> I will test the patch : >>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops >>> afterwards, but that patch requires moving my logic to the new vb2 callbacks. >> >> I'm looking at merging this series, but I would like to have the output of >> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all >> correct. > > Hello Hans, > > Please have a look at attached file . Unless you want me to add the > whole output to the e-mail ? No, this is fine, thank you! > > I also added output of media-ctl -p for your convenience. > the subdev2 is a device and driver that is not upstream and has some > compliance issues, they are reported by the v4l2-compliance tool, but > they should not affect this series, it's a synopsys driver that was > rejected on mainline a few years ago, I took it for internal usage, but > it's not cleaned up nor worked a lot upon. OK, good to know. From the compliance output: v4l2-compliance 1.22.1, 32 bits, 32-bit time_t This is an old v4l2-compliance version. Compile it directly from the v4l-utils git repo and check the output again. Compliance test for atmel_isc_commo device /dev/media0: As you can see, the driver name is cut off. Isn't 'atmel-isc' a better name? > >> >> And one more question which may have been answered already in the past: >> >> Changing to the MC will break existing applications, doesn't it? Or did I >> miss something? >> > > The existing applications will have to configure the pipeline now. It > will no longer work by configuring just the top video node /dev/video0 . > They would have to use media-ctl for it, something similar with this set > of commands: > > media-ctl -d /dev/media0 --set-v4l2 '"imx219 > 1-0010":0[fmt:SRGGB10_1X10/1920x1080]' > media-ctl -d /dev/media0 --set-v4l2 > '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]' > media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]' > media-ctl -d /dev/media0 --set-v4l2 > '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]' I'd like to see this documented in a new Documentation/admin-guide/media/atmel-isc.rst file. That can be a new patch. Regards, Hans > > Thank you for taking care of this ! > > Eugen > >> Regards, >> >> Hans >> >>> >>> Full series history: >>> >>> Changes in v10: >>> -> split the series into this first fixes part. >>> -> moved IO_MC addition from first patch to the second patch on the driver changes >>> -> edited commit messages >>> -> DT nodes now disabled by default. >>> >>> Changes in v9: >>> -> kernel robot reported isc_link_validate is not static, changed to static. >>> >>> Changes in v8: >>> -> scaler: modified crop bounds to have the exact source size >>> >>> Changes in v7: >>> -> scaler: modified crop bounds to have maximum isc size >>> -> format propagation: did small changes as per Jacopo review >>> >>> >>> Changes in v6: >>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review >>> -> worked on isc-base enum_fmt , reworked as per Jacopo review >>> >>> Changes in v5: >>> -> removed patch that removed the 'stop' variable as it was still required >>> -> added two new trivial patches >>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo >>> >>> >>> Changes in v4: >>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked >>> one patch that was using it >>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation >>> >>> >>> Changes in v3: >>> - change in bindings, small fixes in csi2dc driver and conversion to mc >>> for the isc-base. >>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS >>> >>> Changes in v2: >>> - integrated many changes suggested by Jacopo in the review of the v1 series. >>> - add a few new patches >>> >>> Eugen Hristev (5): >>> media: atmel: atmel-isc: prepare for media controller support >>> media: atmel: atmel-isc: implement media controller >>> ARM: dts: at91: sama7g5: add nodes for video capture >>> ARM: configs: at91: sama7: add xisc and csi2dc >>> ARM: multi_v7_defconfig: add atmel video pipeline modules >>> >>> arch/arm/boot/dts/sama7g5.dtsi | 51 ++ >>> arch/arm/configs/multi_v7_defconfig | 3 + >>> arch/arm/configs/sama7_defconfig | 2 + >>> drivers/media/platform/atmel/Makefile | 2 +- >>> drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- >>> .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ >>> drivers/media/platform/atmel/atmel-isc.h | 50 +- >>> .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- >>> .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- >>> 9 files changed, 685 insertions(+), 241 deletions(-) >>> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c >>> >> >
On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote: > On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote: >> On 6/22/22 2:53 PM, Hans Verkuil wrote: >>> Hi Eugen, >>> >>> On 03/05/2022 11:51, Eugen Hristev wrote: >>>> This series is a split from the series : >>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller >>>> and it includes the media controller part. >>>> previous fixes were sent on a different patch series. >>>> >>>> As discussed on the ML, moving forward with having the media link validate at >>>> start/stop streaming call. >>>> I will test the patch : >>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops >>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks. >>> >>> I'm looking at merging this series, but I would like to have the output of >>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all >>> correct. >> >> Hello Hans, >> >> Please have a look at attached file . Unless you want me to add the >> whole output to the e-mail ? >> >> I also added output of media-ctl -p for your convenience. >> the subdev2 is a device and driver that is not upstream and has some >> compliance issues, they are reported by the v4l2-compliance tool, but >> they should not affect this series, it's a synopsys driver that was >> rejected on mainline a few years ago, I took it for internal usage, but >> it's not cleaned up nor worked a lot upon. >> >>> >>> And one more question which may have been answered already in the past: >>> >>> Changing to the MC will break existing applications, doesn't it? Or did I >>> miss something? >>> >> >> The existing applications will have to configure the pipeline now. It >> will no longer work by configuring just the top video node /dev/video0 . >> They would have to use media-ctl for it, something similar with this set >> of commands: > > To add on top of that, actually, the reality is that without the MC > support in atmel-isc , some of our platforms do not work at all, because > the csi2dc driver which is in the middle of the pipeline, is a MC > driver. So it will not work without configuring it with MC anyway. It > used to work in a very preliminary version of the csi2dc driver which I > sent a few years ago, but that way of handling things was rejected. > Hence I changed the csi2dc to being full-MC driver (requested for new > drivers) and now I am completing the conversion for the whole pipeline. > We are using this MC-centric approach in production for our products to > be as close as possible to mainline, and backported it to our 5.15 > internal releases, which people are using right now. I'm not all that keen on breaking userspace for those who do NOT use the Atmel BSP. Basically some platforms are currently broken, and with this patch series some other platforms are broken, but at least can be fixed by changing userspace. How feasible is it to do something similar that TI did for the cal driver? (drivers/media/platform/ti/cal) I.e., based on a module option the MC is enabled or disabled. And if a csi2dc is present, then the MC API is always enabled. Regards, Hans > >> >> media-ctl -d /dev/media0 --set-v4l2 '"imx219 >> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]' >> media-ctl -d /dev/media0 --set-v4l2 >> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]' >> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]' >> media-ctl -d /dev/media0 --set-v4l2 >> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]' >> >> Thank you for taking care of this ! >> >> Eugen >> >>> Regards, >>> >>> Hans >>> >>>> >>>> Full series history: >>>> >>>> Changes in v10: >>>> -> split the series into this first fixes part. >>>> -> moved IO_MC addition from first patch to the second patch on the driver changes >>>> -> edited commit messages >>>> -> DT nodes now disabled by default. >>>> >>>> Changes in v9: >>>> -> kernel robot reported isc_link_validate is not static, changed to static. >>>> >>>> Changes in v8: >>>> -> scaler: modified crop bounds to have the exact source size >>>> >>>> Changes in v7: >>>> -> scaler: modified crop bounds to have maximum isc size >>>> -> format propagation: did small changes as per Jacopo review >>>> >>>> >>>> Changes in v6: >>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review >>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review >>>> >>>> Changes in v5: >>>> -> removed patch that removed the 'stop' variable as it was still required >>>> -> added two new trivial patches >>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo >>>> >>>> >>>> Changes in v4: >>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked >>>> one patch that was using it >>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation >>>> >>>> >>>> Changes in v3: >>>> - change in bindings, small fixes in csi2dc driver and conversion to mc >>>> for the isc-base. >>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS >>>> >>>> Changes in v2: >>>> - integrated many changes suggested by Jacopo in the review of the v1 series. >>>> - add a few new patches >>>> >>>> Eugen Hristev (5): >>>> media: atmel: atmel-isc: prepare for media controller support >>>> media: atmel: atmel-isc: implement media controller >>>> ARM: dts: at91: sama7g5: add nodes for video capture >>>> ARM: configs: at91: sama7: add xisc and csi2dc >>>> ARM: multi_v7_defconfig: add atmel video pipeline modules >>>> >>>> arch/arm/boot/dts/sama7g5.dtsi | 51 ++ >>>> arch/arm/configs/multi_v7_defconfig | 3 + >>>> arch/arm/configs/sama7_defconfig | 2 + >>>> drivers/media/platform/atmel/Makefile | 2 +- >>>> drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- >>>> .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ >>>> drivers/media/platform/atmel/atmel-isc.h | 50 +- >>>> .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- >>>> .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- >>>> 9 files changed, 685 insertions(+), 241 deletions(-) >>>> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c >>>> >>> >> >
On 6/22/22 4:35 PM, Hans Verkuil wrote: > Hi Eugen, > > On 22/06/2022 14:25, Eugen.Hristev@microchip.com wrote: >> On 6/22/22 2:53 PM, Hans Verkuil wrote: >>> Hi Eugen, >>> >>> On 03/05/2022 11:51, Eugen Hristev wrote: >>>> This series is a split from the series : >>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller >>>> and it includes the media controller part. >>>> previous fixes were sent on a different patch series. >>>> >>>> As discussed on the ML, moving forward with having the media link validate at >>>> start/stop streaming call. >>>> I will test the patch : >>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops >>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks. >>> >>> I'm looking at merging this series, but I would like to have the output of >>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all >>> correct. >> >> Hello Hans, >> >> Please have a look at attached file . Unless you want me to add the >> whole output to the e-mail ? > > No, this is fine, thank you! > >> >> I also added output of media-ctl -p for your convenience. >> the subdev2 is a device and driver that is not upstream and has some >> compliance issues, they are reported by the v4l2-compliance tool, but >> they should not affect this series, it's a synopsys driver that was >> rejected on mainline a few years ago, I took it for internal usage, but >> it's not cleaned up nor worked a lot upon. > > OK, good to know. > > From the compliance output: > > v4l2-compliance 1.22.1, 32 bits, 32-bit time_t > > This is an old v4l2-compliance version. Compile it directly from the > v4l-utils git repo and check the output again. Okay, I will start a build. I have to get my full environment ready, fire up a Buildroot. It will be ready tomorrow. > > Compliance test for atmel_isc_commo device /dev/media0: > > As you can see, the driver name is cut off. Isn't 'atmel-isc' > a better name? Maybe, but this name is the name of the entity, and is taken from the name of the kernel module. Since the split of the isc driver into atmel_isc_common and atmel-sama5d2-isc , atmel-sama7g5-isc , this name has been set for the module here: https://git.linuxtv.org/hverkuil/media_tree.git/tree/drivers/media/platform/atmel/Makefile?h=for-v5.20e#n7 And the ISC driver takes the module name and uses it for video name here: https://git.linuxtv.org/hverkuil/media_tree.git/tree/drivers/media/platform/atmel/atmel-isc-base.c?h=for-v5.20e#n1999 > >> >>> >>> And one more question which may have been answered already in the past: >>> >>> Changing to the MC will break existing applications, doesn't it? Or did I >>> miss something? >>> >> >> The existing applications will have to configure the pipeline now. It >> will no longer work by configuring just the top video node /dev/video0 . >> They would have to use media-ctl for it, something similar with this set >> of commands: >> >> media-ctl -d /dev/media0 --set-v4l2 '"imx219 >> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]' >> media-ctl -d /dev/media0 --set-v4l2 >> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]' >> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]' >> media-ctl -d /dev/media0 --set-v4l2 >> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]' > > I'd like to see this documented in a new > Documentation/admin-guide/media/atmel-isc.rst file. That can be a new patch. Allright, I can do that. > > Regards, > > Hans > >> >> Thank you for taking care of this ! >> >> Eugen >> >>> Regards, >>> >>> Hans >>> >>>> >>>> Full series history: >>>> >>>> Changes in v10: >>>> -> split the series into this first fixes part. >>>> -> moved IO_MC addition from first patch to the second patch on the driver changes >>>> -> edited commit messages >>>> -> DT nodes now disabled by default. >>>> >>>> Changes in v9: >>>> -> kernel robot reported isc_link_validate is not static, changed to static. >>>> >>>> Changes in v8: >>>> -> scaler: modified crop bounds to have the exact source size >>>> >>>> Changes in v7: >>>> -> scaler: modified crop bounds to have maximum isc size >>>> -> format propagation: did small changes as per Jacopo review >>>> >>>> >>>> Changes in v6: >>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review >>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review >>>> >>>> Changes in v5: >>>> -> removed patch that removed the 'stop' variable as it was still required >>>> -> added two new trivial patches >>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo >>>> >>>> >>>> Changes in v4: >>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked >>>> one patch that was using it >>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation >>>> >>>> >>>> Changes in v3: >>>> - change in bindings, small fixes in csi2dc driver and conversion to mc >>>> for the isc-base. >>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS >>>> >>>> Changes in v2: >>>> - integrated many changes suggested by Jacopo in the review of the v1 series. >>>> - add a few new patches >>>> >>>> Eugen Hristev (5): >>>> media: atmel: atmel-isc: prepare for media controller support >>>> media: atmel: atmel-isc: implement media controller >>>> ARM: dts: at91: sama7g5: add nodes for video capture >>>> ARM: configs: at91: sama7: add xisc and csi2dc >>>> ARM: multi_v7_defconfig: add atmel video pipeline modules >>>> >>>> arch/arm/boot/dts/sama7g5.dtsi | 51 ++ >>>> arch/arm/configs/multi_v7_defconfig | 3 + >>>> arch/arm/configs/sama7_defconfig | 2 + >>>> drivers/media/platform/atmel/Makefile | 2 +- >>>> drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- >>>> .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ >>>> drivers/media/platform/atmel/atmel-isc.h | 50 +- >>>> .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- >>>> .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- >>>> 9 files changed, 685 insertions(+), 241 deletions(-) >>>> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c >>>> >>> >> >
On 6/22/22 4:47 PM, Hans Verkuil wrote: > On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote: >> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote: >>> On 6/22/22 2:53 PM, Hans Verkuil wrote: >>>> Hi Eugen, >>>> >>>> On 03/05/2022 11:51, Eugen Hristev wrote: >>>>> This series is a split from the series : >>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller >>>>> and it includes the media controller part. >>>>> previous fixes were sent on a different patch series. >>>>> >>>>> As discussed on the ML, moving forward with having the media link validate at >>>>> start/stop streaming call. >>>>> I will test the patch : >>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops >>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks. >>>> >>>> I'm looking at merging this series, but I would like to have the output of >>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all >>>> correct. >>> >>> Hello Hans, >>> >>> Please have a look at attached file . Unless you want me to add the >>> whole output to the e-mail ? >>> >>> I also added output of media-ctl -p for your convenience. >>> the subdev2 is a device and driver that is not upstream and has some >>> compliance issues, they are reported by the v4l2-compliance tool, but >>> they should not affect this series, it's a synopsys driver that was >>> rejected on mainline a few years ago, I took it for internal usage, but >>> it's not cleaned up nor worked a lot upon. >>> >>>> >>>> And one more question which may have been answered already in the past: >>>> >>>> Changing to the MC will break existing applications, doesn't it? Or did I >>>> miss something? >>>> >>> >>> The existing applications will have to configure the pipeline now. It >>> will no longer work by configuring just the top video node /dev/video0 . >>> They would have to use media-ctl for it, something similar with this set >>> of commands: >> >> To add on top of that, actually, the reality is that without the MC >> support in atmel-isc , some of our platforms do not work at all, because >> the csi2dc driver which is in the middle of the pipeline, is a MC >> driver. So it will not work without configuring it with MC anyway. It >> used to work in a very preliminary version of the csi2dc driver which I >> sent a few years ago, but that way of handling things was rejected. >> Hence I changed the csi2dc to being full-MC driver (requested for new >> drivers) and now I am completing the conversion for the whole pipeline. >> We are using this MC-centric approach in production for our products to >> be as close as possible to mainline, and backported it to our 5.15 >> internal releases, which people are using right now. > > I'm not all that keen on breaking userspace for those who do NOT use the > Atmel BSP. Basically some platforms are currently broken, and with this patch > series some other platforms are broken, but at least can be fixed by changing > userspace. > > How feasible is it to do something similar that TI did for the cal driver? > (drivers/media/platform/ti/cal) > > I.e., based on a module option the MC is enabled or disabled. And if a > csi2dc is present, then the MC API is always enabled. Some platforms that are now broken, never worked, because they would need all the modules to be configured with MC anyway. The other platforms, that work now by just configuring the top video node, work, but in a quite limited way, as sensor drivers can be MC-capable and configurable from userspace, but the top video driver will overwrite their configuration based on it's own decision algorithm. With the MC approach, all the platforms work, the drawback is, as you said, that the userspace has to configure the pipeline from head to toe; but , we knew that: moving to a MC approach, makes the old way of configuring the image capture simply 'not enough'. It would be difficult to maintain a driver that would use the MC API and handle the things for itself and just check the pipeline; and in the same time if a module parameter is different, pass configuration down the pipeline and have an algorithm implemented that would interact with the subdev, ask for it's capabilities, and then decide on its own what the subdev would use. The driver would be a bit big and it would have a lot of code. That is one of the advantages of these patches, to simplify the driver. I would prefer to not have to keep that code, and move to MC approach, but in the end you have arguments and you are in charge. Eugen > > Regards, > > Hans > >> >>> >>> media-ctl -d /dev/media0 --set-v4l2 '"imx219 >>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]' >>> media-ctl -d /dev/media0 --set-v4l2 >>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]' >>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]' >>> media-ctl -d /dev/media0 --set-v4l2 >>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]' >>> >>> Thank you for taking care of this ! >>> >>> Eugen >>> >>>> Regards, >>>> >>>> Hans >>>> >>>>> >>>>> Full series history: >>>>> >>>>> Changes in v10: >>>>> -> split the series into this first fixes part. >>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes >>>>> -> edited commit messages >>>>> -> DT nodes now disabled by default. >>>>> >>>>> Changes in v9: >>>>> -> kernel robot reported isc_link_validate is not static, changed to static. >>>>> >>>>> Changes in v8: >>>>> -> scaler: modified crop bounds to have the exact source size >>>>> >>>>> Changes in v7: >>>>> -> scaler: modified crop bounds to have maximum isc size >>>>> -> format propagation: did small changes as per Jacopo review >>>>> >>>>> >>>>> Changes in v6: >>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review >>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review >>>>> >>>>> Changes in v5: >>>>> -> removed patch that removed the 'stop' variable as it was still required >>>>> -> added two new trivial patches >>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo >>>>> >>>>> >>>>> Changes in v4: >>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked >>>>> one patch that was using it >>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation >>>>> >>>>> >>>>> Changes in v3: >>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc >>>>> for the isc-base. >>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS >>>>> >>>>> Changes in v2: >>>>> - integrated many changes suggested by Jacopo in the review of the v1 series. >>>>> - add a few new patches >>>>> >>>>> Eugen Hristev (5): >>>>> media: atmel: atmel-isc: prepare for media controller support >>>>> media: atmel: atmel-isc: implement media controller >>>>> ARM: dts: at91: sama7g5: add nodes for video capture >>>>> ARM: configs: at91: sama7: add xisc and csi2dc >>>>> ARM: multi_v7_defconfig: add atmel video pipeline modules >>>>> >>>>> arch/arm/boot/dts/sama7g5.dtsi | 51 ++ >>>>> arch/arm/configs/multi_v7_defconfig | 3 + >>>>> arch/arm/configs/sama7_defconfig | 2 + >>>>> drivers/media/platform/atmel/Makefile | 2 +- >>>>> drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- >>>>> .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ >>>>> drivers/media/platform/atmel/atmel-isc.h | 50 +- >>>>> .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- >>>>> .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- >>>>> 9 files changed, 685 insertions(+), 241 deletions(-) >>>>> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c >>>>> >>>> >>> >> >
Hi Hans, Eugen On Wed, Jun 22, 2022 at 03:47:33PM +0200, Hans Verkuil wrote: > On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote: > > On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote: > >> On 6/22/22 2:53 PM, Hans Verkuil wrote: > >>> Hi Eugen, > >>> > >>> On 03/05/2022 11:51, Eugen Hristev wrote: > >>>> This series is a split from the series : > >>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller > >>>> and it includes the media controller part. > >>>> previous fixes were sent on a different patch series. > >>>> > >>>> As discussed on the ML, moving forward with having the media link validate at > >>>> start/stop streaming call. > >>>> I will test the patch : > >>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops > >>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks. > >>> > >>> I'm looking at merging this series, but I would like to have the output of > >>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all > >>> correct. > >> > >> Hello Hans, > >> > >> Please have a look at attached file . Unless you want me to add the > >> whole output to the e-mail ? > >> > >> I also added output of media-ctl -p for your convenience. > >> the subdev2 is a device and driver that is not upstream and has some > >> compliance issues, they are reported by the v4l2-compliance tool, but > >> they should not affect this series, it's a synopsys driver that was > >> rejected on mainline a few years ago, I took it for internal usage, but > >> it's not cleaned up nor worked a lot upon. > >> > >>> > >>> And one more question which may have been answered already in the past: > >>> > >>> Changing to the MC will break existing applications, doesn't it? Or did I > >>> miss something? > >>> > >> > >> The existing applications will have to configure the pipeline now. It > >> will no longer work by configuring just the top video node /dev/video0 . > >> They would have to use media-ctl for it, something similar with this set > >> of commands: > > > > To add on top of that, actually, the reality is that without the MC > > support in atmel-isc , some of our platforms do not work at all, because > > the csi2dc driver which is in the middle of the pipeline, is a MC > > driver. So it will not work without configuring it with MC anyway. It > > used to work in a very preliminary version of the csi2dc driver which I > > sent a few years ago, but that way of handling things was rejected. > > Hence I changed the csi2dc to being full-MC driver (requested for new > > drivers) and now I am completing the conversion for the whole pipeline. > > We are using this MC-centric approach in production for our products to > > be as close as possible to mainline, and backported it to our 5.15 > > internal releases, which people are using right now. > > I'm not all that keen on breaking userspace for those who do NOT use the > Atmel BSP. Basically some platforms are currently broken, and with this patch > series some other platforms are broken, but at least can be fixed by changing > userspace. > > How feasible is it to do something similar that TI did for the cal driver? > (drivers/media/platform/ti/cal) > > I.e., based on a module option the MC is enabled or disabled. And if a > csi2dc is present, then the MC API is always enabled. > I think I have suggested Eugen to move to MC when he started looking in libcamera, so sorry for the intrusion but I feel a bit bad for not rising the point earlier and get him to v10 I understand your point Hans, and when a vendor upstreaming code or a user requires to maintain compatibility, the burden of keeping more code in to handle the MC and non-MC cases is worth the complications. But if even the vendor wants to move to MC to allow more use-cases I think we have to acknolege that if you're running mainline on an embedded system you could expect to adjust your setup between kernel updates. The idea to document the media-ctl commands required to setup the pipeline it's helpful, and might help in the interim period until the platform is not supported by libcamera. That said, if Eugen wants to give the flag a try I won't oppose :) > Regards, > > Hans > > > > >> > >> media-ctl -d /dev/media0 --set-v4l2 '"imx219 > >> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]' > >> media-ctl -d /dev/media0 --set-v4l2 > >> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]' > >> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]' > >> media-ctl -d /dev/media0 --set-v4l2 > >> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]' > >> > >> Thank you for taking care of this ! > >> > >> Eugen > >> > >>> Regards, > >>> > >>> Hans > >>> > >>>> > >>>> Full series history: > >>>> > >>>> Changes in v10: > >>>> -> split the series into this first fixes part. > >>>> -> moved IO_MC addition from first patch to the second patch on the driver changes > >>>> -> edited commit messages > >>>> -> DT nodes now disabled by default. > >>>> > >>>> Changes in v9: > >>>> -> kernel robot reported isc_link_validate is not static, changed to static. > >>>> > >>>> Changes in v8: > >>>> -> scaler: modified crop bounds to have the exact source size > >>>> > >>>> Changes in v7: > >>>> -> scaler: modified crop bounds to have maximum isc size > >>>> -> format propagation: did small changes as per Jacopo review > >>>> > >>>> > >>>> Changes in v6: > >>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review > >>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review > >>>> > >>>> Changes in v5: > >>>> -> removed patch that removed the 'stop' variable as it was still required > >>>> -> added two new trivial patches > >>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo > >>>> > >>>> > >>>> Changes in v4: > >>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked > >>>> one patch that was using it > >>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation > >>>> > >>>> > >>>> Changes in v3: > >>>> - change in bindings, small fixes in csi2dc driver and conversion to mc > >>>> for the isc-base. > >>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS > >>>> > >>>> Changes in v2: > >>>> - integrated many changes suggested by Jacopo in the review of the v1 series. > >>>> - add a few new patches > >>>> > >>>> Eugen Hristev (5): > >>>> media: atmel: atmel-isc: prepare for media controller support > >>>> media: atmel: atmel-isc: implement media controller > >>>> ARM: dts: at91: sama7g5: add nodes for video capture > >>>> ARM: configs: at91: sama7: add xisc and csi2dc > >>>> ARM: multi_v7_defconfig: add atmel video pipeline modules > >>>> > >>>> arch/arm/boot/dts/sama7g5.dtsi | 51 ++ > >>>> arch/arm/configs/multi_v7_defconfig | 3 + > >>>> arch/arm/configs/sama7_defconfig | 2 + > >>>> drivers/media/platform/atmel/Makefile | 2 +- > >>>> drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- > >>>> .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ > >>>> drivers/media/platform/atmel/atmel-isc.h | 50 +- > >>>> .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- > >>>> .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- > >>>> 9 files changed, 685 insertions(+), 241 deletions(-) > >>>> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c > >>>> > >>> > >> > > >
On 6/22/22 5:14 PM, Jacopo Mondi wrote: > Hi Hans, Eugen > > On Wed, Jun 22, 2022 at 03:47:33PM +0200, Hans Verkuil wrote: >> On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote: >>> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote: >>>> On 6/22/22 2:53 PM, Hans Verkuil wrote: >>>>> Hi Eugen, >>>>> >>>>> On 03/05/2022 11:51, Eugen Hristev wrote: >>>>>> This series is a split from the series : >>>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller >>>>>> and it includes the media controller part. >>>>>> previous fixes were sent on a different patch series. >>>>>> >>>>>> As discussed on the ML, moving forward with having the media link validate at >>>>>> start/stop streaming call. >>>>>> I will test the patch : >>>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops >>>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks. >>>>> >>>>> I'm looking at merging this series, but I would like to have the output of >>>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all >>>>> correct. >>>> >>>> Hello Hans, >>>> >>>> Please have a look at attached file . Unless you want me to add the >>>> whole output to the e-mail ? >>>> >>>> I also added output of media-ctl -p for your convenience. >>>> the subdev2 is a device and driver that is not upstream and has some >>>> compliance issues, they are reported by the v4l2-compliance tool, but >>>> they should not affect this series, it's a synopsys driver that was >>>> rejected on mainline a few years ago, I took it for internal usage, but >>>> it's not cleaned up nor worked a lot upon. >>>> >>>>> >>>>> And one more question which may have been answered already in the past: >>>>> >>>>> Changing to the MC will break existing applications, doesn't it? Or did I >>>>> miss something? >>>>> >>>> >>>> The existing applications will have to configure the pipeline now. It >>>> will no longer work by configuring just the top video node /dev/video0 . >>>> They would have to use media-ctl for it, something similar with this set >>>> of commands: >>> >>> To add on top of that, actually, the reality is that without the MC >>> support in atmel-isc , some of our platforms do not work at all, because >>> the csi2dc driver which is in the middle of the pipeline, is a MC >>> driver. So it will not work without configuring it with MC anyway. It >>> used to work in a very preliminary version of the csi2dc driver which I >>> sent a few years ago, but that way of handling things was rejected. >>> Hence I changed the csi2dc to being full-MC driver (requested for new >>> drivers) and now I am completing the conversion for the whole pipeline. >>> We are using this MC-centric approach in production for our products to >>> be as close as possible to mainline, and backported it to our 5.15 >>> internal releases, which people are using right now. >> >> I'm not all that keen on breaking userspace for those who do NOT use the >> Atmel BSP. Basically some platforms are currently broken, and with this patch >> series some other platforms are broken, but at least can be fixed by changing >> userspace. >> >> How feasible is it to do something similar that TI did for the cal driver? >> (drivers/media/platform/ti/cal) >> >> I.e., based on a module option the MC is enabled or disabled. And if a >> csi2dc is present, then the MC API is always enabled. >> > > I think I have suggested Eugen to move to MC when he > started looking in libcamera, so sorry for the intrusion but I feel > a bit bad for not rising the point earlier and get him to v10 Don't get me wrong, I like the MC approach, after implementing it, I am happy how the driver turned out. It's much simpler and covering a plethora of new use cases which were previously not available, as it was much more rigid. So I agree with what you suggested, and I support the idea as well. > > I understand your point Hans, and when a vendor upstreaming code or a > user requires to maintain compatibility, the burden of keeping more > code in to handle the MC and non-MC cases is worth the complications. We are pretty much convinced to use the MC-only approach and are moving in that direction. But, if we have to keep the old code to maintain backwards compatibility , we have no choice. However, we will move forward, and only use the MC approach from now on, and will no longer use this driver without MC. That use case will be out of our scope. If there are people using it and it works, all the better then. > > But if even the vendor wants to move to MC to allow more use-cases I > think we have to acknolege that if you're running mainline on an > embedded system you could expect to adjust your setup between kernel > updates. The idea to document the media-ctl commands required to setup > the pipeline it's helpful, and might help in the interim period until > the platform is not supported by libcamera. > > That said, if Eugen wants to give the flag a try I won't > oppose :) > > >> Regards, >> >> Hans >> >>> >>>> >>>> media-ctl -d /dev/media0 --set-v4l2 '"imx219 >>>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]' >>>> media-ctl -d /dev/media0 --set-v4l2 >>>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]' >>>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]' >>>> media-ctl -d /dev/media0 --set-v4l2 >>>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]' >>>> >>>> Thank you for taking care of this ! >>>> >>>> Eugen >>>> >>>>> Regards, >>>>> >>>>> Hans >>>>> >>>>>> >>>>>> Full series history: >>>>>> >>>>>> Changes in v10: >>>>>> -> split the series into this first fixes part. >>>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes >>>>>> -> edited commit messages >>>>>> -> DT nodes now disabled by default. >>>>>> >>>>>> Changes in v9: >>>>>> -> kernel robot reported isc_link_validate is not static, changed to static. >>>>>> >>>>>> Changes in v8: >>>>>> -> scaler: modified crop bounds to have the exact source size >>>>>> >>>>>> Changes in v7: >>>>>> -> scaler: modified crop bounds to have maximum isc size >>>>>> -> format propagation: did small changes as per Jacopo review >>>>>> >>>>>> >>>>>> Changes in v6: >>>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review >>>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review >>>>>> >>>>>> Changes in v5: >>>>>> -> removed patch that removed the 'stop' variable as it was still required >>>>>> -> added two new trivial patches >>>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo >>>>>> >>>>>> >>>>>> Changes in v4: >>>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked >>>>>> one patch that was using it >>>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation >>>>>> >>>>>> >>>>>> Changes in v3: >>>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc >>>>>> for the isc-base. >>>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS >>>>>> >>>>>> Changes in v2: >>>>>> - integrated many changes suggested by Jacopo in the review of the v1 series. >>>>>> - add a few new patches >>>>>> >>>>>> Eugen Hristev (5): >>>>>> media: atmel: atmel-isc: prepare for media controller support >>>>>> media: atmel: atmel-isc: implement media controller >>>>>> ARM: dts: at91: sama7g5: add nodes for video capture >>>>>> ARM: configs: at91: sama7: add xisc and csi2dc >>>>>> ARM: multi_v7_defconfig: add atmel video pipeline modules >>>>>> >>>>>> arch/arm/boot/dts/sama7g5.dtsi | 51 ++ >>>>>> arch/arm/configs/multi_v7_defconfig | 3 + >>>>>> arch/arm/configs/sama7_defconfig | 2 + >>>>>> drivers/media/platform/atmel/Makefile | 2 +- >>>>>> drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- >>>>>> .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ >>>>>> drivers/media/platform/atmel/atmel-isc.h | 50 +- >>>>>> .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- >>>>>> .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- >>>>>> 9 files changed, 685 insertions(+), 241 deletions(-) >>>>>> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c >>>>>> >>>>> >>>> >>> >>
Hi Eugen, Jacopo, On 22/06/2022 16:14, Jacopo Mondi wrote: > Hi Hans, Eugen > > On Wed, Jun 22, 2022 at 03:47:33PM +0200, Hans Verkuil wrote: >> On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote: >>> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote: >>>> On 6/22/22 2:53 PM, Hans Verkuil wrote: >>>>> Hi Eugen, >>>>> >>>>> On 03/05/2022 11:51, Eugen Hristev wrote: >>>>>> This series is a split from the series : >>>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller >>>>>> and it includes the media controller part. >>>>>> previous fixes were sent on a different patch series. >>>>>> >>>>>> As discussed on the ML, moving forward with having the media link validate at >>>>>> start/stop streaming call. >>>>>> I will test the patch : >>>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops >>>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks. >>>>> >>>>> I'm looking at merging this series, but I would like to have the output of >>>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all >>>>> correct. >>>> >>>> Hello Hans, >>>> >>>> Please have a look at attached file . Unless you want me to add the >>>> whole output to the e-mail ? >>>> >>>> I also added output of media-ctl -p for your convenience. >>>> the subdev2 is a device and driver that is not upstream and has some >>>> compliance issues, they are reported by the v4l2-compliance tool, but >>>> they should not affect this series, it's a synopsys driver that was >>>> rejected on mainline a few years ago, I took it for internal usage, but >>>> it's not cleaned up nor worked a lot upon. >>>> >>>>> >>>>> And one more question which may have been answered already in the past: >>>>> >>>>> Changing to the MC will break existing applications, doesn't it? Or did I >>>>> miss something? >>>>> >>>> >>>> The existing applications will have to configure the pipeline now. It >>>> will no longer work by configuring just the top video node /dev/video0 . >>>> They would have to use media-ctl for it, something similar with this set >>>> of commands: >>> >>> To add on top of that, actually, the reality is that without the MC >>> support in atmel-isc , some of our platforms do not work at all, because >>> the csi2dc driver which is in the middle of the pipeline, is a MC >>> driver. So it will not work without configuring it with MC anyway. It >>> used to work in a very preliminary version of the csi2dc driver which I >>> sent a few years ago, but that way of handling things was rejected. >>> Hence I changed the csi2dc to being full-MC driver (requested for new >>> drivers) and now I am completing the conversion for the whole pipeline. >>> We are using this MC-centric approach in production for our products to >>> be as close as possible to mainline, and backported it to our 5.15 >>> internal releases, which people are using right now. >> >> I'm not all that keen on breaking userspace for those who do NOT use the >> Atmel BSP. Basically some platforms are currently broken, and with this patch >> series some other platforms are broken, but at least can be fixed by changing >> userspace. >> >> How feasible is it to do something similar that TI did for the cal driver? >> (drivers/media/platform/ti/cal) >> >> I.e., based on a module option the MC is enabled or disabled. And if a >> csi2dc is present, then the MC API is always enabled. >> > > I think I have suggested Eugen to move to MC when he > started looking in libcamera, so sorry for the intrusion but I feel > a bit bad for not rising the point earlier and get him to v10 > > I understand your point Hans, and when a vendor upstreaming code or a > user requires to maintain compatibility, the burden of keeping more > code in to handle the MC and non-MC cases is worth the complications. Eugen, can you provide a list of platforms that will break with this change and which platforms are currently broken without this series? I'm trying to get a bit of a feel of the potential problems this change will introduce. > > But if even the vendor wants to move to MC to allow more use-cases I > think we have to acknolege that if you're running mainline on an > embedded system you could expect to adjust your setup between kernel > updates. The idea to document the media-ctl commands required to setup > the pipeline it's helpful, and might help in the interim period until > the platform is not supported by libcamera. Well, I don't want Linus to start yelling at me for breaking userspace :-) We have broken userspace API (intentionally) in the past, but only with good reasons. And sometimes a driver is used so rarely that it is not worth the effort to try and keep compatible. As a developer I'd love to just forget about the old API, but as subsystem maintainer I need good arguments. Another option might be to take the TI cal approach, but have warnings that it will be removed in, say, 2 years time. Or even make a copy of the driver for the old platforms, and perhaps move that to staging to be removed eventually. The idea of a sudden breakage when going from kernel K to K+1 doesn't sit well with me, if there was a transition period of 1-2 years then that would be better. Regards, Hans > > That said, if Eugen wants to give the flag a try I won't > oppose :) > > >> Regards, >> >> Hans >> >>> >>>> >>>> media-ctl -d /dev/media0 --set-v4l2 '"imx219 >>>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]' >>>> media-ctl -d /dev/media0 --set-v4l2 >>>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]' >>>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]' >>>> media-ctl -d /dev/media0 --set-v4l2 >>>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]' >>>> >>>> Thank you for taking care of this ! >>>> >>>> Eugen >>>> >>>>> Regards, >>>>> >>>>> Hans >>>>> >>>>>> >>>>>> Full series history: >>>>>> >>>>>> Changes in v10: >>>>>> -> split the series into this first fixes part. >>>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes >>>>>> -> edited commit messages >>>>>> -> DT nodes now disabled by default. >>>>>> >>>>>> Changes in v9: >>>>>> -> kernel robot reported isc_link_validate is not static, changed to static. >>>>>> >>>>>> Changes in v8: >>>>>> -> scaler: modified crop bounds to have the exact source size >>>>>> >>>>>> Changes in v7: >>>>>> -> scaler: modified crop bounds to have maximum isc size >>>>>> -> format propagation: did small changes as per Jacopo review >>>>>> >>>>>> >>>>>> Changes in v6: >>>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review >>>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review >>>>>> >>>>>> Changes in v5: >>>>>> -> removed patch that removed the 'stop' variable as it was still required >>>>>> -> added two new trivial patches >>>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo >>>>>> >>>>>> >>>>>> Changes in v4: >>>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked >>>>>> one patch that was using it >>>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation >>>>>> >>>>>> >>>>>> Changes in v3: >>>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc >>>>>> for the isc-base. >>>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS >>>>>> >>>>>> Changes in v2: >>>>>> - integrated many changes suggested by Jacopo in the review of the v1 series. >>>>>> - add a few new patches >>>>>> >>>>>> Eugen Hristev (5): >>>>>> media: atmel: atmel-isc: prepare for media controller support >>>>>> media: atmel: atmel-isc: implement media controller >>>>>> ARM: dts: at91: sama7g5: add nodes for video capture >>>>>> ARM: configs: at91: sama7: add xisc and csi2dc >>>>>> ARM: multi_v7_defconfig: add atmel video pipeline modules >>>>>> >>>>>> arch/arm/boot/dts/sama7g5.dtsi | 51 ++ >>>>>> arch/arm/configs/multi_v7_defconfig | 3 + >>>>>> arch/arm/configs/sama7_defconfig | 2 + >>>>>> drivers/media/platform/atmel/Makefile | 2 +- >>>>>> drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- >>>>>> .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ >>>>>> drivers/media/platform/atmel/atmel-isc.h | 50 +- >>>>>> .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- >>>>>> .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- >>>>>> 9 files changed, 685 insertions(+), 241 deletions(-) >>>>>> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c >>>>>> >>>>> >>>> >>> >>
Hi Hans, On Wed, Jun 22, 2022 at 04:55:27PM +0200, Hans Verkuil wrote: > Hi Eugen, Jacopo, > > On 22/06/2022 16:14, Jacopo Mondi wrote: > > Hi Hans, Eugen > > > > On Wed, Jun 22, 2022 at 03:47:33PM +0200, Hans Verkuil wrote: > >> On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote: > >>> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote: > >>>> On 6/22/22 2:53 PM, Hans Verkuil wrote: > >>>>> Hi Eugen, > >>>>> > >>>>> On 03/05/2022 11:51, Eugen Hristev wrote: > >>>>>> This series is a split from the series : > >>>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller > >>>>>> and it includes the media controller part. > >>>>>> previous fixes were sent on a different patch series. > >>>>>> > >>>>>> As discussed on the ML, moving forward with having the media link validate at > >>>>>> start/stop streaming call. > >>>>>> I will test the patch : > >>>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops > >>>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks. > >>>>> > >>>>> I'm looking at merging this series, but I would like to have the output of > >>>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all > >>>>> correct. > >>>> > >>>> Hello Hans, > >>>> > >>>> Please have a look at attached file . Unless you want me to add the > >>>> whole output to the e-mail ? > >>>> > >>>> I also added output of media-ctl -p for your convenience. > >>>> the subdev2 is a device and driver that is not upstream and has some > >>>> compliance issues, they are reported by the v4l2-compliance tool, but > >>>> they should not affect this series, it's a synopsys driver that was > >>>> rejected on mainline a few years ago, I took it for internal usage, but > >>>> it's not cleaned up nor worked a lot upon. > >>>> > >>>>> > >>>>> And one more question which may have been answered already in the past: > >>>>> > >>>>> Changing to the MC will break existing applications, doesn't it? Or did I > >>>>> miss something? > >>>>> > >>>> > >>>> The existing applications will have to configure the pipeline now. It > >>>> will no longer work by configuring just the top video node /dev/video0 . > >>>> They would have to use media-ctl for it, something similar with this set > >>>> of commands: > >>> > >>> To add on top of that, actually, the reality is that without the MC > >>> support in atmel-isc , some of our platforms do not work at all, because > >>> the csi2dc driver which is in the middle of the pipeline, is a MC > >>> driver. So it will not work without configuring it with MC anyway. It > >>> used to work in a very preliminary version of the csi2dc driver which I > >>> sent a few years ago, but that way of handling things was rejected. > >>> Hence I changed the csi2dc to being full-MC driver (requested for new > >>> drivers) and now I am completing the conversion for the whole pipeline. > >>> We are using this MC-centric approach in production for our products to > >>> be as close as possible to mainline, and backported it to our 5.15 > >>> internal releases, which people are using right now. > >> > >> I'm not all that keen on breaking userspace for those who do NOT use the > >> Atmel BSP. Basically some platforms are currently broken, and with this patch > >> series some other platforms are broken, but at least can be fixed by changing > >> userspace. > >> > >> How feasible is it to do something similar that TI did for the cal driver? > >> (drivers/media/platform/ti/cal) > >> > >> I.e., based on a module option the MC is enabled or disabled. And if a > >> csi2dc is present, then the MC API is always enabled. > >> > > > > I think I have suggested Eugen to move to MC when he > > started looking in libcamera, so sorry for the intrusion but I feel > > a bit bad for not rising the point earlier and get him to v10 > > > > I understand your point Hans, and when a vendor upstreaming code or a > > user requires to maintain compatibility, the burden of keeping more > > code in to handle the MC and non-MC cases is worth the complications. > > Eugen, can you provide a list of platforms that will break with this > change and which platforms are currently broken without this series? > > I'm trying to get a bit of a feel of the potential problems this change > will introduce. > > > > > But if even the vendor wants to move to MC to allow more use-cases I > > think we have to acknolege that if you're running mainline on an > > embedded system you could expect to adjust your setup between kernel > > updates. The idea to document the media-ctl commands required to setup > > the pipeline it's helpful, and might help in the interim period until > > the platform is not supported by libcamera. > > Well, I don't want Linus to start yelling at me for breaking userspace :-) > > We have broken userspace API (intentionally) in the past, but only with > good reasons. And sometimes a driver is used so rarely that it is not worth > the effort to try and keep compatible. > > As a developer I'd love to just forget about the old API, but as subsystem > maintainer I need good arguments. I understand and I think these are all valid concerns. Finding a balance between new features and legacy is not easy. > > Another option might be to take the TI cal approach, but have warnings that > it will be removed in, say, 2 years time. Or even make a copy of the driver > for the old platforms, and perhaps move that to staging to be removed eventually. > > The idea of a sudden breakage when going from kernel K to K+1 doesn't sit > well with me, if there was a transition period of 1-2 years then that would be > better. > If staging works for you that's probably the easiest option. Let's see what Eugen prefers! > Regards, > > Hans > > > > > That said, if Eugen wants to give the flag a try I won't > > oppose :) > > > > > >> Regards, > >> > >> Hans > >> > >>> > >>>> > >>>> media-ctl -d /dev/media0 --set-v4l2 '"imx219 > >>>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]' > >>>> media-ctl -d /dev/media0 --set-v4l2 > >>>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]' > >>>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]' > >>>> media-ctl -d /dev/media0 --set-v4l2 > >>>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]' > >>>> > >>>> Thank you for taking care of this ! > >>>> > >>>> Eugen > >>>> > >>>>> Regards, > >>>>> > >>>>> Hans > >>>>> > >>>>>> > >>>>>> Full series history: > >>>>>> > >>>>>> Changes in v10: > >>>>>> -> split the series into this first fixes part. > >>>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes > >>>>>> -> edited commit messages > >>>>>> -> DT nodes now disabled by default. > >>>>>> > >>>>>> Changes in v9: > >>>>>> -> kernel robot reported isc_link_validate is not static, changed to static. > >>>>>> > >>>>>> Changes in v8: > >>>>>> -> scaler: modified crop bounds to have the exact source size > >>>>>> > >>>>>> Changes in v7: > >>>>>> -> scaler: modified crop bounds to have maximum isc size > >>>>>> -> format propagation: did small changes as per Jacopo review > >>>>>> > >>>>>> > >>>>>> Changes in v6: > >>>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review > >>>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review > >>>>>> > >>>>>> Changes in v5: > >>>>>> -> removed patch that removed the 'stop' variable as it was still required > >>>>>> -> added two new trivial patches > >>>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo > >>>>>> > >>>>>> > >>>>>> Changes in v4: > >>>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked > >>>>>> one patch that was using it > >>>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation > >>>>>> > >>>>>> > >>>>>> Changes in v3: > >>>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc > >>>>>> for the isc-base. > >>>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS > >>>>>> > >>>>>> Changes in v2: > >>>>>> - integrated many changes suggested by Jacopo in the review of the v1 series. > >>>>>> - add a few new patches > >>>>>> > >>>>>> Eugen Hristev (5): > >>>>>> media: atmel: atmel-isc: prepare for media controller support > >>>>>> media: atmel: atmel-isc: implement media controller > >>>>>> ARM: dts: at91: sama7g5: add nodes for video capture > >>>>>> ARM: configs: at91: sama7: add xisc and csi2dc > >>>>>> ARM: multi_v7_defconfig: add atmel video pipeline modules > >>>>>> > >>>>>> arch/arm/boot/dts/sama7g5.dtsi | 51 ++ > >>>>>> arch/arm/configs/multi_v7_defconfig | 3 + > >>>>>> arch/arm/configs/sama7_defconfig | 2 + > >>>>>> drivers/media/platform/atmel/Makefile | 2 +- > >>>>>> drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- > >>>>>> .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ > >>>>>> drivers/media/platform/atmel/atmel-isc.h | 50 +- > >>>>>> .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- > >>>>>> .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- > >>>>>> 9 files changed, 685 insertions(+), 241 deletions(-) > >>>>>> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c > >>>>>> > >>>>> > >>>> > >>> > >> >
On 6/22/22 6:46 PM, Jacopo Mondi wrote: > Hi Hans, > > On Wed, Jun 22, 2022 at 04:55:27PM +0200, Hans Verkuil wrote: >> Hi Eugen, Jacopo, >> >> On 22/06/2022 16:14, Jacopo Mondi wrote: >>> Hi Hans, Eugen >>> >>> On Wed, Jun 22, 2022 at 03:47:33PM +0200, Hans Verkuil wrote: >>>> On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote: >>>>> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote: >>>>>> On 6/22/22 2:53 PM, Hans Verkuil wrote: >>>>>>> Hi Eugen, >>>>>>> >>>>>>> On 03/05/2022 11:51, Eugen Hristev wrote: >>>>>>>> This series is a split from the series : >>>>>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller >>>>>>>> and it includes the media controller part. >>>>>>>> previous fixes were sent on a different patch series. >>>>>>>> >>>>>>>> As discussed on the ML, moving forward with having the media link validate at >>>>>>>> start/stop streaming call. >>>>>>>> I will test the patch : >>>>>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops >>>>>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks. >>>>>>> >>>>>>> I'm looking at merging this series, but I would like to have the output of >>>>>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all >>>>>>> correct. >>>>>> >>>>>> Hello Hans, >>>>>> >>>>>> Please have a look at attached file . Unless you want me to add the >>>>>> whole output to the e-mail ? >>>>>> >>>>>> I also added output of media-ctl -p for your convenience. >>>>>> the subdev2 is a device and driver that is not upstream and has some >>>>>> compliance issues, they are reported by the v4l2-compliance tool, but >>>>>> they should not affect this series, it's a synopsys driver that was >>>>>> rejected on mainline a few years ago, I took it for internal usage, but >>>>>> it's not cleaned up nor worked a lot upon. >>>>>> >>>>>>> >>>>>>> And one more question which may have been answered already in the past: >>>>>>> >>>>>>> Changing to the MC will break existing applications, doesn't it? Or did I >>>>>>> miss something? >>>>>>> >>>>>> >>>>>> The existing applications will have to configure the pipeline now. It >>>>>> will no longer work by configuring just the top video node /dev/video0 . >>>>>> They would have to use media-ctl for it, something similar with this set >>>>>> of commands: >>>>> >>>>> To add on top of that, actually, the reality is that without the MC >>>>> support in atmel-isc , some of our platforms do not work at all, because >>>>> the csi2dc driver which is in the middle of the pipeline, is a MC >>>>> driver. So it will not work without configuring it with MC anyway. It >>>>> used to work in a very preliminary version of the csi2dc driver which I >>>>> sent a few years ago, but that way of handling things was rejected. >>>>> Hence I changed the csi2dc to being full-MC driver (requested for new >>>>> drivers) and now I am completing the conversion for the whole pipeline. >>>>> We are using this MC-centric approach in production for our products to >>>>> be as close as possible to mainline, and backported it to our 5.15 >>>>> internal releases, which people are using right now. >>>> >>>> I'm not all that keen on breaking userspace for those who do NOT use the >>>> Atmel BSP. Basically some platforms are currently broken, and with this patch >>>> series some other platforms are broken, but at least can be fixed by changing >>>> userspace. >>>> >>>> How feasible is it to do something similar that TI did for the cal driver? >>>> (drivers/media/platform/ti/cal) >>>> >>>> I.e., based on a module option the MC is enabled or disabled. And if a >>>> csi2dc is present, then the MC API is always enabled. >>>> >>> >>> I think I have suggested Eugen to move to MC when he >>> started looking in libcamera, so sorry for the intrusion but I feel >>> a bit bad for not rising the point earlier and get him to v10 >>> >>> I understand your point Hans, and when a vendor upstreaming code or a >>> user requires to maintain compatibility, the burden of keeping more >>> code in to handle the MC and non-MC cases is worth the complications. >> >> Eugen, can you provide a list of platforms that will break with this >> change and which platforms are currently broken without this series? Hi Hans, Basically the sama5d2 platform (we have several versions of the chip : sama5d21, sama5d27, sama5d29, in various packages, SIP, SoMs, on many different boards ) would be broken. this is the old platform. It would be broken if the sensor default format is a mismatch with the default format of the ISC . Basically the old code currently is propagating all the frame information down to the sensor, thing that no longer happens with this patch series. The platform that needs MC is mainly sama7g5 , which has a longer pipeline, supports CSI2 bus, and has more drivers (the csi2dc is one of them ), some are not mainlined. Future platforms , which are currently in prototyping, have a similar pipeline with sama7g5, some have more complicated pipelines, but they include the ISC and we plan to use the same driver. >> >> I'm trying to get a bit of a feel of the potential problems this change >> will introduce. >> >>> >>> But if even the vendor wants to move to MC to allow more use-cases I >>> think we have to acknolege that if you're running mainline on an >>> embedded system you could expect to adjust your setup between kernel >>> updates. The idea to document the media-ctl commands required to setup >>> the pipeline it's helpful, and might help in the interim period until >>> the platform is not supported by libcamera. >> >> Well, I don't want Linus to start yelling at me for breaking userspace :-) >> >> We have broken userspace API (intentionally) in the past, but only with >> good reasons. And sometimes a driver is used so rarely that it is not worth >> the effort to try and keep compatible. >> >> As a developer I'd love to just forget about the old API, but as subsystem >> maintainer I need good arguments. > > I understand and I think these are all valid concerns. Finding a > balance between new features and legacy is not easy. > >> >> Another option might be to take the TI cal approach, but have warnings that >> it will be removed in, say, 2 years time. Or even make a copy of the driver >> for the old platforms, and perhaps move that to staging to be removed eventually. >> >> The idea of a sudden breakage when going from kernel K to K+1 doesn't sit >> well with me, if there was a transition period of 1-2 years then that would be >> better. >> > > If staging works for you that's probably the easiest option. Let's see > what Eugen prefers! Hi Jacopo, How does the staging solution work ? I do not fully understand the options here to make an educated choice Thanks for helping out, Eugen > >> Regards, >> >> Hans >> >>> >>> That said, if Eugen wants to give the flag a try I won't >>> oppose :) >>> >>> >>>> Regards, >>>> >>>> Hans >>>> >>>>> >>>>>> >>>>>> media-ctl -d /dev/media0 --set-v4l2 '"imx219 >>>>>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]' >>>>>> media-ctl -d /dev/media0 --set-v4l2 >>>>>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]' >>>>>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]' >>>>>> media-ctl -d /dev/media0 --set-v4l2 >>>>>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]' >>>>>> >>>>>> Thank you for taking care of this ! >>>>>> >>>>>> Eugen >>>>>> >>>>>>> Regards, >>>>>>> >>>>>>> Hans >>>>>>> >>>>>>>> >>>>>>>> Full series history: >>>>>>>> >>>>>>>> Changes in v10: >>>>>>>> -> split the series into this first fixes part. >>>>>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes >>>>>>>> -> edited commit messages >>>>>>>> -> DT nodes now disabled by default. >>>>>>>> >>>>>>>> Changes in v9: >>>>>>>> -> kernel robot reported isc_link_validate is not static, changed to static. >>>>>>>> >>>>>>>> Changes in v8: >>>>>>>> -> scaler: modified crop bounds to have the exact source size >>>>>>>> >>>>>>>> Changes in v7: >>>>>>>> -> scaler: modified crop bounds to have maximum isc size >>>>>>>> -> format propagation: did small changes as per Jacopo review >>>>>>>> >>>>>>>> >>>>>>>> Changes in v6: >>>>>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review >>>>>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review >>>>>>>> >>>>>>>> Changes in v5: >>>>>>>> -> removed patch that removed the 'stop' variable as it was still required >>>>>>>> -> added two new trivial patches >>>>>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo >>>>>>>> >>>>>>>> >>>>>>>> Changes in v4: >>>>>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked >>>>>>>> one patch that was using it >>>>>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation >>>>>>>> >>>>>>>> >>>>>>>> Changes in v3: >>>>>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc >>>>>>>> for the isc-base. >>>>>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS >>>>>>>> >>>>>>>> Changes in v2: >>>>>>>> - integrated many changes suggested by Jacopo in the review of the v1 series. >>>>>>>> - add a few new patches >>>>>>>> >>>>>>>> Eugen Hristev (5): >>>>>>>> media: atmel: atmel-isc: prepare for media controller support >>>>>>>> media: atmel: atmel-isc: implement media controller >>>>>>>> ARM: dts: at91: sama7g5: add nodes for video capture >>>>>>>> ARM: configs: at91: sama7: add xisc and csi2dc >>>>>>>> ARM: multi_v7_defconfig: add atmel video pipeline modules >>>>>>>> >>>>>>>> arch/arm/boot/dts/sama7g5.dtsi | 51 ++ >>>>>>>> arch/arm/configs/multi_v7_defconfig | 3 + >>>>>>>> arch/arm/configs/sama7_defconfig | 2 + >>>>>>>> drivers/media/platform/atmel/Makefile | 2 +- >>>>>>>> drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- >>>>>>>> .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ >>>>>>>> drivers/media/platform/atmel/atmel-isc.h | 50 +- >>>>>>>> .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- >>>>>>>> .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- >>>>>>>> 9 files changed, 685 insertions(+), 241 deletions(-) >>>>>>>> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>
Hi Eugen, On Thu, Jun 23, 2022 at 08:39:48AM +0000, Eugen.Hristev@microchip.com wrote: > On 6/22/22 6:46 PM, Jacopo Mondi wrote: > > Hi Hans, > > > > On Wed, Jun 22, 2022 at 04:55:27PM +0200, Hans Verkuil wrote: > >> Hi Eugen, Jacopo, > >> > >> On 22/06/2022 16:14, Jacopo Mondi wrote: > >>> Hi Hans, Eugen > >>> > >>> On Wed, Jun 22, 2022 at 03:47:33PM +0200, Hans Verkuil wrote: > >>>> On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote: > >>>>> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote: > >>>>>> On 6/22/22 2:53 PM, Hans Verkuil wrote: > >>>>>>> Hi Eugen, > >>>>>>> > >>>>>>> On 03/05/2022 11:51, Eugen Hristev wrote: > >>>>>>>> This series is a split from the series : > >>>>>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller > >>>>>>>> and it includes the media controller part. > >>>>>>>> previous fixes were sent on a different patch series. > >>>>>>>> > >>>>>>>> As discussed on the ML, moving forward with having the media link validate at > >>>>>>>> start/stop streaming call. > >>>>>>>> I will test the patch : > >>>>>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops > >>>>>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks. > >>>>>>> > >>>>>>> I'm looking at merging this series, but I would like to have the output of > >>>>>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all > >>>>>>> correct. > >>>>>> > >>>>>> Hello Hans, > >>>>>> > >>>>>> Please have a look at attached file . Unless you want me to add the > >>>>>> whole output to the e-mail ? > >>>>>> > >>>>>> I also added output of media-ctl -p for your convenience. > >>>>>> the subdev2 is a device and driver that is not upstream and has some > >>>>>> compliance issues, they are reported by the v4l2-compliance tool, but > >>>>>> they should not affect this series, it's a synopsys driver that was > >>>>>> rejected on mainline a few years ago, I took it for internal usage, but > >>>>>> it's not cleaned up nor worked a lot upon. > >>>>>> > >>>>>>> > >>>>>>> And one more question which may have been answered already in the past: > >>>>>>> > >>>>>>> Changing to the MC will break existing applications, doesn't it? Or did I > >>>>>>> miss something? > >>>>>>> > >>>>>> > >>>>>> The existing applications will have to configure the pipeline now. It > >>>>>> will no longer work by configuring just the top video node /dev/video0 . > >>>>>> They would have to use media-ctl for it, something similar with this set > >>>>>> of commands: > >>>>> > >>>>> To add on top of that, actually, the reality is that without the MC > >>>>> support in atmel-isc , some of our platforms do not work at all, because > >>>>> the csi2dc driver which is in the middle of the pipeline, is a MC > >>>>> driver. So it will not work without configuring it with MC anyway. It > >>>>> used to work in a very preliminary version of the csi2dc driver which I > >>>>> sent a few years ago, but that way of handling things was rejected. > >>>>> Hence I changed the csi2dc to being full-MC driver (requested for new > >>>>> drivers) and now I am completing the conversion for the whole pipeline. > >>>>> We are using this MC-centric approach in production for our products to > >>>>> be as close as possible to mainline, and backported it to our 5.15 > >>>>> internal releases, which people are using right now. > >>>> > >>>> I'm not all that keen on breaking userspace for those who do NOT use the > >>>> Atmel BSP. Basically some platforms are currently broken, and with this patch > >>>> series some other platforms are broken, but at least can be fixed by changing > >>>> userspace. > >>>> > >>>> How feasible is it to do something similar that TI did for the cal driver? > >>>> (drivers/media/platform/ti/cal) > >>>> > >>>> I.e., based on a module option the MC is enabled or disabled. And if a > >>>> csi2dc is present, then the MC API is always enabled. > >>>> > >>> > >>> I think I have suggested Eugen to move to MC when he > >>> started looking in libcamera, so sorry for the intrusion but I feel > >>> a bit bad for not rising the point earlier and get him to v10 > >>> > >>> I understand your point Hans, and when a vendor upstreaming code or a > >>> user requires to maintain compatibility, the burden of keeping more > >>> code in to handle the MC and non-MC cases is worth the complications. > >> > >> Eugen, can you provide a list of platforms that will break with this > >> change and which platforms are currently broken without this series? > > Hi Hans, > > Basically the sama5d2 platform (we have several versions of the chip : > sama5d21, sama5d27, sama5d29, in various packages, SIP, SoMs, on many > different boards ) would be broken. this is the old platform. > It would be broken if the sensor default format is a mismatch with the > default format of the ISC . Basically the old code currently is > propagating all the frame information down to the sensor, thing that no > longer happens with this patch series. > > The platform that needs MC is mainly sama7g5 , which has a longer > pipeline, supports CSI2 bus, and has more drivers (the csi2dc is one of > them ), some are not mainlined. > Future platforms , which are currently in prototyping, have a similar > pipeline with sama7g5, some have more complicated pipelines, but they > include the ISC and we plan to use the same driver. > > >> > >> I'm trying to get a bit of a feel of the potential problems this change > >> will introduce. > >> > >>> > >>> But if even the vendor wants to move to MC to allow more use-cases I > >>> think we have to acknolege that if you're running mainline on an > >>> embedded system you could expect to adjust your setup between kernel > >>> updates. The idea to document the media-ctl commands required to setup > >>> the pipeline it's helpful, and might help in the interim period until > >>> the platform is not supported by libcamera. > >> > >> Well, I don't want Linus to start yelling at me for breaking userspace :-) > >> > >> We have broken userspace API (intentionally) in the past, but only with > >> good reasons. And sometimes a driver is used so rarely that it is not worth > >> the effort to try and keep compatible. > >> > >> As a developer I'd love to just forget about the old API, but as subsystem > >> maintainer I need good arguments. > > > > I understand and I think these are all valid concerns. Finding a > > balance between new features and legacy is not easy. > > > >> > >> Another option might be to take the TI cal approach, but have warnings that > >> it will be removed in, say, 2 years time. Or even make a copy of the driver > >> for the old platforms, and perhaps move that to staging to be removed eventually. > >> > >> The idea of a sudden breakage when going from kernel K to K+1 doesn't sit > >> well with me, if there was a transition period of 1-2 years then that would be > >> better. > >> > > > > If staging works for you that's probably the easiest option. Let's see > > what Eugen prefers! > > Hi Jacopo, > > How does the staging solution work ? I do not fully understand the > options here to make an educated choice Hans should probably tell, but my interepetation would be to move the existing driver (before this series) to drivers/staging/ and advance the existing one in drivers/media/ to MC support. Users of the old driver interface could keep using the one in (de)staging for a little longer. Would changing the driver KConfig symbol name help making the change more evident maybe ? Users that upgrade to a new kernel will be notified about the new symbol instead of being silently moved to the new interface. Thanks j > > Thanks for helping out, > > Eugen > > > > > >> Regards, > >> > >> Hans > >> > >>> > >>> That said, if Eugen wants to give the flag a try I won't > >>> oppose :) > >>> > >>> > >>>> Regards, > >>>> > >>>> Hans > >>>> > >>>>> > >>>>>> > >>>>>> media-ctl -d /dev/media0 --set-v4l2 '"imx219 > >>>>>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]' > >>>>>> media-ctl -d /dev/media0 --set-v4l2 > >>>>>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]' > >>>>>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]' > >>>>>> media-ctl -d /dev/media0 --set-v4l2 > >>>>>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]' > >>>>>> > >>>>>> Thank you for taking care of this ! > >>>>>> > >>>>>> Eugen > >>>>>> > >>>>>>> Regards, > >>>>>>> > >>>>>>> Hans > >>>>>>> > >>>>>>>> > >>>>>>>> Full series history: > >>>>>>>> > >>>>>>>> Changes in v10: > >>>>>>>> -> split the series into this first fixes part. > >>>>>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes > >>>>>>>> -> edited commit messages > >>>>>>>> -> DT nodes now disabled by default. > >>>>>>>> > >>>>>>>> Changes in v9: > >>>>>>>> -> kernel robot reported isc_link_validate is not static, changed to static. > >>>>>>>> > >>>>>>>> Changes in v8: > >>>>>>>> -> scaler: modified crop bounds to have the exact source size > >>>>>>>> > >>>>>>>> Changes in v7: > >>>>>>>> -> scaler: modified crop bounds to have maximum isc size > >>>>>>>> -> format propagation: did small changes as per Jacopo review > >>>>>>>> > >>>>>>>> > >>>>>>>> Changes in v6: > >>>>>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review > >>>>>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review > >>>>>>>> > >>>>>>>> Changes in v5: > >>>>>>>> -> removed patch that removed the 'stop' variable as it was still required > >>>>>>>> -> added two new trivial patches > >>>>>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo > >>>>>>>> > >>>>>>>> > >>>>>>>> Changes in v4: > >>>>>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked > >>>>>>>> one patch that was using it > >>>>>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation > >>>>>>>> > >>>>>>>> > >>>>>>>> Changes in v3: > >>>>>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc > >>>>>>>> for the isc-base. > >>>>>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS > >>>>>>>> > >>>>>>>> Changes in v2: > >>>>>>>> - integrated many changes suggested by Jacopo in the review of the v1 series. > >>>>>>>> - add a few new patches > >>>>>>>> > >>>>>>>> Eugen Hristev (5): > >>>>>>>> media: atmel: atmel-isc: prepare for media controller support > >>>>>>>> media: atmel: atmel-isc: implement media controller > >>>>>>>> ARM: dts: at91: sama7g5: add nodes for video capture > >>>>>>>> ARM: configs: at91: sama7: add xisc and csi2dc > >>>>>>>> ARM: multi_v7_defconfig: add atmel video pipeline modules > >>>>>>>> > >>>>>>>> arch/arm/boot/dts/sama7g5.dtsi | 51 ++ > >>>>>>>> arch/arm/configs/multi_v7_defconfig | 3 + > >>>>>>>> arch/arm/configs/sama7_defconfig | 2 + > >>>>>>>> drivers/media/platform/atmel/Makefile | 2 +- > >>>>>>>> drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- > >>>>>>>> .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ > >>>>>>>> drivers/media/platform/atmel/atmel-isc.h | 50 +- > >>>>>>>> .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- > >>>>>>>> .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- > >>>>>>>> 9 files changed, 685 insertions(+), 241 deletions(-) > >>>>>>>> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >> >
On 23/06/2022 11:19, Jacopo Mondi wrote: > Hi Eugen, > > On Thu, Jun 23, 2022 at 08:39:48AM +0000, Eugen.Hristev@microchip.com wrote: >> On 6/22/22 6:46 PM, Jacopo Mondi wrote: >>> Hi Hans, >>> >>> On Wed, Jun 22, 2022 at 04:55:27PM +0200, Hans Verkuil wrote: >>>> Hi Eugen, Jacopo, >>>> >>>> On 22/06/2022 16:14, Jacopo Mondi wrote: >>>>> Hi Hans, Eugen >>>>> >>>>> On Wed, Jun 22, 2022 at 03:47:33PM +0200, Hans Verkuil wrote: >>>>>> On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote: >>>>>>> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote: >>>>>>>> On 6/22/22 2:53 PM, Hans Verkuil wrote: >>>>>>>>> Hi Eugen, >>>>>>>>> >>>>>>>>> On 03/05/2022 11:51, Eugen Hristev wrote: >>>>>>>>>> This series is a split from the series : >>>>>>>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller >>>>>>>>>> and it includes the media controller part. >>>>>>>>>> previous fixes were sent on a different patch series. >>>>>>>>>> >>>>>>>>>> As discussed on the ML, moving forward with having the media link validate at >>>>>>>>>> start/stop streaming call. >>>>>>>>>> I will test the patch : >>>>>>>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops >>>>>>>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks. >>>>>>>>> >>>>>>>>> I'm looking at merging this series, but I would like to have the output of >>>>>>>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all >>>>>>>>> correct. >>>>>>>> >>>>>>>> Hello Hans, >>>>>>>> >>>>>>>> Please have a look at attached file . Unless you want me to add the >>>>>>>> whole output to the e-mail ? >>>>>>>> >>>>>>>> I also added output of media-ctl -p for your convenience. >>>>>>>> the subdev2 is a device and driver that is not upstream and has some >>>>>>>> compliance issues, they are reported by the v4l2-compliance tool, but >>>>>>>> they should not affect this series, it's a synopsys driver that was >>>>>>>> rejected on mainline a few years ago, I took it for internal usage, but >>>>>>>> it's not cleaned up nor worked a lot upon. >>>>>>>> >>>>>>>>> >>>>>>>>> And one more question which may have been answered already in the past: >>>>>>>>> >>>>>>>>> Changing to the MC will break existing applications, doesn't it? Or did I >>>>>>>>> miss something? >>>>>>>>> >>>>>>>> >>>>>>>> The existing applications will have to configure the pipeline now. It >>>>>>>> will no longer work by configuring just the top video node /dev/video0 . >>>>>>>> They would have to use media-ctl for it, something similar with this set >>>>>>>> of commands: >>>>>>> >>>>>>> To add on top of that, actually, the reality is that without the MC >>>>>>> support in atmel-isc , some of our platforms do not work at all, because >>>>>>> the csi2dc driver which is in the middle of the pipeline, is a MC >>>>>>> driver. So it will not work without configuring it with MC anyway. It >>>>>>> used to work in a very preliminary version of the csi2dc driver which I >>>>>>> sent a few years ago, but that way of handling things was rejected. >>>>>>> Hence I changed the csi2dc to being full-MC driver (requested for new >>>>>>> drivers) and now I am completing the conversion for the whole pipeline. >>>>>>> We are using this MC-centric approach in production for our products to >>>>>>> be as close as possible to mainline, and backported it to our 5.15 >>>>>>> internal releases, which people are using right now. >>>>>> >>>>>> I'm not all that keen on breaking userspace for those who do NOT use the >>>>>> Atmel BSP. Basically some platforms are currently broken, and with this patch >>>>>> series some other platforms are broken, but at least can be fixed by changing >>>>>> userspace. >>>>>> >>>>>> How feasible is it to do something similar that TI did for the cal driver? >>>>>> (drivers/media/platform/ti/cal) >>>>>> >>>>>> I.e., based on a module option the MC is enabled or disabled. And if a >>>>>> csi2dc is present, then the MC API is always enabled. >>>>>> >>>>> >>>>> I think I have suggested Eugen to move to MC when he >>>>> started looking in libcamera, so sorry for the intrusion but I feel >>>>> a bit bad for not rising the point earlier and get him to v10 >>>>> >>>>> I understand your point Hans, and when a vendor upstreaming code or a >>>>> user requires to maintain compatibility, the burden of keeping more >>>>> code in to handle the MC and non-MC cases is worth the complications. >>>> >>>> Eugen, can you provide a list of platforms that will break with this >>>> change and which platforms are currently broken without this series? >> >> Hi Hans, >> >> Basically the sama5d2 platform (we have several versions of the chip : >> sama5d21, sama5d27, sama5d29, in various packages, SIP, SoMs, on many >> different boards ) would be broken. this is the old platform. >> It would be broken if the sensor default format is a mismatch with the >> default format of the ISC . Basically the old code currently is >> propagating all the frame information down to the sensor, thing that no >> longer happens with this patch series. >> >> The platform that needs MC is mainly sama7g5 , which has a longer >> pipeline, supports CSI2 bus, and has more drivers (the csi2dc is one of >> them ), some are not mainlined. >> Future platforms , which are currently in prototyping, have a similar >> pipeline with sama7g5, some have more complicated pipelines, but they >> include the ISC and we plan to use the same driver. >> >>>> >>>> I'm trying to get a bit of a feel of the potential problems this change >>>> will introduce. >>>> >>>>> >>>>> But if even the vendor wants to move to MC to allow more use-cases I >>>>> think we have to acknolege that if you're running mainline on an >>>>> embedded system you could expect to adjust your setup between kernel >>>>> updates. The idea to document the media-ctl commands required to setup >>>>> the pipeline it's helpful, and might help in the interim period until >>>>> the platform is not supported by libcamera. >>>> >>>> Well, I don't want Linus to start yelling at me for breaking userspace :-) >>>> >>>> We have broken userspace API (intentionally) in the past, but only with >>>> good reasons. And sometimes a driver is used so rarely that it is not worth >>>> the effort to try and keep compatible. >>>> >>>> As a developer I'd love to just forget about the old API, but as subsystem >>>> maintainer I need good arguments. >>> >>> I understand and I think these are all valid concerns. Finding a >>> balance between new features and legacy is not easy. >>> >>>> >>>> Another option might be to take the TI cal approach, but have warnings that >>>> it will be removed in, say, 2 years time. Or even make a copy of the driver >>>> for the old platforms, and perhaps move that to staging to be removed eventually. >>>> >>>> The idea of a sudden breakage when going from kernel K to K+1 doesn't sit >>>> well with me, if there was a transition period of 1-2 years then that would be >>>> better. >>>> >>> >>> If staging works for you that's probably the easiest option. Let's see >>> what Eugen prefers! >> >> Hi Jacopo, >> >> How does the staging solution work ? I do not fully understand the >> options here to make an educated choice > > Hans should probably tell, but my interepetation would be to move the > existing driver (before this series) to drivers/staging/ and advance > the existing one in drivers/media/ to MC support. Right. And strip the support for the newer platforms from the staging driver. So it is just for sama5d2. > > Users of the old driver interface could keep using the one in (de)staging > for a little longer. > > Would changing the driver KConfig symbol name help making the change > more evident maybe ? Users that upgrade to a new kernel will be > notified about the new symbol instead of being silently moved to the new > interface. I'm inclined to change the Kconfig symbol for both old and new drivers if we decide to go in this direction: in both cases you need to be aware that there are major changes: the new uses the MC API, the old is marked deprecated and users should be aware that it will be removed eventually and they should work to switch to the 'new' driver. Regards, Hans > > Thanks > j > >> >> Thanks for helping out, >> >> Eugen >> >> >>> >>>> Regards, >>>> >>>> Hans >>>> >>>>> >>>>> That said, if Eugen wants to give the flag a try I won't >>>>> oppose :) >>>>> >>>>> >>>>>> Regards, >>>>>> >>>>>> Hans >>>>>> >>>>>>> >>>>>>>> >>>>>>>> media-ctl -d /dev/media0 --set-v4l2 '"imx219 >>>>>>>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]' >>>>>>>> media-ctl -d /dev/media0 --set-v4l2 >>>>>>>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]' >>>>>>>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]' >>>>>>>> media-ctl -d /dev/media0 --set-v4l2 >>>>>>>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]' >>>>>>>> >>>>>>>> Thank you for taking care of this ! >>>>>>>> >>>>>>>> Eugen >>>>>>>> >>>>>>>>> Regards, >>>>>>>>> >>>>>>>>> Hans >>>>>>>>> >>>>>>>>>> >>>>>>>>>> Full series history: >>>>>>>>>> >>>>>>>>>> Changes in v10: >>>>>>>>>> -> split the series into this first fixes part. >>>>>>>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes >>>>>>>>>> -> edited commit messages >>>>>>>>>> -> DT nodes now disabled by default. >>>>>>>>>> >>>>>>>>>> Changes in v9: >>>>>>>>>> -> kernel robot reported isc_link_validate is not static, changed to static. >>>>>>>>>> >>>>>>>>>> Changes in v8: >>>>>>>>>> -> scaler: modified crop bounds to have the exact source size >>>>>>>>>> >>>>>>>>>> Changes in v7: >>>>>>>>>> -> scaler: modified crop bounds to have maximum isc size >>>>>>>>>> -> format propagation: did small changes as per Jacopo review >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Changes in v6: >>>>>>>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review >>>>>>>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review >>>>>>>>>> >>>>>>>>>> Changes in v5: >>>>>>>>>> -> removed patch that removed the 'stop' variable as it was still required >>>>>>>>>> -> added two new trivial patches >>>>>>>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Changes in v4: >>>>>>>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked >>>>>>>>>> one patch that was using it >>>>>>>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Changes in v3: >>>>>>>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc >>>>>>>>>> for the isc-base. >>>>>>>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS >>>>>>>>>> >>>>>>>>>> Changes in v2: >>>>>>>>>> - integrated many changes suggested by Jacopo in the review of the v1 series. >>>>>>>>>> - add a few new patches >>>>>>>>>> >>>>>>>>>> Eugen Hristev (5): >>>>>>>>>> media: atmel: atmel-isc: prepare for media controller support >>>>>>>>>> media: atmel: atmel-isc: implement media controller >>>>>>>>>> ARM: dts: at91: sama7g5: add nodes for video capture >>>>>>>>>> ARM: configs: at91: sama7: add xisc and csi2dc >>>>>>>>>> ARM: multi_v7_defconfig: add atmel video pipeline modules >>>>>>>>>> >>>>>>>>>> arch/arm/boot/dts/sama7g5.dtsi | 51 ++ >>>>>>>>>> arch/arm/configs/multi_v7_defconfig | 3 + >>>>>>>>>> arch/arm/configs/sama7_defconfig | 2 + >>>>>>>>>> drivers/media/platform/atmel/Makefile | 2 +- >>>>>>>>>> drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++--------- >>>>>>>>>> .../media/platform/atmel/atmel-isc-scaler.c | 267 ++++++++++ >>>>>>>>>> drivers/media/platform/atmel/atmel-isc.h | 50 +- >>>>>>>>>> .../media/platform/atmel/atmel-sama5d2-isc.c | 34 +- >>>>>>>>>> .../media/platform/atmel/atmel-sama7g5-isc.c | 32 +- >>>>>>>>>> 9 files changed, 685 insertions(+), 241 deletions(-) >>>>>>>>>> create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>> >>