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

["Du, Bin" <bin.du@amd.com>]

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