| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a7a2e647-eee6-4ca7-ba91-13f5617e9b90@amd.com> Date: Fri, 20 Jun 2025 11:32:07 +0800 From: "Du, Bin" <bin.du@....com> To: Mario Limonciello <mario.limonciello@....com>, mchehab@...nel.org, hverkuil@...all.nl, laurent.pinchart+renesas@...asonboard.com, bryan.odonoghue@...aro.org, sakari.ailus@...ux.intel.com, prabhakar.mahadev-lad.rj@...renesas.com, linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, Alex Deucher <alexander.deucher@....com> Cc: pratap.nirujogi@....com, benjamin.chan@....com, king.li@....com, gjorgji.rosikopulos@....com, Phil.Jawich@....com, Dominic.Antony@....com, Svetoslav.Stoilov@....com, "amd-gfx@...ts.freedesktop.org" <amd-gfx@...ts.freedesktop.org> Subject: Re: [PATCH v2 4/8] media: platform: amd: Add isp4 fw and hw interface 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@....com> >>>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@....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
Powered by blists - more mailing lists