Re: [PATCH] drm/panfrost: DMA map all pages shared with the GPU

[Steven Price <steven.price@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);