[PATCH] drm/shmem-helper: Fix comment describing dma-buf mappings

[Daniel Vetter]

On Tue, Jan 09, 2024 at 11:43:05AM +0100, Jacek Lawrynowicz wrote:
> `shmem->map_wc was` set to `false` but the comment suggested WC was
> enabled for imported buffers.
> 
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz at linux.intel.com>
> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index e435f986cd13..1532f1766170 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -75,7 +75,7 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
>  
>  	if (private) {
>  		drm_gem_private_object_init(dev, obj, size);
> -		shmem->map_wc = false; /* dma-buf mappings use always writecombine */
> +		shmem->map_wc = false; /* dma-buf mappings are never write-combined */

I think neither is correct, because for a dma_buf import it is up to the
importer to set up everything, including whether mappings should be
write-combined or not. And setting this to false ensures that helpers
don't muck around with the caching setting.

Also there's private buffer objects for other reasons, but the overlap
between drivers that have those and which use shmem helpers is zero.

So I think overall a better comment would be:

		/* This disables all shmem helper caching code and leaves
		 * all decision entirely to the buffer provider */

Maybe in a very old version where shmem helpers didn't correctly use the
dma_buf functions there was some justification for the original comment,
but that's been long ago fixed in a series of patches to make sure
dma_buf_vmap/mmap are used consistently and directly.

Care to respin with a wording of your choice for the comment? If you're
bored you could also try to dig through history a bit and collect some of
the commits that made this comment largely obsolete, since I don't think
any of the map_wc == true paths are even reachable anymore for private
objects ...

Cheers, Sima
>  	} else {
>  		ret = drm_gem_object_init(dev, obj, size);
>  	}
> -- 
> 2.43.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch