Re: [PATCH] drm/gem_shmem: Use a writecombine mapping for ->vaddr

From: Daniel Vetter <daniel@ffwll.ch>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Rob Herring <robh+dt@kernel.org>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/gem_shmem: Use a writecombine mapping for ->vaddr
Date: Wed, 29 May 2019 08:58:28 +0200	[thread overview]
Message-ID: <20190529065828.GU21222@phenom.ffwll.local> (raw)
In-Reply-To: <20190529065121.13485-1-boris.brezillon@collabora.com>

On Wed, May 29, 2019 at 08:51:21AM +0200, Boris Brezillon wrote:
> Right now, the BO is mapped as a cached region when ->vmap() is called
> and the underlying object is not a dmabuf.
> Doing that makes cache management a bit more complicated (you'd need
> to call dma_map/unmap_sg() on the ->sgt field everytime the BO is about
> to be passed to the GPU/CPU), so let's map the BO with writecombine
> attributes instead (as done in most drivers).
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Found this issue while working on panfrost perfcnt where the GPU dumps
> perf counter values in memory and the CPU reads them back in
> kernel-space. This patch seems to solve the unpredictable behavior I
> had.
> 
> I can also go for the other option (call dma_map/unmap/_sg() when
> needed) if you think that's more appropriate.

Uh, I guess shmem helpers (or gem helpers in general) need some concept
about what kind of cpu mapping is desired. Since some cpus (like e.g.
i915) do actually want cached mode for everything.

Same is needed for the userspace mmap, those should all agree.

Default probably best if we go with uncached.
-Daniel

> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 1ee208c2c85e..472ea5d81f82 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -255,7 +255,8 @@ static void *drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem)
>  	if (obj->import_attach)
>  		shmem->vaddr = dma_buf_vmap(obj->import_attach->dmabuf);
>  	else
> -		shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT, VM_MAP, PAGE_KERNEL);
> +		shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT,
> +				    VM_MAP, pgprot_writecombine(PAGE_KERNEL));
>  
>  	if (!shmem->vaddr) {
>  		DRM_DEBUG_KMS("Failed to vmap pages\n");
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel