mbox series

[RFC,V4,0/4] media: platform: Add support for Face Detection (FD) on mt8183 SoC

Message ID 20191204124732.10932-1-Jerry-Ch.chen@mediatek.com (mailing list archive)
Headers show
Series media: platform: Add support for Face Detection (FD) on mt8183 SoC | expand

Message

Jerry-ch Chen Dec. 4, 2019, 12:47 p.m. UTC
Hello,

This RFC patch series is adding Face Detection (FD) driver on Mediatek
mt8183 SoC. It belongs to the first Mediatek's camera driver series based
on V4L2 and media controller framework. I posted the main part of the FD
driver as RFC to discuss first and would like some review comments.

==============
Introduction
==============

Face Detection (FD) unit provides hardware accelerated face detection
feature. It can detect different sizes of faces in a given image.

The driver is implemented as a normal V4L2 memory-to-memory device and
supports V4L2 controls for detection settings. It has two buffer queues.

1. Video output buffer: RAW image for face detection.

2. Meta capture buffer: Result of the detected faces.

==================
Changes in v4
==================

RFC v4 includes the following modification:
1. Includes v4l2-mem2mem changes: add v4l2_m2m_suspend, v4l2_m2m_resume

2. Move FD V4L2 control ids back inside FD driver

3. Depend on newer SCP driver api

4. Add exit hw flow for FD driver

5. Add hardware timeout handling in the FD firmware

6. Move allocation of dma buffer from connect / disconnect to driver probe
/ remove

Todo:
 - Add v4l2 control menus for private mtk fd control
==================
Changes in v3
==================

RFC v3 includes the following modification:
1. Adjust the private control ids and place them in
 include/uapi/linux/mtk-fd-v4l2-controls.h

2. Merge struct mtk_fd_hw info struct mtk_fd_dev

3. Define FD meta capture buffer in include/uapi/linux/videodev2.h

4. Remove the usage of get_reserved_memory by scp driver,
 use dma_alloc api instead

Todo:
 - Add v4l2 control menus for private mtk fd control
 - Refine the job finish flow when system suspend
==================
Changes in v2
==================

RFC v2 includes the following modification:
1. Implement FD as a V4L2 mem2mem driver

2. Replace meta input with V4L2 controls

==================
Changes in v1
==================

RFC v1 includes the following modification:
1. Uses Request API instead of FD's buffer collection design

2. removed unnecessary abstraction structurally, including mtk_fd_ctx and
related ops

3. removed the fd_smem node from device tree

4. Fixed the common issues Tomasz commented on Mediatek ISP Pass 1's RFC v0
patch series

==================
Dependent patch
==================

FD driver depends on SCP driver. The patches are as following:

[1]. Add support for mt8183 SCP
https://patchwork.kernel.org/cover/1152350/

==================
Compliance test
==================

* Version: https://git.linuxtv.org/v4l-utils.git/commit/?id=b16f9e945d74aa5
* Note: Some failures are caused by the implementation of FD driver,
        whic is a m2m device with VIDEO_OUT and META_CAPTURE queues,
        therefore we can't set V4L2_CAP_VIDEO_M2M in device capability, and
        fail in some non-m2m v4l2 test cases.
* Test command: v4l2-compliance -m 2
* test output:

v4l2-compliance SHA: not available, 32 bits

Compliance test for mtk-fd-4.0 device /dev/media2:

Media Driver Info:
        Driver name      : mtk-fd-4.0
        Model            : mtk-fd-4.0
        Serial           :
        Bus info         : platform:1502b000.fd
        Media version    : 4.19.84
        Hardware revision: 0x00000000 (0)
        Driver version   : 4.19.84

Required ioctls:
        test MEDIA_IOC_DEVICE_INFO: OK

Allow for multiple opens:
        test second /dev/media2 open: OK
        test MEDIA_IOC_DEVICE_INFO: OK
        test for unlimited opens: OK

Media Controller ioctls:
        test MEDIA_IOC_G_TOPOLOGY: OK
        Entities: 3 Interfaces: 1 Pads: 4 Links: 4
        test MEDIA_IOC_ENUM_ENTITIES/LINKS: OK
        test MEDIA_IOC_SETUP_LINK: OK

Total for mtk-fd-4.0 device /dev/media2: 7, Succeeded: 7, Failed: 0, Warnings: 0
--------------------------------------------------------------------------------
Compliance test for mtk-fd-4.0 device /dev/video32:

Driver Info:
        Driver name      : mtk-fd-4.0
        Card type        : mtk-fd-4.0
        Bus info         : platform:1502b000.fd
        Driver version   : 4.19.84
        Capabilities     : 0x84a02000
                Video Output Multiplanar
                Metadata Capture
                Streaming
                Extended Pix Format
                Device Capabilities
        Device Caps      : 0x04a02000
                Video Output Multiplanar
                Metadata Capture
                Streaming
                Extended Pix Format
Media Driver Info:
        Driver name      : mtk-fd-4.0
        Model            : mtk-fd-4.0
        Serial           :
        Bus info         : platform:1502b000.fd
        Media version    : 4.19.84
        Hardware revision: 0x00000000 (0)
        Driver version   : 4.19.84
Interface Info:
        ID               : 0x0300000c
        Type             : V4L Video
Entity Info:
        ID               : 0x00000001 (1)
        Name             : mtk-fd-4.0-source
        Function         : V4L2 I/O
        Pad 0x01000002   : 0: Source
          Link 0x02000008: to remote pad 0x1000005 of entity 'mtk-fd-4.0-proc': Data, Enabled, Immutable

Required ioctls:
        test MC information (see 'Media Driver Info' above): OK
                fail: v4l2-compliance.cpp(668): dcaps & output_caps
        test VIDIOC_QUERYCAP: FAIL

Allow for multiple opens:
        test second /dev/video32 open: OK
                fail: v4l2-compliance.cpp(668): dcaps & output_caps
        test VIDIOC_QUERYCAP: FAIL
        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 (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: 1 Private Controls: 6

Format ioctls:
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
        test VIDIOC_G/S_PARM: OK (Not Supported)
        test VIDIOC_G_FBUF: OK (Not Supported)
                fail: v4l2-test-formats.cpp(457): pixelformat 00000000 () for buftype 10 not reported by ENUM_FMT
        test VIDIOC_G_FMT: FAIL
                fail: v4l2-test-formats.cpp(457): pixelformat 00000000 () for buftype 10 not reported by ENUM_FMT
        test VIDIOC_TRY_FMT: FAIL
                fail: v4l2-test-formats.cpp(457): pixelformat ffffffff (-BE) for buftype 10 not reported by ENUM_FMT
        test VIDIOC_S_FMT: FAIL
        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:
                fail: v4l2-test-buffers.cpp(667): q2.reqbufs(node->node2, 1) != EBUSY
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL
        test VIDIOC_EXPBUF: OK
        test Requests: OK

Total for mtk-fd-4.0 device /dev/video32: 45, Succeeded: 39, Failed: 6, Warnings: 0

Grand Total for mtk-fd-4.0 device /dev/media2: 52, Succeeded: 46, Failed: 6, Warnings: 0

Comments

Jerry-ch Chen May 8, 2020, 2:02 a.m. UTC | #1
Hi Laurent, Tomasz, Matthias,

gentle ping for this patch set,
If no new comments, I would like to send a newer version.

Thanks and Best Regards,
Jerry

On Wed, 2019-12-04 at 20:47 +0800, Jerry-ch Chen wrote:
> Hello,
> 
> This RFC patch series is adding Face Detection (FD) driver on Mediatek
> mt8183 SoC. It belongs to the first Mediatek's camera driver series based
> on V4L2 and media controller framework. I posted the main part of the FD
> driver as RFC to discuss first and would like some review comments.
> 
> ==============
> Introduction
> ==============
> 
> Face Detection (FD) unit provides hardware accelerated face detection
> feature. It can detect different sizes of faces in a given image.
> 
> The driver is implemented as a normal V4L2 memory-to-memory device and
> supports V4L2 controls for detection settings. It has two buffer queues.
> 
> 1. Video output buffer: RAW image for face detection.
> 
> 2. Meta capture buffer: Result of the detected faces.
> 
> ==================
> Changes in v4
> ==================
> 
> RFC v4 includes the following modification:
> 1. Includes v4l2-mem2mem changes: add v4l2_m2m_suspend, v4l2_m2m_resume
> 
> 2. Move FD V4L2 control ids back inside FD driver
> 
> 3. Depend on newer SCP driver api
> 
> 4. Add exit hw flow for FD driver
> 
> 5. Add hardware timeout handling in the FD firmware
> 
> 6. Move allocation of dma buffer from connect / disconnect to driver probe
> / remove
> 
> Todo:
>  - Add v4l2 control menus for private mtk fd control
> ==================
> Changes in v3
> ==================
> 
> RFC v3 includes the following modification:
> 1. Adjust the private control ids and place them in
>  include/uapi/linux/mtk-fd-v4l2-controls.h
> 
> 2. Merge struct mtk_fd_hw info struct mtk_fd_dev
> 
> 3. Define FD meta capture buffer in include/uapi/linux/videodev2.h
> 
> 4. Remove the usage of get_reserved_memory by scp driver,
>  use dma_alloc api instead
> 
> Todo:
>  - Add v4l2 control menus for private mtk fd control
>  - Refine the job finish flow when system suspend
> ==================
> Changes in v2
> ==================
> 
> RFC v2 includes the following modification:
> 1. Implement FD as a V4L2 mem2mem driver
> 
> 2. Replace meta input with V4L2 controls
> 
> ==================
> Changes in v1
> ==================
> 
> RFC v1 includes the following modification:
> 1. Uses Request API instead of FD's buffer collection design
> 
> 2. removed unnecessary abstraction structurally, including mtk_fd_ctx and
> related ops
> 
> 3. removed the fd_smem node from device tree
> 
> 4. Fixed the common issues Tomasz commented on Mediatek ISP Pass 1's RFC v0
> patch series
> 
> ==================
> Dependent patch
> ==================
> 
> FD driver depends on SCP driver. The patches are as following:
> 
> [1]. Add support for mt8183 SCP
> https://patchwork.kernel.org/cover/1152350/
> 
> ==================
> Compliance test
> ==================
> 
> * Version: https://git.linuxtv.org/v4l-utils.git/commit/?id=b16f9e945d74aa5
> * Note: Some failures are caused by the implementation of FD driver,
>         whic is a m2m device with VIDEO_OUT and META_CAPTURE queues,
>         therefore we can't set V4L2_CAP_VIDEO_M2M in device capability, and
>         fail in some non-m2m v4l2 test cases.
> * Test command: v4l2-compliance -m 2
> * test output:
> 
> v4l2-compliance SHA: not available, 32 bits
> 
> Compliance test for mtk-fd-4.0 device /dev/media2:
> 
> Media Driver Info:
>         Driver name      : mtk-fd-4.0
>         Model            : mtk-fd-4.0
>         Serial           :
>         Bus info         : platform:1502b000.fd
>         Media version    : 4.19.84
>         Hardware revision: 0x00000000 (0)
>         Driver version   : 4.19.84
> 
> Required ioctls:
>         test MEDIA_IOC_DEVICE_INFO: OK
> 
> Allow for multiple opens:
>         test second /dev/media2 open: OK
>         test MEDIA_IOC_DEVICE_INFO: OK
>         test for unlimited opens: OK
> 
> Media Controller ioctls:
>         test MEDIA_IOC_G_TOPOLOGY: OK
>         Entities: 3 Interfaces: 1 Pads: 4 Links: 4
>         test MEDIA_IOC_ENUM_ENTITIES/LINKS: OK
>         test MEDIA_IOC_SETUP_LINK: OK
> 
> Total for mtk-fd-4.0 device /dev/media2: 7, Succeeded: 7, Failed: 0, Warnings: 0
> --------------------------------------------------------------------------------
> Compliance test for mtk-fd-4.0 device /dev/video32:
> 
> Driver Info:
>         Driver name      : mtk-fd-4.0
>         Card type        : mtk-fd-4.0
>         Bus info         : platform:1502b000.fd
>         Driver version   : 4.19.84
>         Capabilities     : 0x84a02000
>                 Video Output Multiplanar
>                 Metadata Capture
>                 Streaming
>                 Extended Pix Format
>                 Device Capabilities
>         Device Caps      : 0x04a02000
>                 Video Output Multiplanar
>                 Metadata Capture
>                 Streaming
>                 Extended Pix Format
> Media Driver Info:
>         Driver name      : mtk-fd-4.0
>         Model            : mtk-fd-4.0
>         Serial           :
>         Bus info         : platform:1502b000.fd
>         Media version    : 4.19.84
>         Hardware revision: 0x00000000 (0)
>         Driver version   : 4.19.84
> Interface Info:
>         ID               : 0x0300000c
>         Type             : V4L Video
> Entity Info:
>         ID               : 0x00000001 (1)
>         Name             : mtk-fd-4.0-source
>         Function         : V4L2 I/O
>         Pad 0x01000002   : 0: Source
>           Link 0x02000008: to remote pad 0x1000005 of entity 'mtk-fd-4.0-proc': Data, Enabled, Immutable
> 
> Required ioctls:
>         test MC information (see 'Media Driver Info' above): OK
>                 fail: v4l2-compliance.cpp(668): dcaps & output_caps
>         test VIDIOC_QUERYCAP: FAIL
> 
> Allow for multiple opens:
>         test second /dev/video32 open: OK
>                 fail: v4l2-compliance.cpp(668): dcaps & output_caps
>         test VIDIOC_QUERYCAP: FAIL
>         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 (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: 1 Private Controls: 6
> 
> Format ioctls:
>         test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
>         test VIDIOC_G/S_PARM: OK (Not Supported)
>         test VIDIOC_G_FBUF: OK (Not Supported)
>                 fail: v4l2-test-formats.cpp(457): pixelformat 00000000 () for buftype 10 not reported by ENUM_FMT
>         test VIDIOC_G_FMT: FAIL
>                 fail: v4l2-test-formats.cpp(457): pixelformat 00000000 () for buftype 10 not reported by ENUM_FMT
>         test VIDIOC_TRY_FMT: FAIL
>                 fail: v4l2-test-formats.cpp(457): pixelformat ffffffff (-BE) for buftype 10 not reported by ENUM_FMT
>         test VIDIOC_S_FMT: FAIL
>         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:
>                 fail: v4l2-test-buffers.cpp(667): q2.reqbufs(node->node2, 1) != EBUSY
>         test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL
>         test VIDIOC_EXPBUF: OK
>         test Requests: OK
> 
> Total for mtk-fd-4.0 device /dev/video32: 45, Succeeded: 39, Failed: 6, Warnings: 0
> 
> Grand Total for mtk-fd-4.0 device /dev/media2: 52, Succeeded: 46, Failed: 6, Warnings: 0
>
Tomasz Figa May 13, 2020, 9:45 p.m. UTC | #2
Hi Jerry,

On Fri, May 8, 2020 at 4:03 AM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
>
> Hi Laurent, Tomasz, Matthias,
>
> gentle ping for this patch set,
> If no new comments, I would like to send a newer version.
>

Sorry, I still haven't had a chance to look at the series, so feel
free to send a new version and I will take a look at the new one.

Best regards,
Tomasz

> Thanks and Best Regards,
> Jerry
>
> On Wed, 2019-12-04 at 20:47 +0800, Jerry-ch Chen wrote:
> > Hello,
> >
> > This RFC patch series is adding Face Detection (FD) driver on Mediatek
> > mt8183 SoC. It belongs to the first Mediatek's camera driver series based
> > on V4L2 and media controller framework. I posted the main part of the FD
> > driver as RFC to discuss first and would like some review comments.
> >
> > ==============
> > Introduction
> > ==============
> >
> > Face Detection (FD) unit provides hardware accelerated face detection
> > feature. It can detect different sizes of faces in a given image.
> >
> > The driver is implemented as a normal V4L2 memory-to-memory device and
> > supports V4L2 controls for detection settings. It has two buffer queues.
> >
> > 1. Video output buffer: RAW image for face detection.
> >
> > 2. Meta capture buffer: Result of the detected faces.
> >
> > ==================
> > Changes in v4
> > ==================
> >
> > RFC v4 includes the following modification:
> > 1. Includes v4l2-mem2mem changes: add v4l2_m2m_suspend, v4l2_m2m_resume
> >
> > 2. Move FD V4L2 control ids back inside FD driver
> >
> > 3. Depend on newer SCP driver api
> >
> > 4. Add exit hw flow for FD driver
> >
> > 5. Add hardware timeout handling in the FD firmware
> >
> > 6. Move allocation of dma buffer from connect / disconnect to driver probe
> > / remove
> >
> > Todo:
> >  - Add v4l2 control menus for private mtk fd control
> > ==================
> > Changes in v3
> > ==================
> >
> > RFC v3 includes the following modification:
> > 1. Adjust the private control ids and place them in
> >  include/uapi/linux/mtk-fd-v4l2-controls.h
> >
> > 2. Merge struct mtk_fd_hw info struct mtk_fd_dev
> >
> > 3. Define FD meta capture buffer in include/uapi/linux/videodev2.h
> >
> > 4. Remove the usage of get_reserved_memory by scp driver,
> >  use dma_alloc api instead
> >
> > Todo:
> >  - Add v4l2 control menus for private mtk fd control
> >  - Refine the job finish flow when system suspend
> > ==================
> > Changes in v2
> > ==================
> >
> > RFC v2 includes the following modification:
> > 1. Implement FD as a V4L2 mem2mem driver
> >
> > 2. Replace meta input with V4L2 controls
> >
> > ==================
> > Changes in v1
> > ==================
> >
> > RFC v1 includes the following modification:
> > 1. Uses Request API instead of FD's buffer collection design
> >
> > 2. removed unnecessary abstraction structurally, including mtk_fd_ctx and
> > related ops
> >
> > 3. removed the fd_smem node from device tree
> >
> > 4. Fixed the common issues Tomasz commented on Mediatek ISP Pass 1's RFC v0
> > patch series
> >
> > ==================
> > Dependent patch
> > ==================
> >
> > FD driver depends on SCP driver. The patches are as following:
> >
> > [1]. Add support for mt8183 SCP
> > https://patchwork.kernel.org/cover/1152350/
> >
> > ==================
> > Compliance test
> > ==================
> >
> > * Version: https://git.linuxtv.org/v4l-utils.git/commit/?id=b16f9e945d74aa5
> > * Note: Some failures are caused by the implementation of FD driver,
> >         whic is a m2m device with VIDEO_OUT and META_CAPTURE queues,
> >         therefore we can't set V4L2_CAP_VIDEO_M2M in device capability, and
> >         fail in some non-m2m v4l2 test cases.
> > * Test command: v4l2-compliance -m 2
> > * test output:
> >
> > v4l2-compliance SHA: not available, 32 bits
> >
> > Compliance test for mtk-fd-4.0 device /dev/media2:
> >
> > Media Driver Info:
> >         Driver name      : mtk-fd-4.0
> >         Model            : mtk-fd-4.0
> >         Serial           :
> >         Bus info         : platform:1502b000.fd
> >         Media version    : 4.19.84
> >         Hardware revision: 0x00000000 (0)
> >         Driver version   : 4.19.84
> >
> > Required ioctls:
> >         test MEDIA_IOC_DEVICE_INFO: OK
> >
> > Allow for multiple opens:
> >         test second /dev/media2 open: OK
> >         test MEDIA_IOC_DEVICE_INFO: OK
> >         test for unlimited opens: OK
> >
> > Media Controller ioctls:
> >         test MEDIA_IOC_G_TOPOLOGY: OK
> >         Entities: 3 Interfaces: 1 Pads: 4 Links: 4
> >         test MEDIA_IOC_ENUM_ENTITIES/LINKS: OK
> >         test MEDIA_IOC_SETUP_LINK: OK
> >
> > Total for mtk-fd-4.0 device /dev/media2: 7, Succeeded: 7, Failed: 0, Warnings: 0
> > --------------------------------------------------------------------------------
> > Compliance test for mtk-fd-4.0 device /dev/video32:
> >
> > Driver Info:
> >         Driver name      : mtk-fd-4.0
> >         Card type        : mtk-fd-4.0
> >         Bus info         : platform:1502b000.fd
> >         Driver version   : 4.19.84
> >         Capabilities     : 0x84a02000
> >                 Video Output Multiplanar
> >                 Metadata Capture
> >                 Streaming
> >                 Extended Pix Format
> >                 Device Capabilities
> >         Device Caps      : 0x04a02000
> >                 Video Output Multiplanar
> >                 Metadata Capture
> >                 Streaming
> >                 Extended Pix Format
> > Media Driver Info:
> >         Driver name      : mtk-fd-4.0
> >         Model            : mtk-fd-4.0
> >         Serial           :
> >         Bus info         : platform:1502b000.fd
> >         Media version    : 4.19.84
> >         Hardware revision: 0x00000000 (0)
> >         Driver version   : 4.19.84
> > Interface Info:
> >         ID               : 0x0300000c
> >         Type             : V4L Video
> > Entity Info:
> >         ID               : 0x00000001 (1)
> >         Name             : mtk-fd-4.0-source
> >         Function         : V4L2 I/O
> >         Pad 0x01000002   : 0: Source
> >           Link 0x02000008: to remote pad 0x1000005 of entity 'mtk-fd-4.0-proc': Data, Enabled, Immutable
> >
> > Required ioctls:
> >         test MC information (see 'Media Driver Info' above): OK
> >                 fail: v4l2-compliance.cpp(668): dcaps & output_caps
> >         test VIDIOC_QUERYCAP: FAIL
> >
> > Allow for multiple opens:
> >         test second /dev/video32 open: OK
> >                 fail: v4l2-compliance.cpp(668): dcaps & output_caps
> >         test VIDIOC_QUERYCAP: FAIL
> >         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 (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: 1 Private Controls: 6
> >
> > Format ioctls:
> >         test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> >         test VIDIOC_G/S_PARM: OK (Not Supported)
> >         test VIDIOC_G_FBUF: OK (Not Supported)
> >                 fail: v4l2-test-formats.cpp(457): pixelformat 00000000 () for buftype 10 not reported by ENUM_FMT
> >         test VIDIOC_G_FMT: FAIL
> >                 fail: v4l2-test-formats.cpp(457): pixelformat 00000000 () for buftype 10 not reported by ENUM_FMT
> >         test VIDIOC_TRY_FMT: FAIL
> >                 fail: v4l2-test-formats.cpp(457): pixelformat ffffffff (-BE) for buftype 10 not reported by ENUM_FMT
> >         test VIDIOC_S_FMT: FAIL
> >         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:
> >                 fail: v4l2-test-buffers.cpp(667): q2.reqbufs(node->node2, 1) != EBUSY
> >         test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL
> >         test VIDIOC_EXPBUF: OK
> >         test Requests: OK
> >
> > Total for mtk-fd-4.0 device /dev/video32: 45, Succeeded: 39, Failed: 6, Warnings: 0
> >
> > Grand Total for mtk-fd-4.0 device /dev/media2: 52, Succeeded: 46, Failed: 6, Warnings: 0
> >
>
Tomasz Figa May 21, 2020, 6:38 p.m. UTC | #3
Hi Jerry,

On Wed, May 13, 2020 at 11:45:37PM +0200, Tomasz Figa wrote:
> Hi Jerry,
> 
> On Fri, May 8, 2020 at 4:03 AM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> >
> > Hi Laurent, Tomasz, Matthias,
> >
> > gentle ping for this patch set,
> > If no new comments, I would like to send a newer version.
> >
> 
> Sorry, I still haven't had a chance to look at the series, so feel
> free to send a new version and I will take a look at the new one.
> 

Finally found some time to review the series. Again sorry for the delay
and thanks for your patience.

Some general comments:
1) The metadata format FourCC should be added in a separate patch,
together with documentation for it.
2) Control IDs, structs used by the userspace, etc. should be defined in
a header under include/uapi/linux.

Please also check my replies to particular patches for further comments.

Best regards,
Tomasz
Jerry-ch Chen June 30, 2020, 2:10 p.m. UTC | #4
Hi Tomasz,

On Thu, 2020-05-21 at 18:38 +0000, Tomasz Figa wrote:
> Hi Jerry,
> 
> On Wed, May 13, 2020 at 11:45:37PM +0200, Tomasz Figa wrote:
> > Hi Jerry,
> > 
> > On Fri, May 8, 2020 at 4:03 AM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > >
> > > Hi Laurent, Tomasz, Matthias,
> > >
> > > gentle ping for this patch set,
> > > If no new comments, I would like to send a newer version.
> > >
> > 
> > Sorry, I still haven't had a chance to look at the series, so feel
> > free to send a new version and I will take a look at the new one.
> > 
> 
> Finally found some time to review the series. Again sorry for the delay
> and thanks for your patience.
> 
> Some general comments:
> 1) The metadata format FourCC should be added in a separate patch,
> together with documentation for it.
> 2) Control IDs, structs used by the userspace, etc. should be defined in
> a header under include/uapi/linux.
> 
> Please also check my replies to particular patches for further comments.
> 
> Best regards,
> Tomasz

Appreciate for your reply,

So far, I've locally created an uapi header:
include/uapi/linux/mtk_fd_40.h
which provides some values, control ids, and the definitions of
structures that would be needed by user of mtk_fd_40 driver.
In addition, I also provide a MACRO as example in comments that can
extract the struct member with bit length and offset
definitions(eliminate the bit-fields).

Also, I would like to rename struct fd_user_output with struct
mtk_fd_hw_result. I worry fd_user_output would be a confusing name.
I will add them in a separate patch in next version.

I am still working on the documentation, which might be
Documentation/media/uapi/v4l/pixfmt-meta-mtk-fd-40.rst.
Refering the other pixfmt-*.rst files, I will try to provide the
flat-table of the metadata with the structure of the mtk_fd_hw_result.

I am confusing that should I remain the name with -40 in the tail of rst
file?
Since I think the layout of metadata might be different in the future
mtk fd drivers. Maybe they create a new one or should they update the
rst file when they are upstreaming?

Other comments are almost fixed, I will inform you by this mail thread
as soon as I finish the documentation of fd metadata format.

Sorry for the late response,
Thanks and Best Regards,
Jerry
Tomasz Figa June 30, 2020, 5:19 p.m. UTC | #5
Hi Jerry,

On Tue, Jun 30, 2020 at 10:10:53PM +0800, Jerry-ch Chen wrote:
> Hi Tomasz,
> 
> On Thu, 2020-05-21 at 18:38 +0000, Tomasz Figa wrote:
> > Hi Jerry,
> > 
> > On Wed, May 13, 2020 at 11:45:37PM +0200, Tomasz Figa wrote:
> > > Hi Jerry,
> > > 
> > > On Fri, May 8, 2020 at 4:03 AM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > >
> > > > Hi Laurent, Tomasz, Matthias,
> > > >
> > > > gentle ping for this patch set,
> > > > If no new comments, I would like to send a newer version.
> > > >
> > > 
> > > Sorry, I still haven't had a chance to look at the series, so feel
> > > free to send a new version and I will take a look at the new one.
> > > 
> > 
> > Finally found some time to review the series. Again sorry for the delay
> > and thanks for your patience.
> > 
> > Some general comments:
> > 1) The metadata format FourCC should be added in a separate patch,
> > together with documentation for it.
> > 2) Control IDs, structs used by the userspace, etc. should be defined in
> > a header under include/uapi/linux.
> > 
> > Please also check my replies to particular patches for further comments.
> > 
> > Best regards,
> > Tomasz
> 
> Appreciate for your reply,
> 
> So far, I've locally created an uapi header:
> include/uapi/linux/mtk_fd_40.h
> which provides some values, control ids, and the definitions of
> structures that would be needed by user of mtk_fd_40 driver.
> In addition, I also provide a MACRO as example in comments that can
> extract the struct member with bit length and offset
> definitions(eliminate the bit-fields).
> 
> Also, I would like to rename struct fd_user_output with struct
> mtk_fd_hw_result. I worry fd_user_output would be a confusing name.

The change sounds good to me.

> I will add them in a separate patch in next version.
> 

Okay.

> I am still working on the documentation, which might be
> Documentation/media/uapi/v4l/pixfmt-meta-mtk-fd-40.rst.
> Refering the other pixfmt-*.rst files, I will try to provide the
> flat-table of the metadata with the structure of the mtk_fd_hw_result.
> 

Sounds good to me.

> I am confusing that should I remain the name with -40 in the tail of rst
> file?

The header and documentation file names should match the driver name.  I
just noticed there is some inconsistency in the naming, though. The
driver seems to be located under drivers/media/platform/mtk-isp/fd, but
the driver name in the platform driver struct and as reported by
VIDIOC_QUERYCAP seems to be "mtk-fd-4.0". 

Since we have many mtk-* drivers in the tree currently, I think it might
make sense to consolidate them under drivers/media/platform/mediatek,
similarly to drivers/media/platform/qcom or /rockchip. But it could be
done later, as a follow-up.

My suggestion would be to place the driver under
drivers/media/platform/mtk-fd-40 and also rename the related Kconfig
symbol to include the _40 suffix.

What do you think?

> Since I think the layout of metadata might be different in the future
> mtk fd drivers. Maybe they create a new one or should they update the
> rst file when they are upstreaming?
> 

I think we'll decide what to do when that happens. Depending on whether
there is more face detection hardware available, we might be able to
migrate to standard controls and metadata buffer formats at that time.

> Other comments are almost fixed, I will inform you by this mail thread
> as soon as I finish the documentation of fd metadata format.

Okay, thanks.

Best regards,
Tomasz
Jerry-ch Chen Nov. 11, 2020, 11:51 a.m. UTC | #6
Hi Tomasz,

On Wed, 2020-07-01 at 01:19 +0800, Tomasz Figa wrote:
> Hi Jerry,
> 
> On Tue, Jun 30, 2020 at 10:10:53PM +0800, Jerry-ch Chen wrote:
> > Hi Tomasz,
> > 
> > On Thu, 2020-05-21 at 18:38 +0000, Tomasz Figa wrote:
> > > Hi Jerry,
> > > 
> > > On Wed, May 13, 2020 at 11:45:37PM +0200, Tomasz Figa wrote:
> > > > Hi Jerry,
> > > > 
> > > > On Fri, May 8, 2020 at 4:03 AM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > >
> > > > > Hi Laurent, Tomasz, Matthias,
> > > > >
> > > > > gentle ping for this patch set,
> > > > > If no new comments, I would like to send a newer version.
> > > > >
> > > > 
> > > > Sorry, I still haven't had a chance to look at the series, so feel
> > > > free to send a new version and I will take a look at the new one.
> > > > 
> > > 
> > > Finally found some time to review the series. Again sorry for the delay
> > > and thanks for your patience.
> > > 
> > > Some general comments:
> > > 1) The metadata format FourCC should be added in a separate patch,
> > > together with documentation for it.
> > > 2) Control IDs, structs used by the userspace, etc. should be defined in
> > > a header under include/uapi/linux.
> > > 
> > > Please also check my replies to particular patches for further comments.
> > > 
> > > Best regards,
> > > Tomasz
> > 
> > Appreciate for your reply,
> > 
> > So far, I've locally created an uapi header:
> > include/uapi/linux/mtk_fd_40.h
> > which provides some values, control ids, and the definitions of
> > structures that would be needed by user of mtk_fd_40 driver.
> > In addition, I also provide a MACRO as example in comments that can
> > extract the struct member with bit length and offset
> > definitions(eliminate the bit-fields).
> > 
> > Also, I would like to rename struct fd_user_output with struct
> > mtk_fd_hw_result. I worry fd_user_output would be a confusing name.
> 
> The change sounds good to me.
> 
> > I will add them in a separate patch in next version.
> > 
> 
> Okay.
> 
> > I am still working on the documentation, which might be
> > Documentation/media/uapi/v4l/pixfmt-meta-mtk-fd-40.rst.
> > Refering the other pixfmt-*.rst files, I will try to provide the
> > flat-table of the metadata with the structure of the mtk_fd_hw_result.
> > 
> 
> Sounds good to me.
> 
> > I am confusing that should I remain the name with -40 in the tail of rst
> > file?
> 
> The header and documentation file names should match the driver name.  I
> just noticed there is some inconsistency in the naming, though. The
> driver seems to be located under drivers/media/platform/mtk-isp/fd, but
> the driver name in the platform driver struct and as reported by
> VIDIOC_QUERYCAP seems to be "mtk-fd-4.0". 

> Since we have many mtk-* drivers in the tree currently, I think it might
> make sense to consolidate them under drivers/media/platform/mediatek,
> similarly to drivers/media/platform/qcom or /rockchip. But it could be
> done later, as a follow-up.
> 
> My suggestion would be to place the driver under
> drivers/media/platform/mtk-fd-40 and also rename the related Kconfig
> symbol to include the _40 suffix.
> 
> What do you think?
> 

I Appreciate your comments,
Sorry for the late reply.

Would it be possible for me to replace the driver as drivers/media/platform/mtk_fd/mtk-fd-40?(Just like mtk-isp/isp_50)

> > Since I think the layout of metadata might be different in the future
> > mtk fd drivers. Maybe they create a new one or should they update the
> > rst file when they are upstreaming?
> > 
> 
> I think we'll decide what to do when that happens. Depending on whether
> there is more face detection hardware available, we might be able to
> migrate to standard controls and metadata buffer formats at that time.
> 
Ok.

> > Other comments are almost fixed, I will inform you by this mail thread
> > as soon as I finish the documentation of fd metadata format.
> 
> Okay, thanks.
> 
> Best regards,
> Tomasz

Thanks and Best Regards,
Jerry
Tomasz Figa Nov. 12, 2020, 4:26 a.m. UTC | #7
On Wed, Nov 11, 2020 at 8:51 PM Jerry-ch Chen
<Jerry-ch.Chen@mediatek.com> wrote:
>
> Hi Tomasz,
>
> On Wed, 2020-07-01 at 01:19 +0800, Tomasz Figa wrote:
> > Hi Jerry,
> >
> > On Tue, Jun 30, 2020 at 10:10:53PM +0800, Jerry-ch Chen wrote:
> > > Hi Tomasz,
> > >
> > > On Thu, 2020-05-21 at 18:38 +0000, Tomasz Figa wrote:
> > > > Hi Jerry,
> > > >
> > > > On Wed, May 13, 2020 at 11:45:37PM +0200, Tomasz Figa wrote:
> > > > > Hi Jerry,
> > > > >
> > > > > On Fri, May 8, 2020 at 4:03 AM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > >
> > > > > > Hi Laurent, Tomasz, Matthias,
> > > > > >
> > > > > > gentle ping for this patch set,
> > > > > > If no new comments, I would like to send a newer version.
> > > > > >
> > > > >
> > > > > Sorry, I still haven't had a chance to look at the series, so feel
> > > > > free to send a new version and I will take a look at the new one.
> > > > >
> > > >
> > > > Finally found some time to review the series. Again sorry for the delay
> > > > and thanks for your patience.
> > > >
> > > > Some general comments:
> > > > 1) The metadata format FourCC should be added in a separate patch,
> > > > together with documentation for it.
> > > > 2) Control IDs, structs used by the userspace, etc. should be defined in
> > > > a header under include/uapi/linux.
> > > >
> > > > Please also check my replies to particular patches for further comments.
> > > >
> > > > Best regards,
> > > > Tomasz
> > >
> > > Appreciate for your reply,
> > >
> > > So far, I've locally created an uapi header:
> > > include/uapi/linux/mtk_fd_40.h
> > > which provides some values, control ids, and the definitions of
> > > structures that would be needed by user of mtk_fd_40 driver.
> > > In addition, I also provide a MACRO as example in comments that can
> > > extract the struct member with bit length and offset
> > > definitions(eliminate the bit-fields).
> > >
> > > Also, I would like to rename struct fd_user_output with struct
> > > mtk_fd_hw_result. I worry fd_user_output would be a confusing name.
> >
> > The change sounds good to me.
> >
> > > I will add them in a separate patch in next version.
> > >
> >
> > Okay.
> >
> > > I am still working on the documentation, which might be
> > > Documentation/media/uapi/v4l/pixfmt-meta-mtk-fd-40.rst.
> > > Refering the other pixfmt-*.rst files, I will try to provide the
> > > flat-table of the metadata with the structure of the mtk_fd_hw_result.
> > >
> >
> > Sounds good to me.
> >
> > > I am confusing that should I remain the name with -40 in the tail of rst
> > > file?
> >
> > The header and documentation file names should match the driver name.  I
> > just noticed there is some inconsistency in the naming, though. The
> > driver seems to be located under drivers/media/platform/mtk-isp/fd, but
> > the driver name in the platform driver struct and as reported by
> > VIDIOC_QUERYCAP seems to be "mtk-fd-4.0".
>
> > Since we have many mtk-* drivers in the tree currently, I think it might
> > make sense to consolidate them under drivers/media/platform/mediatek,
> > similarly to drivers/media/platform/qcom or /rockchip. But it could be
> > done later, as a follow-up.
> >
> > My suggestion would be to place the driver under
> > drivers/media/platform/mtk-fd-40 and also rename the related Kconfig
> > symbol to include the _40 suffix.
> >
> > What do you think?
> >
>
> I Appreciate your comments,
> Sorry for the late reply.
>
> Would it be possible for me to replace the driver as drivers/media/platform/mtk_fd/mtk-fd-40?(Just like mtk-isp/isp_50)
>

I'm not a big fan of duplicating "mtk fd" in the path. How about just
making it drivers/media/platform/mtk-fd-40?

Best regards,
Tomasz
Jerry-ch Chen Nov. 12, 2020, 12:05 p.m. UTC | #8
Hi Tomasz,

On Thu, 2020-11-12 at 13:26 +0900, Tomasz Figa wrote:
> On Wed, Nov 11, 2020 at 8:51 PM Jerry-ch Chen
> <Jerry-ch.Chen@mediatek.com> wrote:
> >
> > Hi Tomasz,
> >
> > On Wed, 2020-07-01 at 01:19 +0800, Tomasz Figa wrote:
> > > Hi Jerry,
> > >
> > > On Tue, Jun 30, 2020 at 10:10:53PM +0800, Jerry-ch Chen wrote:
> > > > Hi Tomasz,
> > > >
> > > > On Thu, 2020-05-21 at 18:38 +0000, Tomasz Figa wrote:
> > > > > Hi Jerry,
> > > > >
> > > > > On Wed, May 13, 2020 at 11:45:37PM +0200, Tomasz Figa wrote:
> > > > > > Hi Jerry,
> > > > > >
> > > > > > On Fri, May 8, 2020 at 4:03 AM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > >
> > > > > > > Hi Laurent, Tomasz, Matthias,
> > > > > > >
> > > > > > > gentle ping for this patch set,
> > > > > > > If no new comments, I would like to send a newer version.
> > > > > > >
> > > > > >
> > > > > > Sorry, I still haven't had a chance to look at the series, so feel
> > > > > > free to send a new version and I will take a look at the new one.
> > > > > >
> > > > >
> > > > > Finally found some time to review the series. Again sorry for the delay
> > > > > and thanks for your patience.
> > > > >
> > > > > Some general comments:
> > > > > 1) The metadata format FourCC should be added in a separate patch,
> > > > > together with documentation for it.
> > > > > 2) Control IDs, structs used by the userspace, etc. should be defined in
> > > > > a header under include/uapi/linux.
> > > > >
> > > > > Please also check my replies to particular patches for further comments.
> > > > >
> > > > > Best regards,
> > > > > Tomasz
> > > >
> > > > Appreciate for your reply,
> > > >
> > > > So far, I've locally created an uapi header:
> > > > include/uapi/linux/mtk_fd_40.h
> > > > which provides some values, control ids, and the definitions of
> > > > structures that would be needed by user of mtk_fd_40 driver.
> > > > In addition, I also provide a MACRO as example in comments that can
> > > > extract the struct member with bit length and offset
> > > > definitions(eliminate the bit-fields).
> > > >
> > > > Also, I would like to rename struct fd_user_output with struct
> > > > mtk_fd_hw_result. I worry fd_user_output would be a confusing name.
> > >
> > > The change sounds good to me.
> > >
> > > > I will add them in a separate patch in next version.
> > > >
> > >
> > > Okay.
> > >
> > > > I am still working on the documentation, which might be
> > > > Documentation/media/uapi/v4l/pixfmt-meta-mtk-fd-40.rst.
> > > > Refering the other pixfmt-*.rst files, I will try to provide the
> > > > flat-table of the metadata with the structure of the mtk_fd_hw_result.
> > > >
> > >
> > > Sounds good to me.
> > >
> > > > I am confusing that should I remain the name with -40 in the tail of rst
> > > > file?
> > >
> > > The header and documentation file names should match the driver name.  I
> > > just noticed there is some inconsistency in the naming, though. The
> > > driver seems to be located under drivers/media/platform/mtk-isp/fd, but
> > > the driver name in the platform driver struct and as reported by
> > > VIDIOC_QUERYCAP seems to be "mtk-fd-4.0".
> >
> > > Since we have many mtk-* drivers in the tree currently, I think it might
> > > make sense to consolidate them under drivers/media/platform/mediatek,
> > > similarly to drivers/media/platform/qcom or /rockchip. But it could be
> > > done later, as a follow-up.
> > >
> > > My suggestion would be to place the driver under
> > > drivers/media/platform/mtk-fd-40 and also rename the related Kconfig
> > > symbol to include the _40 suffix.
> > >
> > > What do you think?
> > >
> >
> > I Appreciate your comments,
> > Sorry for the late reply.
> >
> > Would it be possible for me to replace the driver as drivers/media/platform/mtk_fd/mtk-fd-40?(Just like mtk-isp/isp_50)
> >
> 
> I'm not a big fan of duplicating "mtk fd" in the path. How about just
> making it drivers/media/platform/mtk-fd-40?
> 

Ok, I will make it drivers/media/platform/mtk-fd-40,
and also rename the related Kconfig symbol to include the _40 suffix.

Thanks and Best Regards,
Jerry.

> Best regards,
> Tomasz
Jerry-ch Chen Dec. 28, 2020, 5:02 p.m. UTC | #9
Hi Tomasz,

On Thu, 2020-11-12 at 20:05 +0800, Jerry-ch Chen wrote:
> Hi Tomasz,
> 
> On Thu, 2020-11-12 at 13:26 +0900, Tomasz Figa wrote:
> > On Wed, Nov 11, 2020 at 8:51 PM Jerry-ch Chen
> > <Jerry-ch.Chen@mediatek.com> wrote:
> > >
> > > Hi Tomasz,
> > >
> > > On Wed, 2020-07-01 at 01:19 +0800, Tomasz Figa wrote:
> > > > Hi Jerry,
> > > >
> > > > On Tue, Jun 30, 2020 at 10:10:53PM +0800, Jerry-ch Chen wrote:
> > > > > Hi Tomasz,
> > > > >
> > > > > On Thu, 2020-05-21 at 18:38 +0000, Tomasz Figa wrote:
> > > > > > Hi Jerry,
> > > > > >
> > > > > > On Wed, May 13, 2020 at 11:45:37PM +0200, Tomasz Figa wrote:
> > > > > > > Hi Jerry,
> > > > > > >
> > > > > > > On Fri, May 8, 2020 at 4:03 AM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > >
> > > > > > > > Hi Laurent, Tomasz, Matthias,
> > > > > > > >
> > > > > > > > gentle ping for this patch set,
> > > > > > > > If no new comments, I would like to send a newer version.
> > > > > > > >
> > > > > > >
> > > > > > > Sorry, I still haven't had a chance to look at the series, so feel
> > > > > > > free to send a new version and I will take a look at the new one.
> > > > > > >
> > > > > >
> > > > > > Finally found some time to review the series. Again sorry for the delay
> > > > > > and thanks for your patience.
> > > > > >
> > > > > > Some general comments:
> > > > > > 1) The metadata format FourCC should be added in a separate patch,
> > > > > > together with documentation for it.
> > > > > > 2) Control IDs, structs used by the userspace, etc. should be defined in
> > > > > > a header under include/uapi/linux.
> > > > > >
> > > > > > Please also check my replies to particular patches for further comments.
> > > > > >
> > > > > > Best regards,
> > > > > > Tomasz
> > > > >
> > > > > Appreciate for your reply,
> > > > >
> > > > > So far, I've locally created an uapi header:
> > > > > include/uapi/linux/mtk_fd_40.h
> > > > > which provides some values, control ids, and the definitions of
> > > > > structures that would be needed by user of mtk_fd_40 driver.
> > > > > In addition, I also provide a MACRO as example in comments that can
> > > > > extract the struct member with bit length and offset
> > > > > definitions(eliminate the bit-fields).
> > > > >
> > > > > Also, I would like to rename struct fd_user_output with struct
> > > > > mtk_fd_hw_result. I worry fd_user_output would be a confusing name.
> > > >
> > > > The change sounds good to me.
> > > >
> > > > > I will add them in a separate patch in next version.
> > > > >
> > > >
> > > > Okay.
> > > >
> > > > > I am still working on the documentation, which might be
> > > > > Documentation/media/uapi/v4l/pixfmt-meta-mtk-fd-40.rst.
> > > > > Refering the other pixfmt-*.rst files, I will try to provide the
> > > > > flat-table of the metadata with the structure of the mtk_fd_hw_result.
> > > > >
> > > >
> > > > Sounds good to me.
> > > >
> > > > > I am confusing that should I remain the name with -40 in the tail of rst
> > > > > file?
> > > >
> > > > The header and documentation file names should match the driver name.  I
> > > > just noticed there is some inconsistency in the naming, though. The
> > > > driver seems to be located under drivers/media/platform/mtk-isp/fd, but
> > > > the driver name in the platform driver struct and as reported by
> > > > VIDIOC_QUERYCAP seems to be "mtk-fd-4.0".
> > >
> > > > Since we have many mtk-* drivers in the tree currently, I think it might
> > > > make sense to consolidate them under drivers/media/platform/mediatek,
> > > > similarly to drivers/media/platform/qcom or /rockchip. But it could be
> > > > done later, as a follow-up.
> > > >
> > > > My suggestion would be to place the driver under
> > > > drivers/media/platform/mtk-fd-40 and also rename the related Kconfig
> > > > symbol to include the _40 suffix.
> > > >
> > > > What do you think?
> > > >
> > >
> > > I Appreciate your comments,
> > > Sorry for the late reply.
> > >
> > > Would it be possible for me to replace the driver as drivers/media/platform/mtk_fd/mtk-fd-40?(Just like mtk-isp/isp_50)
> > >
> > 
> > I'm not a big fan of duplicating "mtk fd" in the path. How about just
> > making it drivers/media/platform/mtk-fd-40?
> > 
> 
> Ok, I will make it drivers/media/platform/mtk-fd-40,
> and also rename the related Kconfig symbol to include the _40 suffix.
> 
> Thanks and Best Regards,
> Jerry.
> 
> > Best regards,
> > Tomasz
> 


I've finish the document of FD driver, describing the structure of the
mtk_fd_hw_result. Could I send the new version of the driver? would the
folder path replacement must be included in the new version?

Thanks and Best Regards,
Jerry
Tomasz Figa Jan. 6, 2021, 6:30 a.m. UTC | #10
On Tue, Dec 29, 2020 at 2:02 AM Jerry-ch Chen
<Jerry-ch.Chen@mediatek.com> wrote:
>
> Hi Tomasz,
>
> On Thu, 2020-11-12 at 20:05 +0800, Jerry-ch Chen wrote:
> > Hi Tomasz,
> >
> > On Thu, 2020-11-12 at 13:26 +0900, Tomasz Figa wrote:
> > > On Wed, Nov 11, 2020 at 8:51 PM Jerry-ch Chen
> > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > >
> > > > Hi Tomasz,
> > > >
> > > > On Wed, 2020-07-01 at 01:19 +0800, Tomasz Figa wrote:
> > > > > Hi Jerry,
> > > > >
> > > > > On Tue, Jun 30, 2020 at 10:10:53PM +0800, Jerry-ch Chen wrote:
> > > > > > Hi Tomasz,
> > > > > >
> > > > > > On Thu, 2020-05-21 at 18:38 +0000, Tomasz Figa wrote:
> > > > > > > Hi Jerry,
> > > > > > >
> > > > > > > On Wed, May 13, 2020 at 11:45:37PM +0200, Tomasz Figa wrote:
> > > > > > > > Hi Jerry,
> > > > > > > >
> > > > > > > > On Fri, May 8, 2020 at 4:03 AM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Laurent, Tomasz, Matthias,
> > > > > > > > >
> > > > > > > > > gentle ping for this patch set,
> > > > > > > > > If no new comments, I would like to send a newer version.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Sorry, I still haven't had a chance to look at the series, so feel
> > > > > > > > free to send a new version and I will take a look at the new one.
> > > > > > > >
> > > > > > >
> > > > > > > Finally found some time to review the series. Again sorry for the delay
> > > > > > > and thanks for your patience.
> > > > > > >
> > > > > > > Some general comments:
> > > > > > > 1) The metadata format FourCC should be added in a separate patch,
> > > > > > > together with documentation for it.
> > > > > > > 2) Control IDs, structs used by the userspace, etc. should be defined in
> > > > > > > a header under include/uapi/linux.
> > > > > > >
> > > > > > > Please also check my replies to particular patches for further comments.
> > > > > > >
> > > > > > > Best regards,
> > > > > > > Tomasz
> > > > > >
> > > > > > Appreciate for your reply,
> > > > > >
> > > > > > So far, I've locally created an uapi header:
> > > > > > include/uapi/linux/mtk_fd_40.h
> > > > > > which provides some values, control ids, and the definitions of
> > > > > > structures that would be needed by user of mtk_fd_40 driver.
> > > > > > In addition, I also provide a MACRO as example in comments that can
> > > > > > extract the struct member with bit length and offset
> > > > > > definitions(eliminate the bit-fields).
> > > > > >
> > > > > > Also, I would like to rename struct fd_user_output with struct
> > > > > > mtk_fd_hw_result. I worry fd_user_output would be a confusing name.
> > > > >
> > > > > The change sounds good to me.
> > > > >
> > > > > > I will add them in a separate patch in next version.
> > > > > >
> > > > >
> > > > > Okay.
> > > > >
> > > > > > I am still working on the documentation, which might be
> > > > > > Documentation/media/uapi/v4l/pixfmt-meta-mtk-fd-40.rst.
> > > > > > Refering the other pixfmt-*.rst files, I will try to provide the
> > > > > > flat-table of the metadata with the structure of the mtk_fd_hw_result.
> > > > > >
> > > > >
> > > > > Sounds good to me.
> > > > >
> > > > > > I am confusing that should I remain the name with -40 in the tail of rst
> > > > > > file?
> > > > >
> > > > > The header and documentation file names should match the driver name.  I
> > > > > just noticed there is some inconsistency in the naming, though. The
> > > > > driver seems to be located under drivers/media/platform/mtk-isp/fd, but
> > > > > the driver name in the platform driver struct and as reported by
> > > > > VIDIOC_QUERYCAP seems to be "mtk-fd-4.0".
> > > >
> > > > > Since we have many mtk-* drivers in the tree currently, I think it might
> > > > > make sense to consolidate them under drivers/media/platform/mediatek,
> > > > > similarly to drivers/media/platform/qcom or /rockchip. But it could be
> > > > > done later, as a follow-up.
> > > > >
> > > > > My suggestion would be to place the driver under
> > > > > drivers/media/platform/mtk-fd-40 and also rename the related Kconfig
> > > > > symbol to include the _40 suffix.
> > > > >
> > > > > What do you think?
> > > > >
> > > >
> > > > I Appreciate your comments,
> > > > Sorry for the late reply.
> > > >
> > > > Would it be possible for me to replace the driver as drivers/media/platform/mtk_fd/mtk-fd-40?(Just like mtk-isp/isp_50)
> > > >
> > >
> > > I'm not a big fan of duplicating "mtk fd" in the path. How about just
> > > making it drivers/media/platform/mtk-fd-40?
> > >
> >
> > Ok, I will make it drivers/media/platform/mtk-fd-40,
> > and also rename the related Kconfig symbol to include the _40 suffix.
> >
> > Thanks and Best Regards,
> > Jerry.
> >
> > > Best regards,
> > > Tomasz
> >
>
>
> I've finish the document of FD driver, describing the structure of the
> mtk_fd_hw_result. Could I send the new version of the driver? would the
> folder path replacement must be included in the new version?

Thanks Jerry and Happy New Year.

Since the driver should be in a good shape after addressing the last
comments, perhaps it would make sense to update the path as well, so
that it can be merged if no other issues are found?

Best regards,
Tomasz