| Message ID | 20200708074912.25422-2-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/ast: Managed MM | expand |
Hi Thomas. On Wed, Jul 08, 2020 at 09:49:07AM +0200, Thomas Zimmermann wrote: > Calling drmm_vram_helper_alloc_mm() sets up a managed instance of > VRAM MM. Releasing the DRM device also frees the memory manager. > > The patch also updates the DRM documentation for VRAM helpers. The > tutorial now only describes the new managed interface. The older > interfaces are deprecated and should not be used in new code. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> A couple of comments... > --- > drivers/gpu/drm/drm_gem_vram_helper.c | 68 ++++++++++++++++++++------- > include/drm/drm_gem_vram_helper.h | 4 ++ > 2 files changed, 55 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c > index ad096600d89f..af116999b193 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -10,6 +10,7 @@ > #include <drm/drm_gem_framebuffer_helper.h> > #include <drm/drm_gem_ttm_helper.h> > #include <drm/drm_gem_vram_helper.h> > +#include <drm/drm_managed.h> > #include <drm/drm_mode.h> > #include <drm/drm_plane.h> > #include <drm/drm_prime.h> > @@ -41,7 +42,7 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs; > * left in VRAM, inactive GEM objects can be moved to system memory. > * > * The easiest way to use the VRAM helper library is to call > - * drm_vram_helper_alloc_mm(). The function allocates and initializes an > + * drmm_vram_helper_alloc_mm(). The function allocates and initializes an > * instance of &struct drm_vram_mm in &struct drm_device.vram_mm . Use > * &DRM_GEM_VRAM_DRIVER to initialize &struct drm_driver and > * &DRM_VRAM_MM_FILE_OPERATIONS to initialize &struct file_operations; > @@ -69,7 +70,7 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs; > * // setup device, vram base and size > * // ... > * > - * ret = drm_vram_helper_alloc_mm(dev, vram_base, vram_size); > + * ret = drmm_vram_helper_alloc_mm(dev, vram_base, vram_size); > * if (ret) > * return ret; This example is how this should be done - but we are not there yet.. See below. > * return 0; > @@ -81,20 +82,12 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs; > * manages an area of video RAM with VRAM MM and provides GEM VRAM objects > * to userspace. > * > - * To clean up the VRAM memory management, call drm_vram_helper_release_mm() > - * in the driver's clean-up code. > + * You don't have to clean up the instance of VRAM MM. > + * drmm_vram_helper_alloc_mm() is a managed interface that installs a > + * clean-up handler to run during the DRM device's release. > * > - * .. code-block:: c > - * > - * void fini_drm_driver() > - * { > - * struct drm_device *dev = ...; > - * > - * drm_vram_helper_release_mm(dev); > - * } > - * > - * For drawing or scanout operations, buffer object have to be pinned in video > - * RAM. Call drm_gem_vram_pin() with &DRM_GEM_VRAM_PL_FLAG_VRAM or > + * For drawing or scanout operations, rsp. buffer objects have to be pinned > + * in video RAM. Call drm_gem_vram_pin() with &DRM_GEM_VRAM_PL_FLAG_VRAM or > * &DRM_GEM_VRAM_PL_FLAG_SYSTEM to pin a buffer object in video RAM or system > * memory. Call drm_gem_vram_unpin() to release the pinned object afterwards. > * > @@ -1177,12 +1170,16 @@ static void drm_vram_mm_cleanup(struct drm_vram_mm *vmm) > */ > > /** > - * drm_vram_helper_alloc_mm - Allocates a device's instance of \ > - &struct drm_vram_mm > + * drm_vram_helper_alloc_mm - Allocates a device's instance of > + * &struct drm_vram_mm > * @dev: the DRM device > * @vram_base: the base address of the video memory > * @vram_size: the size of the video memory in bytes > * > + * The driver is responsible to clean-up the VRAM manager during > + * device cleanup by calling drm_vram_helper_release_mm(). Use > + * drmm_vram_helper_alloc_mm() if possible. > + * > * Returns: > * The new instance of &struct drm_vram_mm on success, or > * an ERR_PTR()-encoded errno code otherwise. drm_vram_helper_alloc_mm is deprecated so just delete the kernel-doc. We do not want kernel-doc of deprecated functions. > @@ -1228,6 +1225,43 @@ void drm_vram_helper_release_mm(struct drm_device *dev) > } > EXPORT_SYMBOL(drm_vram_helper_release_mm); > > +static void drm_vram_mm_release(struct drm_device *dev, void *ptr) > +{ > + drm_vram_helper_release_mm(dev); > +} > + > +/** > + * drmm_vram_helper_alloc_mm - Allocates a device's managed instance of > + * &struct drm_vram_mm > + * @dev: the DRM device > + * @vram_base: the base address of the video memory > + * @vram_size: the size of the video memory in bytes > + * > + * The returned instance of &struct drm_vram_mm is auto-managed and > + * cleaned up as part of device cleanup. This should document that dev->vram_mm is assigned the value of the allocated drm_vram_mm is the allocation succeeds, otherwise set it to NULL. > + * > + * Returns: Some DRM doc uses "RETURNS" - I am not sure what it the most common way. > + * The new instance of &struct drm_vram_mm on success, or > + * an ERR_PTR()-encoded errno code otherwise. > + */ > +struct drm_vram_mm *drmm_vram_helper_alloc_mm(struct drm_device *dev, > + uint64_t vram_base, > + size_t vram_size) > +{ None of the users of drm_vram_helper_alloc_mm() uses the pointer, they all uses PTR_ERR(). So the right interface would be to return an int as already documented in the intro section. I had a patch to convert the function to return an int - but it is better to go direct to a managed interface. > + struct drm_vram_mm *vram_mm; > + int ret; > + > + vram_mm = drm_vram_helper_alloc_mm(dev, vram_base, vram_size); > + if (IS_ERR(vram_mm)) > + return vram_mm; > + ret = drmm_add_action_or_reset(dev, drm_vram_mm_release, NULL); > + if (ret) > + return ERR_PTR(ret); > + > + return vram_mm; > +} > +EXPORT_SYMBOL(drmm_vram_helper_alloc_mm); > + > /* > * Mode-config helpers > */ > diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h > index b63bcd1b996d..a456a272968b 100644 > --- a/include/drm/drm_gem_vram_helper.h > +++ b/include/drm/drm_gem_vram_helper.h > @@ -206,6 +206,10 @@ struct drm_vram_mm *drm_vram_helper_alloc_mm( > struct drm_device *dev, uint64_t vram_base, size_t vram_size); > void drm_vram_helper_release_mm(struct drm_device *dev); > > +struct drm_vram_mm *drmm_vram_helper_alloc_mm(struct drm_device *dev, > + uint64_t vram_base, > + size_t vram_size); > + > /* > * Mode-config helpers > */ > -- > 2.27.0
Hi Sam Am 08.07.20 um 19:19 schrieb Sam Ravnborg: > Hi Thomas. > > On Wed, Jul 08, 2020 at 09:49:07AM +0200, Thomas Zimmermann wrote: >> Calling drmm_vram_helper_alloc_mm() sets up a managed instance of >> VRAM MM. Releasing the DRM device also frees the memory manager. >> >> The patch also updates the DRM documentation for VRAM helpers. The >> tutorial now only describes the new managed interface. The older >> interfaces are deprecated and should not be used in new code. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > A couple of comments... These are all good points. I'll fix them in the next revision. Best regards Thomas > > > >> --- >> drivers/gpu/drm/drm_gem_vram_helper.c | 68 ++++++++++++++++++++------- >> include/drm/drm_gem_vram_helper.h | 4 ++ >> 2 files changed, 55 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c >> index ad096600d89f..af116999b193 100644 >> --- a/drivers/gpu/drm/drm_gem_vram_helper.c >> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c >> @@ -10,6 +10,7 @@ >> #include <drm/drm_gem_framebuffer_helper.h> >> #include <drm/drm_gem_ttm_helper.h> >> #include <drm/drm_gem_vram_helper.h> >> +#include <drm/drm_managed.h> >> #include <drm/drm_mode.h> >> #include <drm/drm_plane.h> >> #include <drm/drm_prime.h> >> @@ -41,7 +42,7 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs; >> * left in VRAM, inactive GEM objects can be moved to system memory. >> * >> * The easiest way to use the VRAM helper library is to call >> - * drm_vram_helper_alloc_mm(). The function allocates and initializes an >> + * drmm_vram_helper_alloc_mm(). The function allocates and initializes an >> * instance of &struct drm_vram_mm in &struct drm_device.vram_mm . Use >> * &DRM_GEM_VRAM_DRIVER to initialize &struct drm_driver and >> * &DRM_VRAM_MM_FILE_OPERATIONS to initialize &struct file_operations; >> @@ -69,7 +70,7 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs; >> * // setup device, vram base and size >> * // ... >> * >> - * ret = drm_vram_helper_alloc_mm(dev, vram_base, vram_size); >> + * ret = drmm_vram_helper_alloc_mm(dev, vram_base, vram_size); >> * if (ret) >> * return ret; > This example is how this should be done - but we are not there yet.. > See below. > >> * return 0; >> @@ -81,20 +82,12 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs; >> * manages an area of video RAM with VRAM MM and provides GEM VRAM objects >> * to userspace. >> * >> - * To clean up the VRAM memory management, call drm_vram_helper_release_mm() >> - * in the driver's clean-up code. >> + * You don't have to clean up the instance of VRAM MM. >> + * drmm_vram_helper_alloc_mm() is a managed interface that installs a >> + * clean-up handler to run during the DRM device's release. >> * >> - * .. code-block:: c >> - * >> - * void fini_drm_driver() >> - * { >> - * struct drm_device *dev = ...; >> - * >> - * drm_vram_helper_release_mm(dev); >> - * } >> - * >> - * For drawing or scanout operations, buffer object have to be pinned in video >> - * RAM. Call drm_gem_vram_pin() with &DRM_GEM_VRAM_PL_FLAG_VRAM or >> + * For drawing or scanout operations, rsp. buffer objects have to be pinned >> + * in video RAM. Call drm_gem_vram_pin() with &DRM_GEM_VRAM_PL_FLAG_VRAM or >> * &DRM_GEM_VRAM_PL_FLAG_SYSTEM to pin a buffer object in video RAM or system >> * memory. Call drm_gem_vram_unpin() to release the pinned object afterwards. >> * >> @@ -1177,12 +1170,16 @@ static void drm_vram_mm_cleanup(struct drm_vram_mm *vmm) >> */ >> >> /** >> - * drm_vram_helper_alloc_mm - Allocates a device's instance of \ >> - &struct drm_vram_mm >> + * drm_vram_helper_alloc_mm - Allocates a device's instance of >> + * &struct drm_vram_mm >> * @dev: the DRM device >> * @vram_base: the base address of the video memory >> * @vram_size: the size of the video memory in bytes >> * >> + * The driver is responsible to clean-up the VRAM manager during >> + * device cleanup by calling drm_vram_helper_release_mm(). Use >> + * drmm_vram_helper_alloc_mm() if possible. >> + * >> * Returns: >> * The new instance of &struct drm_vram_mm on success, or >> * an ERR_PTR()-encoded errno code otherwise. > drm_vram_helper_alloc_mm is deprecated so just delete the kernel-doc. > We do not want kernel-doc of deprecated functions. > > >> @@ -1228,6 +1225,43 @@ void drm_vram_helper_release_mm(struct drm_device *dev) >> } >> EXPORT_SYMBOL(drm_vram_helper_release_mm); >> >> +static void drm_vram_mm_release(struct drm_device *dev, void *ptr) >> +{ >> + drm_vram_helper_release_mm(dev); >> +} >> + >> +/** >> + * drmm_vram_helper_alloc_mm - Allocates a device's managed instance of >> + * &struct drm_vram_mm >> + * @dev: the DRM device >> + * @vram_base: the base address of the video memory >> + * @vram_size: the size of the video memory in bytes >> + * >> + * The returned instance of &struct drm_vram_mm is auto-managed and >> + * cleaned up as part of device cleanup. > This should document that dev->vram_mm is assigned the value of the > allocated drm_vram_mm is the allocation succeeds, otherwise set it to > NULL. > >> + * >> + * Returns: > Some DRM doc uses "RETURNS" - I am not sure what it the most common way. > >> + * The new instance of &struct drm_vram_mm on success, or >> + * an ERR_PTR()-encoded errno code otherwise. >> + */ >> +struct drm_vram_mm *drmm_vram_helper_alloc_mm(struct drm_device *dev, >> + uint64_t vram_base, >> + size_t vram_size) >> +{ > None of the users of drm_vram_helper_alloc_mm() uses the pointer, they > all uses PTR_ERR(). > So the right interface would be to return an int as already documented > in the intro section. > I had a patch to convert the function to return an int - but it is > better to go direct to a managed interface. > > >> + struct drm_vram_mm *vram_mm; >> + int ret; >> + >> + vram_mm = drm_vram_helper_alloc_mm(dev, vram_base, vram_size); >> + if (IS_ERR(vram_mm)) >> + return vram_mm; >> + ret = drmm_add_action_or_reset(dev, drm_vram_mm_release, NULL); >> + if (ret) >> + return ERR_PTR(ret); >> + >> + return vram_mm; >> +} >> +EXPORT_SYMBOL(drmm_vram_helper_alloc_mm); >> + >> /* >> * Mode-config helpers >> */ >> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h >> index b63bcd1b996d..a456a272968b 100644 >> --- a/include/drm/drm_gem_vram_helper.h >> +++ b/include/drm/drm_gem_vram_helper.h >> @@ -206,6 +206,10 @@ struct drm_vram_mm *drm_vram_helper_alloc_mm( >> struct drm_device *dev, uint64_t vram_base, size_t vram_size); >> void drm_vram_helper_release_mm(struct drm_device *dev); >> >> +struct drm_vram_mm *drmm_vram_helper_alloc_mm(struct drm_device *dev, >> + uint64_t vram_base, >> + size_t vram_size); >> + >> /* >> * Mode-config helpers >> */ >> -- >> 2.27.0 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index ad096600d89f..af116999b193 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -10,6 +10,7 @@ #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_ttm_helper.h> #include <drm/drm_gem_vram_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_mode.h> #include <drm/drm_plane.h> #include <drm/drm_prime.h> @@ -41,7 +42,7 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs; * left in VRAM, inactive GEM objects can be moved to system memory. * * The easiest way to use the VRAM helper library is to call - * drm_vram_helper_alloc_mm(). The function allocates and initializes an + * drmm_vram_helper_alloc_mm(). The function allocates and initializes an * instance of &struct drm_vram_mm in &struct drm_device.vram_mm . Use * &DRM_GEM_VRAM_DRIVER to initialize &struct drm_driver and * &DRM_VRAM_MM_FILE_OPERATIONS to initialize &struct file_operations; @@ -69,7 +70,7 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs; * // setup device, vram base and size * // ... * - * ret = drm_vram_helper_alloc_mm(dev, vram_base, vram_size); + * ret = drmm_vram_helper_alloc_mm(dev, vram_base, vram_size); * if (ret) * return ret; * return 0; @@ -81,20 +82,12 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs; * manages an area of video RAM with VRAM MM and provides GEM VRAM objects * to userspace. * - * To clean up the VRAM memory management, call drm_vram_helper_release_mm() - * in the driver's clean-up code. + * You don't have to clean up the instance of VRAM MM. + * drmm_vram_helper_alloc_mm() is a managed interface that installs a + * clean-up handler to run during the DRM device's release. * - * .. code-block:: c - * - * void fini_drm_driver() - * { - * struct drm_device *dev = ...; - * - * drm_vram_helper_release_mm(dev); - * } - * - * For drawing or scanout operations, buffer object have to be pinned in video - * RAM. Call drm_gem_vram_pin() with &DRM_GEM_VRAM_PL_FLAG_VRAM or + * For drawing or scanout operations, rsp. buffer objects have to be pinned + * in video RAM. Call drm_gem_vram_pin() with &DRM_GEM_VRAM_PL_FLAG_VRAM or * &DRM_GEM_VRAM_PL_FLAG_SYSTEM to pin a buffer object in video RAM or system * memory. Call drm_gem_vram_unpin() to release the pinned object afterwards. * @@ -1177,12 +1170,16 @@ static void drm_vram_mm_cleanup(struct drm_vram_mm *vmm) */ /** - * drm_vram_helper_alloc_mm - Allocates a device's instance of \ - &struct drm_vram_mm + * drm_vram_helper_alloc_mm - Allocates a device's instance of + * &struct drm_vram_mm * @dev: the DRM device * @vram_base: the base address of the video memory * @vram_size: the size of the video memory in bytes * + * The driver is responsible to clean-up the VRAM manager during + * device cleanup by calling drm_vram_helper_release_mm(). Use + * drmm_vram_helper_alloc_mm() if possible. + * * Returns: * The new instance of &struct drm_vram_mm on success, or * an ERR_PTR()-encoded errno code otherwise. @@ -1228,6 +1225,43 @@ void drm_vram_helper_release_mm(struct drm_device *dev) } EXPORT_SYMBOL(drm_vram_helper_release_mm); +static void drm_vram_mm_release(struct drm_device *dev, void *ptr) +{ + drm_vram_helper_release_mm(dev); +} + +/** + * drmm_vram_helper_alloc_mm - Allocates a device's managed instance of + * &struct drm_vram_mm + * @dev: the DRM device + * @vram_base: the base address of the video memory + * @vram_size: the size of the video memory in bytes + * + * The returned instance of &struct drm_vram_mm is auto-managed and + * cleaned up as part of device cleanup. + * + * Returns: + * The new instance of &struct drm_vram_mm on success, or + * an ERR_PTR()-encoded errno code otherwise. + */ +struct drm_vram_mm *drmm_vram_helper_alloc_mm(struct drm_device *dev, + uint64_t vram_base, + size_t vram_size) +{ + struct drm_vram_mm *vram_mm; + int ret; + + vram_mm = drm_vram_helper_alloc_mm(dev, vram_base, vram_size); + if (IS_ERR(vram_mm)) + return vram_mm; + ret = drmm_add_action_or_reset(dev, drm_vram_mm_release, NULL); + if (ret) + return ERR_PTR(ret); + + return vram_mm; +} +EXPORT_SYMBOL(drmm_vram_helper_alloc_mm); + /* * Mode-config helpers */ diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h index b63bcd1b996d..a456a272968b 100644 --- a/include/drm/drm_gem_vram_helper.h +++ b/include/drm/drm_gem_vram_helper.h @@ -206,6 +206,10 @@ struct drm_vram_mm *drm_vram_helper_alloc_mm( struct drm_device *dev, uint64_t vram_base, size_t vram_size); void drm_vram_helper_release_mm(struct drm_device *dev); +struct drm_vram_mm *drmm_vram_helper_alloc_mm(struct drm_device *dev, + uint64_t vram_base, + size_t vram_size); + /* * Mode-config helpers */
Calling drmm_vram_helper_alloc_mm() sets up a managed instance of VRAM MM. Releasing the DRM device also frees the memory manager. The patch also updates the DRM documentation for VRAM helpers. The tutorial now only describes the new managed interface. The older interfaces are deprecated and should not be used in new code. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/drm_gem_vram_helper.c | 68 ++++++++++++++++++++------- include/drm/drm_gem_vram_helper.h | 4 ++ 2 files changed, 55 insertions(+), 17 deletions(-)