Re: [PATCH v3 4/6] spi: spi-fsl-dspi: Use non-coherent memory for DMA

[James Clark <james.clark@linaro.org>]

On 25/06/2025 4:04 pm, Frank Li wrote:
> On Wed, Jun 25, 2025 at 10:00:41AM +0100, James Clark wrote:
>>
>>
>> On 24/06/2025 5:39 pm, Frank Li wrote:
>>> On Tue, Jun 24, 2025 at 11:35:34AM +0100, James Clark wrote:
>>>> Using coherent memory here isn't functionally necessary. Because the
>>>> change to use non-coherent memory isn't overly complex and only a few
>>>> synchronization points are required, we might as well do it while fixing
>>>> up some other DMA issues.
>>>>
>>>> Suggested-by: Arnd Bergmann <arnd@arndb.de>
>>>> Signed-off-by: James Clark <james.clark@linaro.org>
>>>
>>> In https://lore.kernel.org/imx/c65c752a-5b60-4f30-8d51-9a903ddd55a6@linaro.org/
>>>
>>> look like less performance, why need this patch.
>>>
>>> In https://lore.kernel.org/imx/ad7e9aa7-74a3-449d-8ed9-cb270fd5c718@linaro.org/
>>> look like better.
>>>
>>> Any conclusion?
>>>
>>> Need performance gain here if it is better.
>>>
>>> Frank
>>>
>>
>> Hi Frank,
>>
>> The performance figures for this set are in the cover letter. It's either
>> the same or faster, there is no evidence of worse performance. The one you
>> linked was a bad result from not testing it in DMA mode, but it's corrected
>> later in that thread.
> 
> Okay! you need mention why need this change, why non-coherent better than
> coherent at your case.
> 
> You descript what you already done, but not descript why need it.
> 
> for example:
> 
> "fixing up some other DMA issues", what issues exactly?

I can remove that line, it might be more appropriate to add in the cover 
letter as it's relating to other commits in this set.

> 
> some benefit, like memcpy from/to non-coherent is faster then from/to
> coherenct memory ...
> 

Yes I can mention that it's expected to be and was measured to be 
faster. That should help people running git log in the future to see why 
we did it.

> of put test data here will be appreciated.
> 
> The cover letter will be lost after patch merge. When someone run git log
> after some year later, they need know why need this change , what purpose ...
> 
> Frank
> 

I somewhat disagree with this. Usually maintainers add a 'Link:' to the 
mailing list when applying patches, so the cover letter shouldn't be 
lost. And these particular performance test results are short lived, in 
several years time other things may have changed. The performance is 
related to a specific device and the state of the rest of the kernel at 
this time. Additionally, I mentioned that it's the combination of two 
commits. In order to put figures on this commit message I would have to 
run another set of tests with only this commit and not the one to 
increase the buffer size which comes after. I did consider reversing the 
order of them to do this, but it wasn't straightforward, and I really 
didn't think it was worth the effort when I can just put the figures on 
the cover letter.

We only need the figures to judge whether to merge it right now, readers 
in the future will want to read the commit message to see what was done 
and why. I'm sure that they can trust that we measured some improvement 
if for some reason the cover letter is lost.

It's very common in the kernel to put figures relating to an entire 
patchset on the cover letter of it, rather than on the last commit message.

> 
>>
>> The reason the figures aren't in this commit is because it requires this
>> change and the one to increase the size of the buffer.
> 
> 
>>
>> Thanks
>> James
>>