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

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

Many thanks Laurent Pinchart

On 8/12/2025 3:34 PM, Laurent Pinchart wrote:
> On Tue, Aug 12, 2025 at 11:36:23AM +0800, Du, Bin wrote:
>> 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
> 
> When I wrote "line break", I meant breaking the line after a sentence,
> before the 75 columns limits. Text blocks should always be wrapped (at
> 75 columns in commit messages, or 80 in kernel code). What you should
> avoid is line breaks not related to the columns limit.
> 
> This is fine:
> 
> This paragraph has a long sentence that does not hold on a single line
> of 72 characters and therefore needs to be wrapped. There is no line
> break otherwise, for instance between the first and second sentence, or
> within a sentence.
> 
> This is not right:
> 
> This paragraph has a long sentence that does not hold on a single line
> of 72 characters and therefore needs to be wrapped.
> There is a line break between the first and second sentence,
> and also a line break in the second sentence, which are not fine.
> 

Really appreciate for your patient explanation and wonderful example, 
it's totally clear now.

>>>>>>> +	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,
Bin