Message-ID: <20250812073432.GF30054@pendragon.ideasonboard.com>
Date: Tue, 12 Aug 2025 10:34:32 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: "Du, Bin" <bin.du@....com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, 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

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.

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

Laurent Pinchart

Powered by blists - more mailing lists