Re: [PATCH 00/10] Keem Bay Camera Subsystem

["Rosikopulos, GjorgjiX" <gjorgjix.rosikopulos@linux.intel.com>]

Hi Laurent,

On 16/04/2021 09:37, Laurent Pinchart wrote:
> Hi Martina and Gjorgji,
>
> Nice to see a new ISP driver :-)
Pleasure for us submit new ISP driver :-).
>
> Before reviewing patches in details, I have a few high-level questions:
>
> - The driver seems to proxy access to the ISP through the VPU firmware.
>    I assume the VPU is a separate CPU core that controls the hardware
>    directly. Is that correct ?

Yes that is correct.

>
> - Does this driver support all the features of the ISP, or only the
>    subset that a particular VPU firmware exposes ? In particular, the ISP
>    is exposed as an inline block, which no memory buffer between the
>    CSI-2 receiver and the ISP, and no ability to capture raw frames. How
>    is one supposed to tune cameras ?

The driver exposes all the features supported by ISP the firmware as 
high level API.

However this patch-set is not exposing all the features of the VPU API 
in userspace. Some of them will came in next patch-set some of them

are part of the separate patch-set (HDR 2DOL and 3DOL support).

The changes which will be posted in next patch-set are:

1. Raw capture support (link can be activated per need).

2. Support for 2 additional scaled outputs which again their links can 
be activated per need.

3. The full size output which is included in this patch-set will remain 
immutable active.


The additional features which will be added as separate patch-set and 
RFC are:

4. 2DOL and 3DOL support. We dont have yet interface for multiple 
streams over one link. We will use current RFC for that. (That why this 
is not included in first patch-set).

Also we need to discuss how to enable raw capture for 2DOL and 3DOL 
usecases....


Regarding the VPU operation. The VPU captures frames from CSI2 in to 
memory and then uses ISP mem2mem processing, there is internal memory 
pool used by the firmware.

When raw capture is enabled the raw buffer pool is allocated in the 
linux driver side and used by VPU for storing the frames to memory, then 
they are processed

by ISP and returned to linux driver side.

>
> - More documentation is needed for both the device architecture (in
>    particular a block diagram of the processing pipeline), and the
>    configuration parameters. Is there ongoing work in this area ?
Yes this is painful part. We are working to get approval for providing 
the needed documentation...
>
> - Last but not least, we need a reference userspace implementation to
>    test this driver. I recommend implementing support in libcamera :-)

So i am not sure if this is a planned effort. Currently we are using 
yavta for doing tests same as IPU3 example,

with prepared configurations and read them from file. I agree that it 
will be great to have libcamera support but

for now is not part of our up-streaming effort :/

Regards,

~Gjorgji

>
> On Fri, Mar 19, 2021 at 06:06:22PM +0000, Martina Krasteva wrote:
>> From: Martina Krasteva <martinax.krasteva@intel.com>
>>
>> Patch series contains Keem Bay Camera Subsystem driver implementation,
>> documentation and devicetree binding document.
>>
>> Gjorgji Rosikopulos (7):
>>    media: Keem Bay Camera: Keem Bay camera driver
>>    media: Keem Bay Camera: Add VPU camera interface
>>    uapi: Keem Bay ISP Parameters data types
>>    media: v4l: Add Keem Bay Camera meta buffer formats
>>    media: Keem Bay Camera: Add ISP sub-device
>>    media: Keem Bay Camera: Add metadata video node
>>    media: admin-guide: Add documentation for Keem Bay Camera
>>
>> Martina Krasteva (3):
>>    dt-bindings: media: Add bindings for Keem Bay Camera
>>    media: Keem Bay Camera: Add pipeline support
>>    media: Keem Bay Camera: Add capture video node
>>
>>   Documentation/admin-guide/media/keembay-camera.dot |   12 +
>>   Documentation/admin-guide/media/keembay-camera.rst |  174 ++
>>   Documentation/admin-guide/media/v4l-drivers.rst    |    1 +
>>   .../bindings/media/intel,keembay-camera.yaml       |   98 ++
>>   .../userspace-api/media/v4l/meta-formats.rst       |    1 +
>>   .../media/v4l/pixfmt-meta-intel-kmb.rst            |   98 ++
>>   MAINTAINERS                                        |   14 +
>>   drivers/media/platform/Kconfig                     |    1 +
>>   drivers/media/platform/Makefile                    |    2 +
>>   drivers/media/platform/keembay-camera/Kconfig      |   11 +
>>   drivers/media/platform/keembay-camera/Makefile     |    5 +
>>   .../platform/keembay-camera/keembay-cam-xlink.c    |  327 ++++
>>   .../platform/keembay-camera/keembay-cam-xlink.h    |   49 +
>>   .../media/platform/keembay-camera/keembay-camera.c |  287 +++
>>   .../media/platform/keembay-camera/keembay-camera.h |   43 +
>>   .../media/platform/keembay-camera/keembay-isp.c    | 1397 +++++++++++++++
>>   .../media/platform/keembay-camera/keembay-isp.h    |  136 ++
>>   .../platform/keembay-camera/keembay-metadata.c     | 1860 ++++++++++++++++++++
>>   .../platform/keembay-camera/keembay-metadata.h     |  154 ++
>>   .../keembay-camera/keembay-params-defaults.c       |  326 ++++
>>   .../keembay-camera/keembay-params-defaults.h       |   38 +
>>   .../platform/keembay-camera/keembay-pipeline.c     |  401 +++++
>>   .../platform/keembay-camera/keembay-pipeline.h     |   75 +
>>   .../media/platform/keembay-camera/keembay-video.c  |  922 ++++++++++
>>   .../media/platform/keembay-camera/keembay-video.h  |   74 +
>>   .../platform/keembay-camera/keembay-vpu-cmd.h      |  110 ++
>>   .../platform/keembay-camera/keembay-vpu-frame.h    |  102 ++
>>   .../platform/keembay-camera/keembay-vpu-isp.h      |  724 ++++++++
>>   .../platform/keembay-camera/keembay-vpu-pipe.h     |  110 ++
>>   .../platform/keembay-camera/keembay-vpu-src.h      |  193 ++
>>   include/uapi/linux/keembay-isp-ctl.h               |  796 +++++++++
>>   include/uapi/linux/videodev2.h                     |    4 +
>>   32 files changed, 8545 insertions(+)
>>   create mode 100644 Documentation/admin-guide/media/keembay-camera.dot
>>   create mode 100644 Documentation/admin-guide/media/keembay-camera.rst
>>   create mode 100644 Documentation/devicetree/bindings/media/intel,keembay-camera.yaml
>>   create mode 100644 Documentation/userspace-api/media/v4l/pixfmt-meta-intel-kmb.rst
>>   create mode 100644 drivers/media/platform/keembay-camera/Kconfig
>>   create mode 100644 drivers/media/platform/keembay-camera/Makefile
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-cam-xlink.c
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-cam-xlink.h
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-camera.c
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-camera.h
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-isp.c
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-isp.h
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-metadata.c
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-metadata.h
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-params-defaults.c
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-params-defaults.h
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-pipeline.c
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-pipeline.h
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-video.c
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-video.h
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-vpu-cmd.h
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-vpu-frame.h
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-vpu-isp.h
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-vpu-pipe.h
>>   create mode 100644 drivers/media/platform/keembay-camera/keembay-vpu-src.h
>>   create mode 100644 include/uapi/linux/keembay-isp-ctl.h
>>
>>
>> base-commit: f00397ee41c79b6155b9b44abd0055b2c0621349