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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aJnYE2Z7F-PK1VHL@kekkonen.localdomain>
Date: Mon, 11 Aug 2025 11:46:27 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: "Du, Bin" <bin.du@....com>
Cc: mchehab@...nel.org, hverkuil@...all.nl,
	laurent.pinchart+renesas@...asonboard.com,
	bryan.odonoghue@...aro.org, prabhakar.mahadev-lad.rj@...renesas.com,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	pratap.nirujogi@....com, benjamin.chan@....com, king.li@....com,
	gjorgji.rosikopulos@....com, Phil.Jawich@....com,
	Dominic.Antony@....com
Subject: Re: [PATCH v2 4/8] media: platform: amd: Add isp4 fw and hw interface

Hi Bin,

On Tue, Jul 29, 2025 at 05:12:03PM +0800, Du, Bin wrote:
> Many thanks Askari Ailus for your careful review
> 
> On 7/28/2025 3:23 PM, Sakari Ailus wrote:
> > Hi Bin,
> > 
> > On Wed, Jun 18, 2025 at 05:19:55PM +0800, 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.
> > 
> > Please rewrap this, some lines above are quite short.
> > 
> Thanks, the line after the short line is supposed to be a new paragraph?
> Should we put all the description in one paragraph?

One or more paragraphs work fine, but a new paragraph is separated from the
previous one by another newline.

...

> > > +	void *cpu_ptr;
> > > +	u64 gpu_addr;
> > > +	u32 ret;
> > > +
> > > +	dev = ispif->dev;
> > > +
> > > +	if (!mem_size)
> > > +		return NULL;
> > > +
> > > +	mem_info = kzalloc(sizeof(*mem_info), GFP_KERNEL);
> > > +	if (!mem_info)
> > > +		return NULL;
> > > +
> > > +	adev = (struct amdgpu_device *)ispif->adev;
> > 
> > Why the cast?
> > 
> > adev isn't a great name here as it's usually used for struct acpi_devices.
> > 
> In the next patch, will use new helper function for this and will no longer
> use amdgpu_device

Use correct types when you can; either way this doesn't seem to be changed
by the further patches in the set.

...

> > > +static int isp4if_gpu_mem_free(struct isp4_interface *ispif,
> > > +			       struct isp4if_gpu_mem_info *mem_info)
> > > +{
> > > +	struct device *dev = ispif->dev;
> > > +	struct amdgpu_bo *bo;
> > > +
> > > +	if (!mem_info) {
> > > +		dev_err(dev, "invalid mem_info\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	bo = (struct amdgpu_bo *)mem_info->mem_handle;
> > 
> > Why do you need to cast here?
> > 
> In the next patch, will use new helper function for this and will no longer
> use amdgpu_bo

Not quite, on top of this patch number 6 adds more of the same.

...

> > > +static struct isp4if_cmd_element *
> > > +isp4if_append_cmd_2_cmdq(struct isp4_interface *ispif,
> > > +			 struct isp4if_cmd_element *cmd_ele)
> > > +{
> > > +	struct isp4if_cmd_element *copy_command = NULL;
> > > +
> > > +	copy_command = kmalloc(sizeof(*copy_command), GFP_KERNEL);
> > > +	if (!copy_command)
> > > +		return NULL;
> > > +
> > > +	memcpy(copy_command, cmd_ele, sizeof(*copy_command));
> > 
> > kmemdup()?
> > 
> Kmemdup is to allocate memory and copy, can't be used here.

Isn't that what you're doing above?

> 
> > > +
> > > +	guard(mutex)(&ispif->cmdq_mutex);
> > > +
> > > +	list_add_tail(&copy_command->list, &ispif->cmdq);
> > > +
> > > +	return copy_command;
> > > +}

-- 
Regards,

Sakari Ailus