| Message ID | 20240109104305.604549-1-jacek.lawrynowicz@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/shmem-helper: Fix comment describing dma-buf mappings | expand |
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@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 >
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@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
On Wed, Jan 10, 2024 at 10:54:42AM +0100, Jacek Lawrynowicz wrote: > 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@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? I think v4c has the same pattern from a quick check ... But yes if you're willing to do the full audit, then this sounds like the cleanest approach. To document the results of your audit please explain for each driver why it's already save in the patch that adds the belwo warning (or that you've fixed it in a preceeding patch), that makes checking things in review easier. > __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"); Yeah this sounds good. Cheers, Sima
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 */ } else { ret = drm_gem_object_init(dev, obj, size); }
`shmem->map_wc was` set to `false` but the comment suggested WC was enabled for imported buffers. Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com> --- drivers/gpu/drm/drm_gem_shmem_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)