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@xxxxxxxxxxxxxxx> 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 > > > commit cc5b6c4868e20f34d46e359930f0ca45a1cab9e3 > Author: Sui Jingfeng <sui.jingfeng@xxxxxxxxx> > 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@xxxxxxxxx> > Signed-off-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx> > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx> > > 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);
