Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

12 participants
3141 discussions

Re: [Linaro-mm-sig] [PATCH 00/19] [Set 2] Rid W=1 warnings from GPU

by Sam Ravnborg

On Sat, Nov 07, 2020 at 06:41:38PM +0000, Lee Jones wrote: > On Sat, 07 Nov 2020, Sam Ravnborg wrote: > > > Hi Christian. > > > > > I'm not sure if we want to do some of the suggested changes to radeon. > > > > All patches for radeon looks good to me except "drm/radeon/radeon: Move > > prototype into shared header". > > Was that the one where the prototype needs moving to radeon.h? Yes, Sam

5 years

Re: [Linaro-mm-sig] [PATCH 00/19] [Set 2] Rid W=1 warnings from GPU

by Christian König

Well that's quite a patch set. First of all can you separate this a bit more by driver? I'm assuming we maintainers are supposed to pick that up and apply it. radeon and amdgpu can stick together since that is mostly Alex and me, but I'm not sure if we want to do some of the suggested changes to radeon. Going to pick up the single TTM change for upstreaming. Thanks, Christian. Am 06.11.20 um 22:49 schrieb Lee Jones: > This set is part of a larger effort attempting to clean-up W=1 > kernel builds, which are currently overwhelmingly riddled with > niggly little warnings. > > There are 5000 warnings to work through. It will take a couple more > sets. Although, ("drm/amd/display/dc/basics/fixpt31_32: Move > variables to where they're used") does take care of 2000 of them! > > Lee Jones (19): > drm/ttm/ttm_range_manager: Demote non-conformant kernel-doc header > drm/r128/ati_pcigart: Source file headers are not good candidates for > kernel-doc > drm/selftests/test-drm_dp_mst_helper: Move > 'sideband_msg_req_encode_decode' onto the heap > drm/mga/mga_dma: Demote kernel-doc abusers to standard comment blocks > drm/mga/mga_state: Remove unused variable 'buf_priv' > drm/radeon/atom: Move prototype into shared location > drm/radeon/radeon_kms: Include header containing our own prototypes > drm/omapdrm/omap_gem: Fix misnamed and missing parameter descriptions > drm/omapdrm/omap_dmm_tiler: Demote abusive use of kernel-doc format > drm/radeon/radeon: Move prototype into shared header > drm/radeon/radeon_drv: Source file headers are not good candidates for > kernel-doc > drm/amd/display/dc/basics/fixpt31_32: Move variables to where they're > used > drm/radeon/radeon_drv: Move prototypes to a shared headerfile > drm/amd/amdgpu/amdgpu_device: Provide documentation for 'reg_addr' > params > drm/radeon: Move prototypes to shared header > drm/amd/amdgpu/amdgpu_kms: Remove 'struct drm_amdgpu_info_device > dev_info' from the stack > drm/radeon/radeon_kms: Fix misnaming of 'radeon_info_ioctl's dev param > drm/radeon/atombios_crtc: Remove description of non-existent function > param 'encoder' > drm/v3d/v3d_drv: Remove unused static variable 'v3d_v3d_pm_ops' > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 + > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 104 +++++++++--------- > .../drm/amd/display/dc/basics/fixpt31_32.c | 5 + > .../gpu/drm/amd/display/include/fixed31_32.h | 6 - > drivers/gpu/drm/mga/mga_dma.c | 10 +- > drivers/gpu/drm/mga/mga_state.c | 2 - > drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 6 +- > drivers/gpu/drm/omapdrm/omap_gem.c | 3 +- > drivers/gpu/drm/r128/ati_pcigart.c | 2 +- > drivers/gpu/drm/radeon/atom.h | 6 + > drivers/gpu/drm/radeon/atombios_crtc.c | 1 - > drivers/gpu/drm/radeon/atombios_encoders.c | 4 - > drivers/gpu/drm/radeon/radeon.h | 6 + > drivers/gpu/drm/radeon/radeon_device.c | 1 + > drivers/gpu/drm/radeon/radeon_device.h | 32 ++++++ > drivers/gpu/drm/radeon/radeon_display.c | 4 - > drivers/gpu/drm/radeon/radeon_drv.c | 11 +- > drivers/gpu/drm/radeon/radeon_drv.h | 7 ++ > drivers/gpu/drm/radeon/radeon_kms.c | 3 +- > .../drm/selftests/test-drm_dp_mst_helper.c | 11 +- > drivers/gpu/drm/ttm/ttm_range_manager.c | 2 +- > drivers/gpu/drm/v3d/v3d_drv.c | 36 ------ > 22 files changed, 138 insertions(+), 126 deletions(-) > create mode 100644 drivers/gpu/drm/radeon/radeon_device.h > > Cc: Alex Deucher <alexander.deucher(a)amd.com> > Cc: amd-gfx(a)lists.freedesktop.org > Cc: Andy Gross <andy.gross(a)ti.com> > Cc: by <jhartmann(a)precisioninsight.com> > Cc: Christian Koenig <christian.koenig(a)amd.com> > Cc: "Christian König" <christian.koenig(a)amd.com> > Cc: Daniel Vetter <daniel(a)ffwll.ch> > Cc: David Airlie <airlied(a)linux.ie> > Cc: dri-devel(a)lists.freedesktop.org > Cc: Eric Anholt <eric(a)anholt.net> > Cc: Faith <faith(a)valinux.com> > Cc: Gareth Hughes <gareth(a)valinux.com> > Cc: Harry Wentland <harry.wentland(a)amd.com> > Cc: Huang Rui <ray.huang(a)amd.com> > Cc: Jeff Hartmann <jhartmann(a)valinux.com> > Cc: Keith Whitwell <keith(a)tungstengraphics.com> > Cc: Leo Li <sunpeng.li(a)amd.com> > Cc: linaro-mm-sig(a)lists.linaro.org > Cc: linux-media(a)vger.kernel.org > Cc: Philipp Zabel <p.zabel(a)pengutronix.de> > Cc: Rob Clark <rob.clark(a)linaro.org> > Cc: Rob Clark <rob(a)ti.com> > Cc: Sumit Semwal <sumit.semwal(a)linaro.org> > Cc: Tomi Valkeinen <tomi.valkeinen(a)ti.com>

5 years

Re: [Linaro-mm-sig] [PATCH 10/19] drm/radeon/radeon: Move prototype into shared header

by Sam Ravnborg

Hi Lee, On Fri, Nov 06, 2020 at 09:49:40PM +0000, Lee Jones wrote: > Unfortunately, a suitable one didn't already exist. > > Fixes the following W=1 kernel build warning(s): > > drivers/gpu/drm/radeon/radeon_device.c:637:6: warning: no previous prototype for ‘radeon_device_is_virtual’ [-Wmissing-prototypes] > 637 | bool radeon_device_is_virtual(void) > | ^~~~~~~~~~~~~~~~~~~~~~~~ > > Cc: Alex Deucher <alexander.deucher(a)amd.com> > Cc: "Christian König" <christian.koenig(a)amd.com> > Cc: David Airlie <airlied(a)linux.ie> > Cc: Daniel Vetter <daniel(a)ffwll.ch> > Cc: Sumit Semwal <sumit.semwal(a)linaro.org> > Cc: amd-gfx(a)lists.freedesktop.org > Cc: dri-devel(a)lists.freedesktop.org > Cc: linux-media(a)vger.kernel.org > Cc: linaro-mm-sig(a)lists.linaro.org > Signed-off-by: Lee Jones <lee.jones(a)linaro.org> > --- > drivers/gpu/drm/radeon/radeon_device.c | 1 + > drivers/gpu/drm/radeon/radeon_device.h | 32 ++++++++++++++++++++++++++ > drivers/gpu/drm/radeon/radeon_drv.c | 3 +-- > 3 files changed, 34 insertions(+), 2 deletions(-) > create mode 100644 drivers/gpu/drm/radeon/radeon_device.h Other public functions in radeon_device.c have their prototype in radeon.h - for example radeon_is_px() Add radeon_device_is_virtual() there so we avoiid this new header. Sam

5 years

Re: [Linaro-mm-sig] [PATCH 00/19] [Set 2] Rid W=1 warnings from GPU

by Sam Ravnborg

Hi Lee and DRM folks. On Fri, Nov 06, 2020 at 09:49:30PM +0000, Lee Jones wrote: > This set is part of a larger effort attempting to clean-up W=1 > kernel builds, which are currently overwhelmingly riddled with > niggly little warnings. > > There are 5000 warnings to work through. It will take a couple more > sets. Although, ("drm/amd/display/dc/basics/fixpt31_32: Move > variables to where they're used") does take care of 2000 of them! > > Lee Jones (19): > drm/ttm/ttm_range_manager: Demote non-conformant kernel-doc header > drm/r128/ati_pcigart: Source file headers are not good candidates for > kernel-doc Applied > drm/selftests/test-drm_dp_mst_helper: Move > 'sideband_msg_req_encode_decode' onto the heap > drm/mga/mga_dma: Demote kernel-doc abusers to standard comment blocks > drm/mga/mga_state: Remove unused variable 'buf_priv' Applied x2 > drm/radeon/atom: Move prototype into shared location > drm/radeon/radeon_kms: Include header containing our own prototypes > drm/omapdrm/omap_gem: Fix misnamed and missing parameter descriptions > drm/omapdrm/omap_dmm_tiler: Demote abusive use of kernel-doc format > drm/radeon/radeon: Move prototype into shared header > drm/radeon/radeon_drv: Source file headers are not good candidates for > kernel-doc > drm/amd/display/dc/basics/fixpt31_32: Move variables to where they're > used > drm/radeon/radeon_drv: Move prototypes to a shared headerfile > drm/amd/amdgpu/amdgpu_device: Provide documentation for 'reg_addr' > params > drm/radeon: Move prototypes to shared header > drm/amd/amdgpu/amdgpu_kms: Remove 'struct drm_amdgpu_info_device > dev_info' from the stack > drm/radeon/radeon_kms: Fix misnaming of 'radeon_info_ioctl's dev param > drm/radeon/atombios_crtc: Remove description of non-existent function > param 'encoder' > drm/v3d/v3d_drv: Remove unused static variable 'v3d_v3d_pm_ops' I have applied the three patches that has no obvious maintainer as indicated above. I assume the respective maintaines to pick radeon, omapdrm, ttm, amd, v3d and selftest patches. Sam

5 years

[PATCH 1/2] mm: mmap: fix fput in error path v2

by Christian König

Patch "495c10cc1c0c CHROMIUM: dma-buf: restore args..." adds a workaround for a bug in mmap_region. As the comment states ->mmap() callback can change vma->vm_file and so we might call fput() on the wrong file. Revert the workaround and proper fix this in mmap_region. v2: drop the extra if in dma_buf_mmap as well Signed-off-by: Christian König <christian.koenig(a)amd.com> Reviewed-by: Jason Gunthorpe <jgg(a)nvidia.com> --- drivers/dma-buf/dma-buf.c | 20 +++----------------- mm/mmap.c | 2 +- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index a6ba4d598f0e..08630d057cf2 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1143,9 +1143,6 @@ EXPORT_SYMBOL_GPL(dma_buf_end_cpu_access); int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, unsigned long pgoff) { - struct file *oldfile; - int ret; - if (WARN_ON(!dmabuf || !vma)) return -EINVAL; @@ -1163,22 +1160,11 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, return -EINVAL; /* readjust the vma */ - get_file(dmabuf->file); - oldfile = vma->vm_file; - vma->vm_file = dmabuf->file; + fput(vma->vm_file); + vma->vm_file = get_file(dmabuf->file); vma->vm_pgoff = pgoff; - ret = dmabuf->ops->mmap(dmabuf, vma); - if (ret) { - /* restore old parameters on failure */ - vma->vm_file = oldfile; - fput(dmabuf->file); - } else { - if (oldfile) - fput(oldfile); - } - return ret; - + return dmabuf->ops->mmap(dmabuf, vma); } EXPORT_SYMBOL_GPL(dma_buf_mmap); diff --git a/mm/mmap.c b/mm/mmap.c index 40248d84ad5f..3a2670d73355 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1852,8 +1852,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, return addr; unmap_and_free_vma: + fput(vma->vm_file); vma->vm_file = NULL; - fput(file); /* Undo any partial mapping done by a device driver. */ unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end); -- 2.17.1

5 years

[PATCH 5.9 267/391] drm/shme-helpers: Fix dma_buf_mmap forwarding bug

by Greg Kroah-Hartman

From: Daniel Vetter <daniel.vetter(a)ffwll.ch> commit f49a51bfdc8ea717c97ccd4cc98b7e6daaa5553a upstream. When we forward an mmap to the dma_buf exporter, they get to own everything. Unfortunately drm_gem_mmap_obj() overwrote vma->vm_private_data after the driver callback, wreaking the exporter complete. This was noticed because vb2_common_vm_close blew up on mali gpu with panfrost after commit 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf"). Unfortunately drm_gem_mmap_obj also acquires a surplus reference that we need to drop in shmem helpers, which is a bit of a mislayer situation. Maybe the entire dma_buf_mmap forwarding should be pulled into core gem code. Note that the only two other drivers which forward mmap in their own code (etnaviv and exynos) get this somewhat right by overwriting the gem mmap code. But they seem to still have the leak. This might be a good excuse to move these drivers over to shmem helpers completely. Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Acked-by: Christian König <christian.koenig(a)amd.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Lucas Stach <l.stach(a)pengutronix.de> Cc: Russell King <linux+etnaviv(a)armlinux.org.uk> Cc: Christian Gmeiner <christian.gmeiner(a)gmail.com> Cc: Inki Dae <inki.dae(a)samsung.com> Cc: Joonyoung Shim <jy0922.shim(a)samsung.com> Cc: Seung-Woo Kim <sw0312.kim(a)samsung.com> Cc: Kyungmin Park <kyungmin.park(a)samsung.com> Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf") Cc: Boris Brezillon <boris.brezillon(a)collabora.com> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Gerd Hoffmann <kraxel(a)redhat.com> Cc: Rob Herring <robh(a)kernel.org> Cc: dri-devel(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: <stable(a)vger.kernel.org> # v5.9+ Reported-and-tested-by: piotr.oniszczuk(a)gmail.com Cc: piotr.oniszczuk(a)gmail.com Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20201027214922.3566743-1-dani… Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- drivers/gpu/drm/drm_gem.c | 4 ++-- drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1085,6 +1085,8 @@ int drm_gem_mmap_obj(struct drm_gem_obje */ drm_gem_object_get(obj); + vma->vm_private_data = obj; + if (obj->funcs && obj->funcs->mmap) { ret = obj->funcs->mmap(obj, vma); if (ret) { @@ -1107,8 +1109,6 @@ int drm_gem_mmap_obj(struct drm_gem_obje vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); } - vma->vm_private_data = obj; - return 0; } EXPORT_SYMBOL(drm_gem_mmap_obj); --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -594,8 +594,13 @@ int drm_gem_shmem_mmap(struct drm_gem_ob /* Remove the fake offset */ vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node); - if (obj->import_attach) + if (obj->import_attach) { + /* Drop the reference drm_gem_mmap_obj() acquired.*/ + drm_gem_object_put(obj); + vma->vm_private_data = NULL; + return dma_buf_mmap(obj->dma_buf, vma, 0); + } shmem = to_drm_gem_shmem_obj(obj);

5 years

Patch "drm/shme-helpers: Fix dma_buf_mmap forwarding bug" has been added to the 5.9-stable tree

by gregkh＠linuxfoundation.org

This is a note to let you know that I've just added the patch titled drm/shme-helpers: Fix dma_buf_mmap forwarding bug to the 5.9-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum… The filename of the patch is: drm-shme-helpers-fix-dma_buf_mmap-forwarding-bug.patch and it can be found in the queue-5.9 subdirectory. If you, or anyone else, feels it should not be added to the stable tree, please let <stable(a)vger.kernel.org> know about it. >From f49a51bfdc8ea717c97ccd4cc98b7e6daaa5553a Mon Sep 17 00:00:00 2001 From: Daniel Vetter <daniel.vetter(a)ffwll.ch> Date: Tue, 27 Oct 2020 22:49:22 +0100 Subject: drm/shme-helpers: Fix dma_buf_mmap forwarding bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From: Daniel Vetter <daniel.vetter(a)ffwll.ch> commit f49a51bfdc8ea717c97ccd4cc98b7e6daaa5553a upstream. When we forward an mmap to the dma_buf exporter, they get to own everything. Unfortunately drm_gem_mmap_obj() overwrote vma->vm_private_data after the driver callback, wreaking the exporter complete. This was noticed because vb2_common_vm_close blew up on mali gpu with panfrost after commit 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf"). Unfortunately drm_gem_mmap_obj also acquires a surplus reference that we need to drop in shmem helpers, which is a bit of a mislayer situation. Maybe the entire dma_buf_mmap forwarding should be pulled into core gem code. Note that the only two other drivers which forward mmap in their own code (etnaviv and exynos) get this somewhat right by overwriting the gem mmap code. But they seem to still have the leak. This might be a good excuse to move these drivers over to shmem helpers completely. Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Acked-by: Christian König <christian.koenig(a)amd.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Lucas Stach <l.stach(a)pengutronix.de> Cc: Russell King <linux+etnaviv(a)armlinux.org.uk> Cc: Christian Gmeiner <christian.gmeiner(a)gmail.com> Cc: Inki Dae <inki.dae(a)samsung.com> Cc: Joonyoung Shim <jy0922.shim(a)samsung.com> Cc: Seung-Woo Kim <sw0312.kim(a)samsung.com> Cc: Kyungmin Park <kyungmin.park(a)samsung.com> Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf") Cc: Boris Brezillon <boris.brezillon(a)collabora.com> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Gerd Hoffmann <kraxel(a)redhat.com> Cc: Rob Herring <robh(a)kernel.org> Cc: dri-devel(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: <stable(a)vger.kernel.org> # v5.9+ Reported-and-tested-by: piotr.oniszczuk(a)gmail.com Cc: piotr.oniszczuk(a)gmail.com Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20201027214922.3566743-1-dani… Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- drivers/gpu/drm/drm_gem.c | 4 ++-- drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1085,6 +1085,8 @@ int drm_gem_mmap_obj(struct drm_gem_obje */ drm_gem_object_get(obj); + vma->vm_private_data = obj; + if (obj->funcs && obj->funcs->mmap) { ret = obj->funcs->mmap(obj, vma); if (ret) { @@ -1107,8 +1109,6 @@ int drm_gem_mmap_obj(struct drm_gem_obje vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); } - vma->vm_private_data = obj; - return 0; } EXPORT_SYMBOL(drm_gem_mmap_obj); --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -594,8 +594,13 @@ int drm_gem_shmem_mmap(struct drm_gem_ob /* Remove the fake offset */ vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node); - if (obj->import_attach) + if (obj->import_attach) { + /* Drop the reference drm_gem_mmap_obj() acquired.*/ + drm_gem_object_put(obj); + vma->vm_private_data = NULL; + return dma_buf_mmap(obj->dma_buf, vma, 0); + } shmem = to_drm_gem_shmem_obj(obj); Patches currently in stable-queue which might be from daniel.vetter(a)ffwll.ch are queue-5.9/drm-ast-separate-drm-driver-from-pci-code.patch queue-5.9/drm-shme-helpers-fix-dma_buf_mmap-forwarding-bug.patch

5 years

Re: [Linaro-mm-sig] WARNING in dma_map_page_attrs

by hch＠lst.de

On Tue, Oct 27, 2020 at 12:52:30PM +0000, Parav Pandit wrote: > > > From: hch(a)lst.de <hch(a)lst.de> > > Sent: Tuesday, October 27, 2020 1:41 PM > > > > On Mon, Oct 26, 2020 at 05:23:48AM +0000, Parav Pandit wrote: > > > Hi Christoph, > > > > > > > From: Jakub Kicinski <kuba(a)kernel.org> > > > > Sent: Saturday, October 24, 2020 11:45 PM > > > > > > > > CC: rdma, looks like rdma from the stack trace > > > > > > > > On Fri, 23 Oct 2020 20:07:17 -0700 syzbot wrote: > > > > > syzbot has found a reproducer for the following issue on: > > > > > > > > > > HEAD commit: 3cb12d27 Merge tag 'net-5.10-rc1' of > > git://git.kernel.org/.. > > > > > > In [1] you mentioned that dma_mask should not be set for dma_virt_ops. > > > So patch [2] removed it. > > > > > > But check to validate the dma mask for all dma_ops was added in [3]. > > > > > > What is the right way? Did I misunderstood your comment about > > dma_mask in [1]? > > > > No, I did not say we don't need the mask. I said copying over the various > > dma-related fields from the parent is bogus. > > > > I think rxe (and ther other drivers/infiniband/sw drivers) need a simple > > dma_coerce_mask_and_coherent and nothing else. > > I see. Does below fix make sense? > Is DMA_MASK_NONE correct? DMA_MASK_NONE is gone in 5.10. I think you want DMA_BIT_MASK(64). That isn't actually correct for 32-bit platforms, but good enough.

5 years

Re: [Linaro-mm-sig] [PATCH] drm/shme-helpers: Fix dma_buf_mmap forwarding bug

by Daniel Vetter

On Wed, Oct 28, 2020 at 09:44:15AM +0100, Boris Brezillon wrote: > On Tue, 27 Oct 2020 22:49:22 +0100 > Daniel Vetter <daniel.vetter(a)ffwll.ch> wrote: > > > When we forward an mmap to the dma_buf exporter, they get to own > > everything. Unfortunately drm_gem_mmap_obj() overwrote > > vma->vm_private_data after the driver callback, wreaking the > > exporter complete. This was noticed because vb2_common_vm_close blew > > up on mali gpu with panfrost after commit 26d3ac3cb04d > > ("drm/shmem-helpers: Redirect mmap for imported dma-buf"). > > > > Unfortunately drm_gem_mmap_obj also acquires a surplus reference that > > we need to drop in shmem helpers, which is a bit of a mislayer > > situation. Maybe the entire dma_buf_mmap forwarding should be pulled > > into core gem code. > > > > Note that the only two other drivers which forward mmap in their own > > code (etnaviv and exynos) get this somewhat right by overwriting the > > gem mmap code. But they seem to still have the leak. This might be a > > good excuse to move these drivers over to shmem helpers completely. > > > > Note to stable team: There's a trivial context conflict with > > d693def4fd1c ("drm: Remove obsolete GEM and PRIME callbacks from > > struct drm_driver"). I'm assuming it's clear where to put the first > > hunk, otherwise I can send a 5.9 version of this. > > > > Cc: Christian König <christian.koenig(a)amd.com> > > Cc: Sumit Semwal <sumit.semwal(a)linaro.org> > > Cc: Lucas Stach <l.stach(a)pengutronix.de> > > Cc: Russell King <linux+etnaviv(a)armlinux.org.uk> > > Cc: Christian Gmeiner <christian.gmeiner(a)gmail.com> > > Cc: Inki Dae <inki.dae(a)samsung.com> > > Cc: Joonyoung Shim <jy0922.shim(a)samsung.com> > > Cc: Seung-Woo Kim <sw0312.kim(a)samsung.com> > > Cc: Kyungmin Park <kyungmin.park(a)samsung.com> > > Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf") > > Cc: Boris Brezillon <boris.brezillon(a)collabora.com> > > Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Patch pushed to drm-misc-fixes, thanks for taking a look. -Daniel > > > Cc: Thomas Zimmermann <tzimmermann(a)suse.de> > > Cc: Gerd Hoffmann <kraxel(a)redhat.com> > > Cc: Rob Herring <robh(a)kernel.org> > > Cc: dri-devel(a)lists.freedesktop.org > > Cc: linux-media(a)vger.kernel.org > > Cc: linaro-mm-sig(a)lists.linaro.org > > Cc: <stable(a)vger.kernel.org> # v5.9+ > > Reported-and-tested-by: piotr.oniszczuk(a)gmail.com > > Cc: piotr.oniszczuk(a)gmail.com > > Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> > > --- > > drivers/gpu/drm/drm_gem.c | 4 ++-- > > drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++++- > > 2 files changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index 1da67d34e55d..d586068f5509 100644 > > --- a/drivers/gpu/drm/drm_gem.c > > +++ b/drivers/gpu/drm/drm_gem.c > > @@ -1076,6 +1076,8 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, > > */ > > drm_gem_object_get(obj); > > > > + vma->vm_private_data = obj; > > + > > if (obj->funcs->mmap) { > > ret = obj->funcs->mmap(obj, vma); > > if (ret) { > > @@ -1096,8 +1098,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, > > vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); > > } > > > > - vma->vm_private_data = obj; > > - > > return 0; > > } > > EXPORT_SYMBOL(drm_gem_mmap_obj); > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > > index fb11df7aced5..8233bda4692f 100644 > > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > > @@ -598,8 +598,13 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > > /* Remove the fake offset */ > > vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node); > > > > - if (obj->import_attach) > > + if (obj->import_attach) { > > + /* Drop the reference drm_gem_mmap_obj() acquired.*/ > > + drm_gem_object_put(obj); > > + vma->vm_private_data = NULL; > > + > > return dma_buf_mmap(obj->dma_buf, vma, 0); > > + } > > > > shmem = to_drm_gem_shmem_obj(obj); > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

[PATCH] drm/shme-helpers: Fix dma_buf_mmap forwarding bug

by Daniel Vetter

When we forward an mmap to the dma_buf exporter, they get to own everything. Unfortunately drm_gem_mmap_obj() overwrote vma->vm_private_data after the driver callback, wreaking the exporter complete. This was noticed because vb2_common_vm_close blew up on mali gpu with panfrost after commit 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf"). Unfortunately drm_gem_mmap_obj also acquires a surplus reference that we need to drop in shmem helpers, which is a bit of a mislayer situation. Maybe the entire dma_buf_mmap forwarding should be pulled into core gem code. Note that the only two other drivers which forward mmap in their own code (etnaviv and exynos) get this somewhat right by overwriting the gem mmap code. But they seem to still have the leak. This might be a good excuse to move these drivers over to shmem helpers completely. Note to stable team: There's a trivial context conflict with d693def4fd1c ("drm: Remove obsolete GEM and PRIME callbacks from struct drm_driver"). I'm assuming it's clear where to put the first hunk, otherwise I can send a 5.9 version of this. Cc: Christian König <christian.koenig(a)amd.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Lucas Stach <l.stach(a)pengutronix.de> Cc: Russell King <linux+etnaviv(a)armlinux.org.uk> Cc: Christian Gmeiner <christian.gmeiner(a)gmail.com> Cc: Inki Dae <inki.dae(a)samsung.com> Cc: Joonyoung Shim <jy0922.shim(a)samsung.com> Cc: Seung-Woo Kim <sw0312.kim(a)samsung.com> Cc: Kyungmin Park <kyungmin.park(a)samsung.com> Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf") Cc: Boris Brezillon <boris.brezillon(a)collabora.com> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Gerd Hoffmann <kraxel(a)redhat.com> Cc: Rob Herring <robh(a)kernel.org> Cc: dri-devel(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: <stable(a)vger.kernel.org> # v5.9+ Reported-and-tested-by: piotr.oniszczuk(a)gmail.com Cc: piotr.oniszczuk(a)gmail.com Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/drm_gem.c | 4 ++-- drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 1da67d34e55d..d586068f5509 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1076,6 +1076,8 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, */ drm_gem_object_get(obj); + vma->vm_private_data = obj; + if (obj->funcs->mmap) { ret = obj->funcs->mmap(obj, vma); if (ret) { @@ -1096,8 +1098,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); } - vma->vm_private_data = obj; - return 0; } EXPORT_SYMBOL(drm_gem_mmap_obj); diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index fb11df7aced5..8233bda4692f 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -598,8 +598,13 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) /* Remove the fake offset */ vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node); - if (obj->import_attach) + if (obj->import_attach) { + /* Drop the reference drm_gem_mmap_obj() acquired.*/ + drm_gem_object_put(obj); + vma->vm_private_data = NULL; + return dma_buf_mmap(obj->dma_buf, vma, 0); + } shmem = to_drm_gem_shmem_obj(obj); -- 2.28.0

5 years

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig