Re: Patch "drm/etnaviv: Drop the offset in page manipulation" has been added to the 6.12-stable tree

[Sui Jingfeng <sui.jingfeng@linux.dev>]

Hi,

On 2025/2/3 16:59, Lucas Stach wrote:
> Hi Sasha,
>
> Am Samstag, dem 01.02.2025 um 23:33 -0500 schrieb Sasha Levin:
>> This is a note to let you know that I've just added the patch titled
>>
>>      drm/etnaviv: Drop the offset in page manipulation
>>
>> to the 6.12-stable tree which can be found at:
>>      http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>>
>> The filename of the patch is:
>>       drm-etnaviv-drop-the-offset-in-page-manipulation.patch
>> and it can be found in the queue-6.12 subdirectory.
>>
>> If you, or anyone else, feels it should not be added to the stable tree,
>> please let <stable@vger.kernel.org> know about it.
>>
> please drop this patch and all its dependencies from all stable queues.
>
> While the code makes certain assumptions that are corrected in this
> patch, those assumptions are always true in all use-cases today.

Those patches are harmless even we apply them, and after apply my pitch,
it requires less CPU computation, right?


> I don't see a reason

I think, if 'sg->offset != 0' could happen  or not is really matters here.
My argument was that the real data is stored at 'sg_dma_address(sg)', NOT
the 'sg_dma_address(sg) - sg->offset'.


As we can create a test that we store some kind of data at the middle of
a BO by the CPU, then map this BO with the MMU and ask the GPU fetch the
data.  Do we really have a way tell the GPU to skip the leading garbage
data?


> to introduce this kind of churn to the stable trees


If I'm wrong or miss something, we can get them back, possibly with new
features, additional description, and comments for use-cases. My argument
just that we don't have good reasons to take the'sg->offset' into account
for now.
  

> to fix a theoretical issue.


The start PA of a buffer segment has been altered, but the corresponding
VA is not.

Maybe a approach has to guarantee correct in the theory first.

> Regards,
> Lucas
>
>>
>> commit cc5b6c4868e20f34d46e359930f0ca45a1cab9e3
>> Author: Sui Jingfeng <sui.jingfeng@linux.dev>
>> Date:   Fri Nov 15 20:32:44 2024 +0800
>>
>>      drm/etnaviv: Drop the offset in page manipulation
>>      
>>      [ Upstream commit 9aad03e7f5db7944d5ee96cd5c595c54be2236e6 ]
>>      
>>      The etnaviv driver, both kernel space and user space, assumes that GPU page
>>      size is 4KiB. Its IOMMU map/unmap 4KiB physical address range once a time.
>>      If 'sg->offset != 0' is true, then the current implementation will map the
>>      IOVA to a wrong area, which may lead to coherency problem. Picture 0 and 1
>>      give the illustration, see below.
>>      
>>        PA start drifted
>>        |
>>        |<--- 'sg_dma_address(sg) - sg->offset'
>>        |               .------ sg_dma_address(sg)
>>        |              |  .---- sg_dma_len(sg)
>>        |<-sg->offset->|  |
>>        V              |<-->|    Another one cpu page
>>        +----+----+----+----+   +----+----+----+----+
>>        |xxxx|         ||||||   |||||||||||||||||||||
>>        +----+----+----+----+   +----+----+----+----+
>>        ^                   ^   ^                   ^
>>        |<---   da_len  --->|   |                   |
>>        |                   |   |                   |
>>        |    .--------------'   |                   |
>>        |    | .----------------'                   |
>>        |    | |                   .----------------'
>>        |    | |                   |
>>        |    | +----+----+----+----+
>>        |    | |||||||||||||||||||||
>>        |    | +----+----+----+----+
>>        |    |
>>        |    '--------------.  da_len = sg_dma_len(sg) + sg->offset, using
>>        |                   |  'sg_dma_len(sg) + sg->offset' will lead to GPUVA
>>        +----+ ~~~~~~~~~~~~~+  collision, but min_t(unsigned int, da_len, va_len)
>>        |xxxx|              |  will clamp it to correct size. But the IOVA will
>>        +----+ ~~~~~~~~~~~~~+  be redirect to wrong area.
>>        ^
>>        |             Picture 0: Possibly wrong implementation.
>>      GPUVA (IOVA)
>>      
>>      --------------------------------------------------------------------------
>>      
>>                       .------- sg_dma_address(sg)
>>                       |  .---- sg_dma_len(sg)
>>        |<-sg->offset->|  |
>>        |              |<-->|    another one cpu page
>>        +----+----+----+----+   +----+----+----+----+
>>        |              ||||||   |||||||||||||||||||||
>>        +----+----+----+----+   +----+----+----+----+
>>                       ^    ^   ^                   ^
>>                       |    |   |                   |
>>        .--------------'    |   |                   |
>>        |                   |   |                   |
>>        |    .--------------'   |                   |
>>        |    | .----------------'                   |
>>        |    | |                   .----------------'
>>        |    | |                   |
>>        +----+ +----+----+----+----+
>>        |||||| ||||||||||||||||||||| The first one is SZ_4K, the second is SZ_16K
>>        +----+ +----+----+----+----+
>>        ^
>>        |           Picture 1: Perfectly correct implementation.
>>      GPUVA (IOVA)
>>      
>>      If sg->offset != 0 is true, IOVA will be mapped to wrong physical address.
>>      Either because there doesn't contain the data or there contains wrong data.
>>      Strictly speaking, the memory area that before sg_dma_address(sg) doesn't
>>      belong to us, and it's likely that the area is being used by other process.
>>      
>>      Because we don't want to introduce confusions about which part is visible
>>      to the GPU, we assumes that the size of GPUVA is always 4KiB aligned. This
>>      is very relaxed requirement, since we already made the decision that GPU
>>      page size is 4KiB (as a canonical decision). And softpin feature is landed,
>>      Mesa's util_vma_heap_alloc() will certainly report correct length of GPUVA
>>      to kernel with desired alignment ensured.
>>      
>>      With above statements agreed, drop the "offset in page" manipulation will
>>      return us a correct implementation at any case.
>>      
>>      Fixes: a8c21a5451d8 ("drm/etnaviv: add initial etnaviv DRM driver")
>>      Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>>      Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>>      Signed-off-by: Sasha Levin <sashal@kernel.org>
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> index a382920ae2be0..b7c09fc86a2cc 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> @@ -82,8 +82,8 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context,
>>   		return -EINVAL;
>>   
>>   	for_each_sgtable_dma_sg(sgt, sg, i) {
>> -		phys_addr_t pa = sg_dma_address(sg) - sg->offset;
>> -		unsigned int da_len = sg_dma_len(sg) + sg->offset;
>> +		phys_addr_t pa = sg_dma_address(sg);
>> +		unsigned int da_len = sg_dma_len(sg);
>>   		unsigned int bytes = min_t(unsigned int, da_len, va_len);
>>   
>>   		VERB("map[%d]: %08x %pap(%x)", i, iova, &pa, bytes);

-- 
Best regards,
Sui