Message-ID: <63949b3b-ca2d-42aa-bc8d-43f2952d307a@amd.com>
Date: Fri, 22 Aug 2025 10:20:01 +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, 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 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.

-- 
Regards,
Bin

Powered by blists - more mailing lists