Skip to contentSkip to conversation

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

DISCUSSION Lucas Stach 3 participants 7 messages Feb 3, 2025 – Feb 4, 2025 1 message not in the archive Stable branch

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Lucas Stach

author
#1 lore raw

Hi Sasha,

Am Samstag, dem 01.02.2025 um 23:33 -0500 schrieb Sasha Levin:

Quoted 7 lines

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

Quoted 7 lines

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 <[email protected]> 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. I don't see a reason to introduce this kind of churn to the stable trees to fix a theoretical issue.

Regards,
Lucas

Quoted 105 lines

commit cc5b6c4868e20f34d46e359930f0ca45a1cab9e3

Author: Sui Jingfeng <[email protected]>
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 <[email protected]>
    Signed-off-by: Lucas Stach <[email protected]>
    Signed-off-by: Sasha Levin <[email protected]>
drivers/gpu/drm/etnaviv/etnaviv_mmu.c +2 -2
index a382920ae2be0..b7c09fc86a2cc 100644
@@ -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);
maintainer
#2 lore raw
Quoted 17 lines · Lucas Stach

Hi Sasha,

Am Samstag, dem 01.02.2025 um 23:33 -0500 schrieb Sasha Levin:

Quoted 7 lines

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

Quoted 7 lines

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 <[email protected]> know about it.

Quoted 6 lines

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. I don't see a reason to introduce this kind of churn to the stable trees to fix a theoretical issue.

Maybe in the future, for "theoretical issues", please don't put a "Fixes:" tag on them?

thanks,

greg k-h

author
#3 lore raw

Am Montag, dem 03.02.2025 um 10:29 +0100 schrieb Greg KH:

Quoted 20 lines · Lucas Stach
Quoted 19 lines

Hi Sasha,

Am Samstag, dem 01.02.2025 um 23:33 -0500 schrieb Sasha Levin:

Quoted 7 lines

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

Quoted 7 lines

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 <[email protected]> know about it.

please drop this patch and all its dependencies from all stable queues.

Quoted 7 lines
Quoted 4 lines

While the code makes certain assumptions that are corrected in this patch, those assumptions are always true in all use-cases today. I don't see a reason to introduce this kind of churn to the stable trees to fix a theoretical issue.

Maybe in the future, for "theoretical issues", please don't put a "Fixes:" tag on them?

Agreed. This tag slipped through when I applied this patch, I'll be more careful next time.

Regards,
Lucas

maintainer
#4 lore raw
Quoted 26 lines · Lucas Stach

Am Montag, dem 03.02.2025 um 10:29 +0100 schrieb Greg KH:

Quoted 25 lines · Lucas Stach
Quoted 23 lines

Hi Sasha,

Am Samstag, dem 01.02.2025 um 23:33 -0500 schrieb Sasha Levin:

Quoted 7 lines

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

Quoted 7 lines

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 <[email protected]> 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. I don't see a reason to introduce this kind of churn to the stable trees to fix a theoretical issue.

Quoted 5 lines
Quoted 2 lines

Maybe in the future, for "theoretical issues", please don't put a "Fixes:" tag on them?

Agreed. This tag slipped through when I applied this patch, I'll be more careful next time.

Ok, all now dropped, thanks.

greg k-h

#5 lore raw
In reply to Lucas Stach

Hi,

Quoted 14 lines · Lucas Stach

Hi Sasha,

Am Samstag, dem 01.02.2025 um 23:33 -0500 schrieb Sasha Levin:

Quoted 11 lines

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.

Quoted 7 lines
Quoted 3 lines

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> 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?

Quoted 1 line

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?

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 107 lines

Regards,
Lucas

Quoted 104 lines

commit cc5b6c4868e20f34d46e359930f0ca45a1cab9e3

Author: Sui Jingfeng <[email protected]>
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 <[email protected]>
     Signed-off-by: Lucas Stach <[email protected]>
     Signed-off-by: Sasha Levin <[email protected]>
drivers/gpu/drm/etnaviv/etnaviv_mmu.c +2 -2
index a382920ae2be0..b7c09fc86a2cc 100644
@@ -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);
Signature · 2 lines
Best regards,
Sui
author
#6 lore raw

Hi Sui,

Am Montag, dem 03.02.2025 um 18:53 +0800 schrieb Sui Jingfeng:

Quoted 51 lines

Hi,

Quoted 14 lines · Lucas Stach

Hi Sasha,

Am Samstag, dem 01.02.2025 um 23:33 -0500 schrieb Sasha Levin:

Quoted 11 lines

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.

Quoted 7 lines
Quoted 3 lines

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> 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?

Quoted 1 line

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?

Quoted 1 line

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.

Quoted 7 lines
Quoted 1 line

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.

I'm aware that one could construct cases where things fall over with the previous code. However, there is no such case in practice. I fully agree that this issue should be fixed, which is obviously why I merged the patch.

I do not agree to introduce churn into the stable trees and burden myself and others to check the correctness of the backports (just because patches do apply to the stable tree does not mean all their prerequisites and underlying assumptions are met), to fix a theoretical issue.

Regards,
Lucas

#7 lore raw

Hi,

Quoted 62 lines · Lucas Stach

Hi Sui,

Am Montag, dem 03.02.2025 um 18:53 +0800 schrieb Sui Jingfeng:

Quoted 48 lines

Hi,

Quoted 14 lines · Lucas Stach

Hi Sasha,

Am Samstag, dem 01.02.2025 um 23:33 -0500 schrieb Sasha Levin:

Quoted 11 lines

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.

Quoted 7 lines
Quoted 3 lines

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> 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?

Quoted 1 line

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?

Quoted 1 line

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.

Quoted 6 lines
Quoted 1 line

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.

I'm aware that one could construct cases where things fall over with the previous code. However, there is no such case in practice. I fully agree that this issue should be fixed, which is obviously why I merged the patch.

Quoted 4 lines

I do not agree to introduce churn into the stable trees and burden myself and others to check the correctness of the backports (just because patches do apply to the stable tree does not mean all their prerequisites and underlying assumptions are met),

The core problem still is, whether 'sg->offset != 0' will happen or not. Without answering this fundamental question I don't think the rest murmurs make technical sense.

The patch is about to stop touching unreasonable part, if stable branches has raised any problem because of this single patch, I think I could help to resolve.

Quoted 1 line

to fix a theoretical issue.

This is concept, not theoretical.

Quoted 2 lines

Regards,
Lucas

Signature · 2 lines
Best regards,
Sui