Re: [PATCH v2 4/8] media: platform: amd: Add isp4 fw and hw interface

[Du, Bin]

Thanks Mario, add more comments

On 6/19/2025 11:11 PM, Mario Limonciello wrote:
> On 6/19/2025 4:58 AM, Du, Bin wrote:
> > Thanks Mario, will fix in the next version and pls see some of my 
> > comments
> >
> > On 6/19/2025 12:17 AM, Mario Limonciello wrote:
> > > +Alex
> > > +amd-gfx
> > >
> > > On 6/18/2025 4:19 AM, Bin Du wrote:
> > > > ISP firmware controls ISP HW pipeline using dedicated embedded 
> > > > processor
> > > > called ccpu.
> > > > The communication between ISP FW and driver is using commands and
> > > > response messages sent through the ring buffer. Command buffers support
> > > > either global setting that is not specific to the steam and support 
> > > > stream
> > > > specific parameters. Response buffers contains ISP FW notification
> > > > information such as frame buffer done and command done. IRQ is used for
> > > > receiving response buffer from ISP firmware, which is handled in the 
> > > > main
> > > > isp4 media device. ISP ccpu is booted up through the firmware loading
> > > > helper function prior to stream start.
> > > > Memory used for command buffer and response buffer needs to be 
> > > > allocated
> > > > from amdgpu buffer manager because isp4 is a child device of amdgpu.
> > > >
> > > > Signed-off-by: Bin Du <Bin.Du@xxxxxxx>
> > > > Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@xxxxxxx>
> > > > ---
> > > >   drivers/media/platform/amd/isp4/Makefile      |   12 +
> > > >   .../platform/amd/isp4/isp4_fw_cmd_resp.h      |  318 +++++
> > > >   .../media/platform/amd/isp4/isp4_interface.c  | 1052 +++++++++++++ 
> > > > ++++
> > > >   .../media/platform/amd/isp4/isp4_interface.h  |  164 +++
> > > >   4 files changed, 1546 insertions(+)
> > > >   create mode 100644 drivers/media/platform/amd/isp4/isp4_fw_cmd_resp.h
> > > >   create mode 100644 drivers/media/platform/amd/isp4/isp4_interface.c
> > > >   create mode 100644 drivers/media/platform/amd/isp4/isp4_interface.h
> > > >
> > > > diff --git a/drivers/media/platform/amd/isp4/Makefile b/drivers/ 
> > > > media/ platform/amd/isp4/Makefile
> > > > index 0e36201fbb30..c0166f954516 100644
> > > > --- a/drivers/media/platform/amd/isp4/Makefile
> > > > +++ b/drivers/media/platform/amd/isp4/Makefile
> > > > @@ -5,10 +5,22 @@
> > > >   obj-$(CONFIG_AMD_ISP4) += amd_capture.o
> > > >   amd_capture-objs := isp4.o    \
> > > >               isp4_phy.o \
> > > > +            isp4_interface.o \
> > > >               isp4_hw.o    \
> > > >   ccflags-y += -I$(srctree)/drivers/media/platform/amd/isp4
> > > > +ccflags-y += -I$(srctree)/drivers/gpu/drm/amd/include
> > > > +ccflags-y += -I$(srctree)/drivers/gpu/drm/amd/amdgpu
> > > > +ccflags-y += -I$(srctree)/drivers/gpu/drm/amd/pm/inc
> > > > +ccflags-y += -I$(srctree)/include/drm
> > > >   ccflags-y += -I$(srctree)/include
> > > > +ccflags-y += -I$(srctree)/include/uapi/drm
> > > > +ccflags-y += -I$(srctree)/drivers/gpu/drm/amd/acp/include
> > > > +ccflags-y += -I$(srctree)/drivers/gpu/drm/amd/display
> > > > +ccflags-y += -I$(srctree)/drivers/gpu/drm/amd/display/include
> > > > +ccflags-y += -I$(srctree)/drivers/gpu/drm/amd/display/modules/inc
> > > > +ccflags-y += -I$(srctree)/drivers/gpu/drm/amd/display/dc
> > > > +ccflags-y += -I$(srctree)/drivers/gpu/drm/amd/display/amdgpu_dm
> > >
> > > IMO this feels like a hack and also fragile to be sharing so much 
> > > across subsystems.
> > >
> > > Either there should be a kernel wide include/ header that can be used 
> > > by both or there should be a helper exported to get just the data 
> > > that is needed.
> >
> > Yes, will refine to remove unnecessary ones in the next version, 
> > actually isp driver needs to access function amdgpu_bo_create_kernel 
> > which is exported by amdgpu and delared in amdgpu_object.h, because 
> > amdgpu_object.h also includes other amd gpu header files, so have to 
> > add these include path to avoid compilation error.
> > It'll be greate if there is kernel wide include, any suggestion for this?
>
> Ah yeah I do see exports as of https://git.kernel.org/torvalds/c/ 
> ebbe34edc0a90
>
> So based on what is in the tree right now what you did "makes sense", 
> but I don't think it's "correct".  From another driver like the ISP 
> driver I feel that the code should really just be something like:
>
> #include <linux/amdgpu/isp.h>
>
> I see a few ways forward.
>
> 1) A new set of helpers created for amdgpu that can take
> opaque" data arguments (maybe the mfd device?) and return back a pointer 
> to the new buffer.  Describe those new helpers in that isp.h header.
>
> 2) Manage the lifecycle of the buffers in sw_init/sw_fini of the ISP in 
> amdgpu.  Store pointers to them for access in the mfd device, and let 
> the ISP driver take references.  This only works if you don't need new 
> buffers after the driver is loaded.
>
> I personally prefer option 2 if it is feasible.
>
> Alex or others might have some other ideas too.
2 is not feasible, because the buffer size and number are decided by App 
at runtime.
1 is really a good idea, will have more investigation internally