Message-ID: <50f0958b-5234-4a89-a57e-5d330cca13af@amd.com>
Date: Tue, 12 Aug 2025 11:36:23 +0800
From: "Du, Bin" <bin.du@....com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: mchehab@...nel.org, hverkuil@...all.nl, 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,
 Mario Limonciello <mario.limonciello@....com>, Richard.Gong@....com,
 anson.tsao@....com
Subject: Re: [PATCH v2 4/8] media: platform: amd: Add isp4 fw and hw interface

Many thanks Laurent Pinchart for the comments.

On 8/11/2025 8:31 PM, Laurent Pinchart wrote:
> On Mon, Aug 11, 2025 at 11:46:27AM +0000, Sakari Ailus wrote:
>> On Tue, Jul 29, 2025 at 05:12:03PM +0800, Du, Bin wrote:
>>> On 7/28/2025 3:23 PM, Sakari Ailus wrote:
>>>> 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.
>>
>> ...
> 
> Paragraphs are defined as a block of text that convey one idea. They
> should be visually separated by a space. As we can't have fractional
> line spacing in plain text, paragraphs need to be separated by a blank
> line. This is a typography rule that maximizes readability. There should
> be no line break between sentences in a single paragraph.
> 
> Whether you write commit messages, formal documentation or comments in
> code, typography is important to give the best experience to readers.
> After all, a block of text that wouldn't focus on the readers would have
> no reason to exist.
> 
> 
> Now compare the above with
> 
> 
> Paragraphs are defined as a block of text that convey one idea. They
> should be visually separated by a space.
> As we can't have fractional line spacing in plain text, paragraphs need
> to be separated by a blank line.
> This is a typography rule that maximizes readability. There should be no
> line break between sentences in a single paragraph. Whether you write
> commit messages, formal documentation or comments in code, typography is
> important to give the best experience to readers.
> After all, a block of text that wouldn't focus on the readers would have
> no reason to exist.
> 

Really appreciate the detailed guide, will follow it. May I summarize 
like this? 1 Separate paragraphs by a blank line. 2 Don't add line break 
between sentences in a single paragraph, an exception to this is commit 
message, because of the 75-character patch check limit, line break can 
be added, but it should at the 75-character limit boundary

>>>>> +	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;
>>>>> +}
> 

Powered by blists - more mailing lists