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 > > 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 >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -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 > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer