Message-ID: <081bc6ed-b1ef-41f2-84dc-a0904c4ed82e@amd.com>
Date: Mon, 22 Sep 2025 17:19:19 +0800
From: "Du, Bin" <bin.du@....com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, mchehab@...nel.org,
 hverkuil@...all.nl, bryan.odonoghue@...aro.org,
 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,
 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 Sakari for the update. All clear on my end — feel free to 
take the time you need.

On 9/22/2025 2:24 PM, Sakari Ailus wrote:
> Hi Bin,
> 
> On Fri, Aug 22, 2025 at 10:20:01AM +0800, Du, Bin wrote:
>> Many thanks Sakari Ailus for your deep insight
>>
>> On 8/20/2025 8:42 PM, Sakari Ailus wrote:
>>> Hi Bin,
>>>
>>> On Tue, Aug 12, 2025 at 09:36:04AM +0800, Du, Bin wrote:
>>>> Many thanks Laurent Pinchart for the review.
>>>>
>>>> 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.
>>>>
>>>>> 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. 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.
>>>
>>> There are a few problems I see with this approach.
>>>
>>> Camera sensors are separate devices from the ISP and they're expected to be
>>> controlled by the respective camera sensor drivers and these drivers only.
>>> The firmware contains the camera control algorithms as well as tuning; this
>>> is something that's better located outside of it.
>>>
>>> The complex camera system comprising of a camera sensor, an ISP and a
>>> microcontroller within you have is right now semi-integrated to the SoC and
>>> I think it needs to be either fully unintegrated (the ISPs we currently
>>> support) or fully integrated (e.g. UVC webcams).
>>>
>>> There are two options that I can see here, in descending order of
>>> preference:
>>>
>>> 1. Control the ISP processing blocks from the AMD ISP driver, via a
>>>      documented UAPI. This includes setting processing block parameters and
>>>      being able to obtain statistics from the ISP. This is aligned with the
>>>      other currently supported ISP drivers.
>>>      This option could include support for the CSI-2 receiver only, with the
>>>      processing taking place in SoftISP. Fully supported ISP would of course
>>>      be preferred.
>>>      Right now I don't have an opinion on whether or not this needs to
>>>      include libcamera support, but the ISP driver wouldn't be of much use
>>>      without that anyway.
>>>
>>> 2. Move sensor control to firmware and make the AMD ISP driver expose an
>>>      interface that looks like a webcam, akin to the UVC driver. In this case
>>>      there's also no use for the sensor driver you've posted earlier.
>>>      Overall, the ISP/sensor combination will probably be limited to use as a
>>>      webcam in this case.
>>>
>>
>> Based on our internal discussion and validation, will make option 2 as our
>> current upstream target, after that, will plan option 1 with more
>> considerations, a. Whether to provide the capability and interface so user
>> can do switch between option 1 and 2. b. Whether and how to expose interface
>> so host can leverage the ISP HW feature to accelerat some processing. Though
>> sensor driver is not used in option 2, we still plan to upstream it because
>> of option 1 as next step.
> 
> I expect this to take some time.
> 
> In the meantime, I'm inclined to merge ov02c05 driver from Bingbu / Hao in
> case they can provide an upstreamable one as that driver would have users
> already today. That being said, it won't be a problem accommodating the
> needs of both into the same driver.
> 

-- 
Regards,
Bin

Powered by blists - more mailing lists