Message-ID: <b94236b7-3a56-45f8-a085-9ca661b0f43c@amd.com>
Date: Tue, 12 Aug 2025 18:04:41 +0800
From: "Du, Bin" <bin.du@....com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Laurent Pinchart <laurent.pinchart@...asonboard.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

Hi Sakari Ailus,

On 8/12/2025 4:20 PM, Sakari Ailus wrote:
> Hi Laurent, Bin,
> 
> On Tue, Aug 12, 2025 at 10:34:32AM +0300, 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.
> 
> I wonder if this should make it to kernel documentation. I've come across
> many new developers recently who would definitely benefit from this.
> 

Yes, totally agree, furthermore, if checkpatch script can be 
strengthened to add this kind of check, developers can even find and fix 
them before submit the patches

> Also, most editors can rewrap paragraphs of text (e.g. Emacs M-q or Joe C-k
> C-j).
> 

-- 
Regards,
Bin

Powered by blists - more mailing lists