[PATCH 1/6] drm/vram-helper: Managed vram helpers
Thomas Zimmermann
tzimmermann at suse.de
Thu Jul 16 11:00:12 UTC 2020
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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 516 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200716/69c1a9fa/attachment.sig>
More information about the dri-devel
mailing list