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@xxxxxxxxxxxxxxx>
>> ---
>> 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
