| Message ID | 20210115093038.10345-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm: Set vm_ops to GEM object's values during mmap | expand |
Hi Thomas, On 15/01/2021 09:30, Thomas Zimmermann wrote: > The GEM mmap code relies on the GEM object's mmap callback to set the > VMA's vm_ops field. This is easily forgotten and already led to a memory > leak in the CMA helpers. Instead set the vm_ops field in the DRM core > code to the GEM object's value. Drivers with different needs can override > this in their mmap callback. > > v2: > * support (vm_ops == NULL) if mmap is given; required by VRAM > helpers > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Fixes: f5ca8eb6f9bd ("drm/cma-helper: Implement mmap as GEM CMA object functions") > Reported-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Re-tested just fine this side ;-) - https://paste.ubuntu.com/p/Jgz6xMKNJX/ Thanks Kieran > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Eric Anholt <eric@anholt.net> > Cc: dri-devel@lists.freedesktop.org > --- > drivers/gpu/drm/drm_gem.c | 19 ++++++++++--------- > drivers/gpu/drm/drm_prime.c | 2 ++ > 2 files changed, 12 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 34b2f111c01c..c2ce78c4edc3 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -1068,20 +1068,17 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, > drm_gem_object_get(obj); > > vma->vm_private_data = obj; > + vma->vm_ops = obj->funcs->vm_ops; > > if (obj->funcs->mmap) { > ret = obj->funcs->mmap(obj, vma); > - if (ret) { > - drm_gem_object_put(obj); > - return ret; > - } > + if (ret) > + goto err_drm_gem_object_put; > WARN_ON(!(vma->vm_flags & VM_DONTEXPAND)); > } else { > - if (obj->funcs->vm_ops) > - vma->vm_ops = obj->funcs->vm_ops; > - else { > - drm_gem_object_put(obj); > - return -EINVAL; > + if (!vma->vm_ops) { > + ret = -EINVAL; > + goto err_drm_gem_object_put; > } > > vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; > @@ -1090,6 +1087,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, > } > > return 0; > + > +err_drm_gem_object_put: > + drm_gem_object_put(obj); > + return ret; > } > EXPORT_SYMBOL(drm_gem_mmap_obj); > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 683aa29ecd3b..2a54f86856af 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -717,6 +717,8 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); > > if (obj->funcs && obj->funcs->mmap) { > + vma->vm_ops = obj->funcs->vm_ops; > + > ret = obj->funcs->mmap(obj, vma); > if (ret) > return ret; >
On Fri, Jan 15, 2021 at 09:57:24AM +0000, Kieran Bingham wrote: > Hi Thomas, > > On 15/01/2021 09:30, Thomas Zimmermann wrote: > > The GEM mmap code relies on the GEM object's mmap callback to set the > > VMA's vm_ops field. This is easily forgotten and already led to a memory > > leak in the CMA helpers. Instead set the vm_ops field in the DRM core > > code to the GEM object's value. Drivers with different needs can override > > this in their mmap callback. > > > > v2: > > * support (vm_ops == NULL) if mmap is given; required by VRAM > > helpers I guess vram helpers need this because ttm has it's own vm_ops struct? Might be another thing worth unifying (eventually). > > > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > Fixes: f5ca8eb6f9bd ("drm/cma-helper: Implement mmap as GEM CMA object functions") > > Reported-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > Re-tested just fine this side ;-) > - https://paste.ubuntu.com/p/Jgz6xMKNJX/ Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Thanks > > Kieran > > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Eric Anholt <eric@anholt.net> > > Cc: dri-devel@lists.freedesktop.org > > --- > > drivers/gpu/drm/drm_gem.c | 19 ++++++++++--------- > > drivers/gpu/drm/drm_prime.c | 2 ++ > > 2 files changed, 12 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index 34b2f111c01c..c2ce78c4edc3 100644 > > --- a/drivers/gpu/drm/drm_gem.c > > +++ b/drivers/gpu/drm/drm_gem.c > > @@ -1068,20 +1068,17 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, > > drm_gem_object_get(obj); > > > > vma->vm_private_data = obj; > > + vma->vm_ops = obj->funcs->vm_ops; > > > > if (obj->funcs->mmap) { > > ret = obj->funcs->mmap(obj, vma); > > - if (ret) { > > - drm_gem_object_put(obj); > > - return ret; > > - } > > + if (ret) > > + goto err_drm_gem_object_put; > > WARN_ON(!(vma->vm_flags & VM_DONTEXPAND)); > > } else { > > - if (obj->funcs->vm_ops) > > - vma->vm_ops = obj->funcs->vm_ops; > > - else { > > - drm_gem_object_put(obj); > > - return -EINVAL; > > + if (!vma->vm_ops) { > > + ret = -EINVAL; > > + goto err_drm_gem_object_put; > > } > > > > vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; > > @@ -1090,6 +1087,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, > > } > > > > return 0; > > + > > +err_drm_gem_object_put: > > + drm_gem_object_put(obj); > > + return ret; > > } > > EXPORT_SYMBOL(drm_gem_mmap_obj); > > > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > > index 683aa29ecd3b..2a54f86856af 100644 > > --- a/drivers/gpu/drm/drm_prime.c > > +++ b/drivers/gpu/drm/drm_prime.c > > @@ -717,6 +717,8 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > > vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); > > > > if (obj->funcs && obj->funcs->mmap) { > > + vma->vm_ops = obj->funcs->vm_ops; > > + Do you know how much we still need the non-obj->funcs path here? Maybe time to detele it and wrape the obj->funcs check in a WARN_ON? -Daniel > > ret = obj->funcs->mmap(obj, vma); > > if (ret) > > return ret; > > >
Hi Am 15.01.21 um 15:11 schrieb Daniel Vetter: > On Fri, Jan 15, 2021 at 09:57:24AM +0000, Kieran Bingham wrote: >> Hi Thomas, >> >> On 15/01/2021 09:30, Thomas Zimmermann wrote: >>> The GEM mmap code relies on the GEM object's mmap callback to set the >>> VMA's vm_ops field. This is easily forgotten and already led to a memory >>> leak in the CMA helpers. Instead set the vm_ops field in the DRM core >>> code to the GEM object's value. Drivers with different needs can override >>> this in their mmap callback. >>> >>> v2: >>> * support (vm_ops == NULL) if mmap is given; required by VRAM >>> helpers > > I guess vram helpers need this because ttm has it's own vm_ops struct? > Might be another thing worth unifying (eventually). I've been working on converting drivers to struct drm_gem_object_funcs.mmap and using DRM helpers for the other mmap callbacks. Part of this would include some unifying of the vm_ops handling. TTM drivers are kind of special here. Best regards Thomas > >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> Fixes: f5ca8eb6f9bd ("drm/cma-helper: Implement mmap as GEM CMA object functions") >>> Reported-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >>> Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >> >> Re-tested just fine this side ;-) >> - https://paste.ubuntu.com/p/Jgz6xMKNJX/ > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> >> Thanks >> >> Kieran >> >>> Cc: Maxime Ripard <mripard@kernel.org> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Cc: David Airlie <airlied@linux.ie> >>> Cc: Daniel Vetter <daniel@ffwll.ch> >>> Cc: Eric Anholt <eric@anholt.net> >>> Cc: dri-devel@lists.freedesktop.org >>> --- >>> drivers/gpu/drm/drm_gem.c | 19 ++++++++++--------- >>> drivers/gpu/drm/drm_prime.c | 2 ++ >>> 2 files changed, 12 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >>> index 34b2f111c01c..c2ce78c4edc3 100644 >>> --- a/drivers/gpu/drm/drm_gem.c >>> +++ b/drivers/gpu/drm/drm_gem.c >>> @@ -1068,20 +1068,17 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, >>> drm_gem_object_get(obj); >>> >>> vma->vm_private_data = obj; >>> + vma->vm_ops = obj->funcs->vm_ops; >>> >>> if (obj->funcs->mmap) { >>> ret = obj->funcs->mmap(obj, vma); >>> - if (ret) { >>> - drm_gem_object_put(obj); >>> - return ret; >>> - } >>> + if (ret) >>> + goto err_drm_gem_object_put; >>> WARN_ON(!(vma->vm_flags & VM_DONTEXPAND)); >>> } else { >>> - if (obj->funcs->vm_ops) >>> - vma->vm_ops = obj->funcs->vm_ops; >>> - else { >>> - drm_gem_object_put(obj); >>> - return -EINVAL; >>> + if (!vma->vm_ops) { >>> + ret = -EINVAL; >>> + goto err_drm_gem_object_put; >>> } >>> >>> vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; >>> @@ -1090,6 +1087,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, >>> } >>> >>> return 0; >>> + >>> +err_drm_gem_object_put: >>> + drm_gem_object_put(obj); >>> + return ret; >>> } >>> EXPORT_SYMBOL(drm_gem_mmap_obj); >>> >>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >>> index 683aa29ecd3b..2a54f86856af 100644 >>> --- a/drivers/gpu/drm/drm_prime.c >>> +++ b/drivers/gpu/drm/drm_prime.c >>> @@ -717,6 +717,8 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) >>> vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); >>> >>> if (obj->funcs && obj->funcs->mmap) { >>> + vma->vm_ops = obj->funcs->vm_ops; >>> + > > Do you know how much we still need the non-obj->funcs path here? Maybe > time to detele it and wrape the obj->funcs check in a WARN_ON? > -Daniel > > >>> ret = obj->funcs->mmap(obj, vma); >>> if (ret) >>> return ret; >>> >> >
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 34b2f111c01c..c2ce78c4edc3 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1068,20 +1068,17 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, drm_gem_object_get(obj); vma->vm_private_data = obj; + vma->vm_ops = obj->funcs->vm_ops; if (obj->funcs->mmap) { ret = obj->funcs->mmap(obj, vma); - if (ret) { - drm_gem_object_put(obj); - return ret; - } + if (ret) + goto err_drm_gem_object_put; WARN_ON(!(vma->vm_flags & VM_DONTEXPAND)); } else { - if (obj->funcs->vm_ops) - vma->vm_ops = obj->funcs->vm_ops; - else { - drm_gem_object_put(obj); - return -EINVAL; + if (!vma->vm_ops) { + ret = -EINVAL; + goto err_drm_gem_object_put; } vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; @@ -1090,6 +1087,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, } return 0; + +err_drm_gem_object_put: + drm_gem_object_put(obj); + return ret; } EXPORT_SYMBOL(drm_gem_mmap_obj); diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 683aa29ecd3b..2a54f86856af 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -717,6 +717,8 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); if (obj->funcs && obj->funcs->mmap) { + vma->vm_ops = obj->funcs->vm_ops; + ret = obj->funcs->mmap(obj, vma); if (ret) return ret;