From: Steven Price <steven.price@arm.com> To: Robin Murphy <robin.murphy@arm.com>, Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>, Rob Herring <robh@kernel.org>, Tomeu Vizoso <tomeu.vizoso@collabora.com> Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> Subject: Re: [PATCH] drm/panfrost: DMA map all pages shared with the GPU Date: Mon, 14 Oct 2019 17:28:27 +0100 [thread overview] Message-ID: <0cfd8582-b4e1-d429-7db8-23814b063403@arm.com> (raw) In-Reply-To: <8f8bd089-102f-9b8b-335b-6be06687d0ac@arm.com> On 14/10/2019 16:55, Steven Price wrote: > On 14/10/2019 16:46, Robin Murphy wrote: >> On 14/10/2019 16:16, Steven Price wrote: >>> Pages shared with the GPU are (often) not cache coherent with the CPU so >>> cache maintenance is required to flush the CPU's caches. This was >>> already done when mapping pages on fault, but wasn't previously done >>> when mapping a freshly allocated page. >>> >>> Fix this by moving the call to dma_map_sg() into mmu_map_sg() ensuring >>> that it is always called when pages are mapped onto the GPU. Since >>> mmu_map_sg() can now fail the code also now has to handle an error >>> return. >>> >>> Not performing this cache maintenance can cause errors in the GPU output >>> (CPU caches are later flushed and overwrite the GPU output). In theory >>> it also allows the GPU (and by extension user space) to observe the >>> memory contents prior to sanitization. >> >> For the non-heap case, aren't the pages already supposed to be mapped by >> drm_gem_shmem_get_pages_sgt()? > > Hmm, well yes - it looks like it *should* work - but I was definitely > seeing cache artefacts until I did this change... let me do some more > testing. It's possible that this is actually only affecting buffers > imported from another driver. Perhaps it's > drm_gem_shmem_prime_import_sg_table() that needs fixing. Yes this does appear to only affect imported buffers from what I can tell. Looking through the code there is something suspicious in drm_gem_map_dma_buf(). The following "fixes" the problem. But I'm not sure the reasoning behind DMA_ATTR_SKIP_CPU_SYNC being specified - presumably someone though it was a good idea! I'm not sure which driver's responsibility it is to ensure the caches are flushed. ---8<---- diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 0a2316e0e812..1f7353abd255 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -625,7 +625,7 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach, sgt = obj->dev->driver->gem_prime_get_sg_table(obj); if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir, - DMA_ATTR_SKIP_CPU_SYNC)) { + 0)) { sg_free_table(sgt); kfree(sgt); sgt = ERR_PTR(-ENOMEM);
WARNING: multiple messages have this Message-ID (diff)
From: Steven Price <steven.price@arm.com> To: Robin Murphy <robin.murphy@arm.com>, Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>, Rob Herring <robh@kernel.org>, Tomeu Vizoso <tomeu.vizoso@collabora.com> Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> Subject: Re: [PATCH] drm/panfrost: DMA map all pages shared with the GPU Date: Mon, 14 Oct 2019 17:28:27 +0100 [thread overview] Message-ID: <0cfd8582-b4e1-d429-7db8-23814b063403@arm.com> (raw) In-Reply-To: <8f8bd089-102f-9b8b-335b-6be06687d0ac@arm.com> On 14/10/2019 16:55, Steven Price wrote: > On 14/10/2019 16:46, Robin Murphy wrote: >> On 14/10/2019 16:16, Steven Price wrote: >>> Pages shared with the GPU are (often) not cache coherent with the CPU so >>> cache maintenance is required to flush the CPU's caches. This was >>> already done when mapping pages on fault, but wasn't previously done >>> when mapping a freshly allocated page. >>> >>> Fix this by moving the call to dma_map_sg() into mmu_map_sg() ensuring >>> that it is always called when pages are mapped onto the GPU. Since >>> mmu_map_sg() can now fail the code also now has to handle an error >>> return. >>> >>> Not performing this cache maintenance can cause errors in the GPU output >>> (CPU caches are later flushed and overwrite the GPU output). In theory >>> it also allows the GPU (and by extension user space) to observe the >>> memory contents prior to sanitization. >> >> For the non-heap case, aren't the pages already supposed to be mapped by >> drm_gem_shmem_get_pages_sgt()? > > Hmm, well yes - it looks like it *should* work - but I was definitely > seeing cache artefacts until I did this change... let me do some more > testing. It's possible that this is actually only affecting buffers > imported from another driver. Perhaps it's > drm_gem_shmem_prime_import_sg_table() that needs fixing. Yes this does appear to only affect imported buffers from what I can tell. Looking through the code there is something suspicious in drm_gem_map_dma_buf(). The following "fixes" the problem. But I'm not sure the reasoning behind DMA_ATTR_SKIP_CPU_SYNC being specified - presumably someone though it was a good idea! I'm not sure which driver's responsibility it is to ensure the caches are flushed. ---8<---- diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 0a2316e0e812..1f7353abd255 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -625,7 +625,7 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach, sgt = obj->dev->driver->gem_prime_get_sg_table(obj); if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir, - DMA_ATTR_SKIP_CPU_SYNC)) { + 0)) { sg_free_table(sgt); kfree(sgt); sgt = ERR_PTR(-ENOMEM); _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-10-14 16:28 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-10-14 15:16 [PATCH] drm/panfrost: DMA map all pages shared with the GPU Steven Price 2019-10-14 15:46 ` Robin Murphy 2019-10-14 15:55 ` Steven Price 2019-10-14 16:28 ` Steven Price [this message] 2019-10-14 16:28 ` Steven Price
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=0cfd8582-b4e1-d429-7db8-23814b063403@arm.com \ --to=steven.price@arm.com \ --cc=airlied@linux.ie \ --cc=alyssa.rosenzweig@collabora.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=robh@kernel.org \ --cc=robin.murphy@arm.com \ --cc=tomeu.vizoso@collabora.com \ /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: linkBe 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.