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

[Jacek Lawrynowicz]

On 09.01.2024 14:14, Daniel Vetter wrote:
> 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 ...

I think that it would be better to add drm_WARN() here - similar to WARNs for import_attach.
Only v3d sets map_wc in gem_create_object() and it is easy to fix.
Would these warnings make sense?

__drm_gem_shmem_create():
drm_WARN(dev, shmem->map_wc, "Object caching is controlled by the underlying dma-buf\n");

__drm_gem_dma_create():
drm_WARN(dev, dma_obj->map_noncoherent, "Object caching is controlled by the underlying dma-buf\n");

Regards,
Jacek