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

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

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@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.
> > 
> 
> 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.

-- 
Kind regards,

Sakari Ailus