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

Du, Bin
2025-06-20T03:32:18Z

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