Re: [PATCH v2 8/8] Documentation: add documentation of AMD isp 4 driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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@amd.com>
> > > Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.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.

-- 
Kind regards,

Sakari Ailus