public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Sui Jingfeng <sui.jingfeng@linux.dev>
To: Lucas Stach <l.stach@pengutronix.de>,
	Russell King <linux+etnaviv@armlinux.org.uk>,
	Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: David Airlie <airlied@gmail.com>, Simona Vetter <simona@ffwll.ch>,
	etnaviv@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org,
	Sui Jingfeng <sui.jingfeng@linux.dev>
Subject: [PATCH v3 1/3] drm/etnaviv: Drop the offset in page manipulation
Date: Fri, 15 Nov 2024 20:32:44 +0800	[thread overview]
Message-ID: <20241115123246.111346-2-sui.jingfeng@linux.dev> (raw)
In-Reply-To: <20241115123246.111346-1-sui.jingfeng@linux.dev>

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>
---
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
index 7e065b3723cf..c786df840a18 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);
-- 
2.34.1


  reply	other threads:[~2024-11-15 12:33 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-11-15 12:32 [PATCH v3 0/3] drm/etnaviv: Trivial mmu map and ummap cleanups Sui Jingfeng
2024-11-15 12:32 ` Sui Jingfeng [this message]
2024-11-15 12:32 ` [PATCH v3 2/3] drm/etnaviv: Fix the debug log of the etnaviv_iommu_map() Sui Jingfeng
2024-11-15 12:32 ` [PATCH v3 3/3] drm/etnaviv: Improve VA, PA, SIZE alignment checking Sui Jingfeng
2024-12-03 17:44 ` [PATCH v3 0/3] drm/etnaviv: Trivial mmu map and ummap cleanups Lucas Stach

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20241115123246.111346-2-sui.jingfeng@linux.dev \
    --to=sui.jingfeng@linux.dev \
    --cc=airlied@gmail.com \
    --cc=christian.gmeiner@gmail.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=etnaviv@lists.freedesktop.org \
    --cc=l.stach@pengutronix.de \
    --cc=linux+etnaviv@armlinux.org.uk \
    --cc=linux-kernel@vger.kernel.org \
    --cc=simona@ffwll.ch \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox