Message-ID: <9b56ab20-dc09-47a8-8ab2-c8ef1fd8f782@amd.com>
Date: Fri, 22 Aug 2025 10:28:37 +0800
From: "Du, Bin" <bin.du@....com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: mchehab@...nel.org, hverkuil@...all.nl, bryan.odonoghue@...aro.org,
 sakari.ailus@...ux.intel.com, prabhakar.mahadev-lad.rj@...renesas.com,
 linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 pratap.nirujogi@....com, benjamin.chan@....com, king.li@....com,
 gjorgji.rosikopulos@....com, Phil.Jawich@....com, Dominic.Antony@....com,
 bin.du@....com, Mario Limonciello <mario.limonciello@....com>,
 Richard.Gong@....com, anson.tsao@....com
Subject: Re: [PATCH v2 8/8] Documentation: add documentation of AMD isp 4
 driver

Many thanks Laurent, sorry for the late response because we had a lot of 
internal discussion and did possible solution prototype validation.

On 8/12/2025 9:42 PM, Laurent Pinchart wrote:
> On Tue, Aug 12, 2025 at 09:36:04AM +0800, Du, Bin wrote:
>> On 8/5/2025 7:37 PM, Laurent Pinchart wrote:
>>> On Wed, Jun 18, 2025 at 05:19:59PM +0800, Bin Du wrote:
>>>> Add documentation for AMD isp 4 and describe the main components
>>>>
>>>> Signed-off-by: Bin Du <Bin.Du@....com>
>>>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@....com>
>>>> ---
>>>>    Documentation/admin-guide/media/amdisp4-1.rst | 64 +++++++++++++++++++
>>>>    Documentation/admin-guide/media/amdisp4.dot   |  8 +++
>>>>    MAINTAINERS                                   |  2 +
>>>>    3 files changed, 74 insertions(+)
>>>>    create mode 100644 Documentation/admin-guide/media/amdisp4-1.rst
>>>>    create mode 100644 Documentation/admin-guide/media/amdisp4.dot
>>>>
>>>> diff --git a/Documentation/admin-guide/media/amdisp4-1.rst b/Documentation/admin-guide/media/amdisp4-1.rst
>>>> new file mode 100644
>>>> index 000000000000..417b15af689a
>>>> --- /dev/null
>>>> +++ b/Documentation/admin-guide/media/amdisp4-1.rst
>>>> @@ -0,0 +1,64 @@
>>>> +.. SPDX-License-Identifier: GPL-2.0
>>>> +
>>>> +.. include:: <isonum.txt>
>>>> +
>>>> +====================================
>>>> +AMD Image Signal Processor (amdisp4)
>>>> +====================================
>>>> +
>>>> +Introduction
>>>> +============
>>>> +
>>>> +This file documents the driver for the AMD ISP4 that is part of
>>>> +AMD Ryzen AI Max 385 SoC.
>>>> +
>>>> +The driver is located under drivers/media/platform/amd/isp4 and uses
>>>> +the Media-Controller API.
>>>> +
>>>> +Topology
>>>> +========
>>>> +
>>>> +.. _amdisp4_topology_graph:
>>>> +
>>>> +.. kernel-figure:: amdisp4.dot
>>>> +     :alt:   Diagram of the media pipeline topology
>>>> +     :align: center
>>>> +
>>>> +
>>>> +
>>>> +The driver has 1 sub-device:
>>>> +
>>>> +- isp: used to resize and process bayer raw frames in to yuv.
>>>> +
>>>> +The driver has 1 video device:
>>>> +
>>>> +- <capture video device: capture device for retrieving images.
>>>> +
>>>> +
>>>> +  - ISP4 Image Signal Processing Subdevice Node
>>>> +-----------------------------------------------
>>>> +
>>>> +The isp4 is represented as a single V4L2 subdev, the sub-device does not
>>>> +provide interface to the user space.
>>>
>>> Doesn't it ? The driver sets the V4L2_SUBDEV_FL_HAS_DEVNODE flag for the
>>> subdev, and calls v4l2_device_register_subdev_nodes().
>>
>> We have exported subdev device to user space during the testing with
>> libcamera sample pipeline.
> 
> But it's not needed anymore ? If not, you could stop exposing the subdev
> to userspace for the time being.
> 

Yes, will not expose ISP subdev to userspace. Please see more details below.

>>> As far as I understand, the camera is exposed by the firmware with a
>>> webcam-like interface. We need to better understand your plans with this
>>> driver. If everything is handled by the firmware, why are the sensor and
>>> subdev exposed to userspace ? Why can't you expose a single video
>>> capture device, with a media device, and handle everything behind the
>>> scene ? I assume there may be more features coming later. Please
>>> document the plan, we can't provide feedback on the architecture
>>> otherwise.
>>
>> Currently, isp fw is controlling the sensor to update just the exposure
>> and gain, since the 3A algorithms run on ISP HW rather than on x86.
> 
> This design decision makes my hair stand on end :-( Exposing the camera
> sensor to both the firmware and the host concurrently is asking for
> trouble. If you really want to abstract the camera behind a firmware and
> only expose a webcam-like API (or not even that in this version, as the
> driver exposes no control as far as I can see), then you should push the
> whole sensor handling to the firmware too. In my opinion that would not
> be a good solution compared to exposing the ISP to the host, but it
> would be better than this hybrid model.
> 

Totally agree, yes, the hybrid model is really a bad idea, based on our 
internal validation, we plan to take one of your suggestions, that is to 
push the whole sensor handling to the firmware, so our ISP driver will 
work like UVC driver.

>> In a
>> future version, we plan to introduce raw output support in the ISP
>> driver, allowing users to choose between AMD’s 3A running on ISP
>> hardware or a custom 3A running on x86. If the user opts for the
>> x86-based 3A, the firmware will relinquish control of the sensor, and
>> hands over full control to the x86 system.
> 
> Will the firmware at that point expose to Linux all the ISP statistics
> needed to implement auto-exposure ? What if I want to also set digital
> gain ? Or have manual white balance (requiring statistics), and manual
> CCM ?
> 

No, because all 3A, ISP and camera control will be put inside firmware. 
Currently, we only support auto mode, so, no manual control will be exposed

>>>> The sub-device is connected to one video node
>>>> +(isp4_capture) with immutable active link. The isp entity is connected
>>>> +to sensor pad 0 and receives the frames using CSI-2 protocol. The sub-device is
>>>> +also responsible to configure CSI2-2 receiver.
>>>> +The sub-device processes bayer raw data from the connected sensor and output
>>>> +them to different YUV formats. The isp also has scaling capabilities.
>>>> +
>>>> +  - isp4_capture - Frames Capture Video Node
>>>> +--------------------------------------------
>>>> +
>>>> +Isp4_capture is a capture device to capture frames to memory.
>>>> +This entity is the DMA engine that write the frames to memory.
>>>> +The entity is connected to isp4 sub-device.
>>>> +
>>>> +Capturing Video Frames Example
>>>> +==============================
>>>> +
>>>> +.. code-block:: bash
>>>> +
>>>> +         # set the links
>>>
>>> This seems very under-documented.
>>
>> Yes, documentation needs to be updated.
>>
>>>> +
>>>> +         # start streaming:
>>>> +         v4l2-ctl "-d" "/dev/video0" "--set-fmt-video=width=1920,height=1080,pixelformat=NV12" "--stream-mmap" "--stream-count=10"
>>>> diff --git a/Documentation/admin-guide/media/amdisp4.dot b/Documentation/admin-guide/media/amdisp4.dot
>>>> new file mode 100644
>>>> index 000000000000..a4c2f0cceb30
>>>> --- /dev/null
>>>> +++ b/Documentation/admin-guide/media/amdisp4.dot
>>>> @@ -0,0 +1,8 @@
>>>> +digraph board {
>>>> +	rankdir=TB
>>>> +	n00000001 [label="{{<port0> 0} | amd isp4\n | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
>>>> +	n00000001:port1 -> n00000004 [style=bold]
>>>> +	n00000004 [label="Preview\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
>>>> +	n0000000a [label="{{} | ov05c10 22-0010\n/dev/v4l-subdev0 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
>>>> +	n0000000a:port0 -> n00000001:port0 [style=bold]
>>>> +}
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 15070afb14b5..e4455bde376f 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -1113,6 +1113,8 @@ M:	Nirujogi Pratap <pratap.nirujogi@....com>
>>>>    L:	linux-media@...r.kernel.org
>>>>    S:	Maintained
>>>>    T:	git git://linuxtv.org/media.git
>>>> +F:	Documentation/admin-guide/media/amdisp4-1.rst
>>>> +F:	Documentation/admin-guide/media/amdisp4.dot
>>>>    F:	drivers/media/platform/amd/Kconfig
>>>>    F:	drivers/media/platform/amd/Makefile
>>>>    F:	drivers/media/platform/amd/isp4/*
> 

-- 
Regards,
Bin

Powered by blists - more mailing lists