All of lore.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 1/2] drm/etnaviv: Fix misunderstanding about the scatterlist structure
Date: Tue, 29 Oct 2024 00:05:54 +0800	[thread overview]
Message-ID: <20241028160555.1006559-1-sui.jingfeng@linux.dev> (raw)

The 'offset' data member of the 'struct scatterlist' denotes the offset
into a SG entry in bytes. The sg_dma_len() macro could be used to get
lengths of SG entries, those lengths are expected to be CPU page size
aligned. Since, at least for now, we call drm_prime_pages_to_sg() to
convert our various page array into an SG list. We pass the number of
CPU page as the third argoument, to tell the size of the backing memory
of GEM buffer object.

drm_prime_pages_to_sg() call sg_alloc_table_from_pages_segment() do the
work, sg_alloc_table_from_pages_segment() always hardcode the Offset to
ZERO. The sizes of *all* SG enties will be a multiple of CPU page size,
that is multiple of PAGE_SIZE.

If the GPU want to map/unmap a bigger page partially, we should use
'sg_dma_address(sg) + sg->offset' to calculate the destination DMA
address, and the size to be map/unmap is 'sg_dma_len(sg) - sg->offset'.

While the current implement is wrong, but since the 'sg->offset' is
alway equal to 0, drm/etnaviv works in practice by good luck. Fix this,
to make it looks right at least from the perspective of concept.

while at it, always fix the absue types:

- sg_dma_address returns DMA address, the type is dma_addr_t, not
  the phys_addr_t, for VRAM there may have another translation between
  the bus address and the final physical address of VRAM or carved out
  RAM.

- The type of sg_dma_len(sg) return is unsigned int, not the size_t.
  Avoid hint the compiler to do unnecessary integer promotion.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
index 1661d589bf3e..4ee9ed96b1d8 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
@@ -80,10 +80,10 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
 		return -EINVAL;
 
 	for_each_sgtable_dma_sg(sgt, sg, i) {
-		phys_addr_t pa = sg_dma_address(sg) - sg->offset;
-		size_t bytes = sg_dma_len(sg) + sg->offset;
+		dma_addr_t pa = sg_dma_address(sg) + sg->offset;
+		unsigned int bytes = sg_dma_len(sg) - sg->offset;
 
-		VERB("map[%d]: %08x %pap(%zx)", i, iova, &pa, bytes);
+		VERB("map[%d]: %08x %pap(%x)", i, iova, &pa, bytes);
 
 		ret = etnaviv_context_map(context, da, pa, bytes, prot);
 		if (ret)
@@ -109,11 +109,11 @@ static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova,
 	int i;
 
 	for_each_sgtable_dma_sg(sgt, sg, i) {
-		size_t bytes = sg_dma_len(sg) + sg->offset;
+		unsigned int bytes = sg_dma_len(sg) - sg->offset;
 
 		etnaviv_context_unmap(context, da, bytes);
 
-		VERB("unmap[%d]: %08x(%zx)", i, iova, bytes);
+		VERB("unmap[%d]: %08x(%x)", i, iova, bytes);
 
 		BUG_ON(!PAGE_ALIGNED(bytes));
 
-- 
2.34.1


             reply	other threads:[~2024-10-28 16:06 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-28 16:05 Sui Jingfeng [this message]
2024-10-28 16:05 ` [PATCH 2/2] drm/etnaviv: Fix the debug log for the mmu map/unmap procudure Sui Jingfeng
2024-10-28 16:34 ` [PATCH 1/2] drm/etnaviv: Fix misunderstanding about the scatterlist structure Sui Jingfeng
2024-10-28 17:27 ` Sui Jingfeng

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=20241028160555.1006559-1-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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.