Hi
Am 24.07.19 um 14:00 schrieb Daniel Vetter:
> On Wed, Jul 24, 2019 at 1:30 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>>
>> The pattern of temporarily pinning and kmap-ing the BO's memory is
>> common enough to justify helper functions that do and undo these
>> operations.
>>
>> The implementation of vmap and vunmap for GEM VRAM helpers is
>> already in PRIME helpers. The patch moves the operations to separate
>> functions and exports them for general use.
>>
>> The patch also adds a note about possible kmap counting. So far this
>> isn't required by drivers, but more complex use cases might make it
>> necessary.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
>> ---
>> drivers/gpu/drm/drm_gem_vram_helper.c | 55 ++++++++++++++++++++++-----
>> include/drm/drm_gem_vram_helper.h | 12 ++++++
>> 2 files changed, 57 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
>> index e0fbfb6570cf..54758e4debca 100644
>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>> @@ -340,6 +340,48 @@ void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo)
>> }
>> EXPORT_SYMBOL(drm_gem_vram_kunmap);
>>
>> +/**
>> + * drm_gem_vram_vmap() - Pins and maps a GEM VRAM object into kernel address
>> + space
>> + * @gem: The GEM VRAM object to map
>
> variable names need to match, kernel-doc should be complaining here.
>
>> + *
>> + * The vmap function pins a GEM VRAM object to it's current location, either
>> + * system or video memory, and maps it's buffer into kernel address space. As
>> + * pinned object cannot be reloacted, you should not permanently pin objects.
>
> Imo also good to point at the corresponding functions, i.e. reference
> unmap here and in the unmap function reference the map function. And
> please make sure the links work in the generated doc output.
>
>> + *
>> + * Returns:
>> + * The buffer's virtual address on success, or
>> + * an ERR_PTR()-encoded error code otherwise.
>> + */
>> +void *drm_gem_vram_vmap(struct drm_gem_vram_object *gbo)
>> +{
>> + int ret;
>> + void *base;
>> +
>> + ret = drm_gem_vram_pin(gbo, 0);
>> + if (ret)
>> + return ERR_PTR(ret);
>> + base = drm_gem_vram_kmap(gbo, true, NULL);
>> + if (IS_ERR(base)) {
>> + drm_gem_vram_unpin(gbo);
>> + return base;
>> + }
>> + return base;
>> +}
>> +EXPORT_SYMBOL(drm_gem_vram_vmap);
>> +
>> +/**
>> + * drm_gem_vram_vunmap() - Unmaps and unpins a GEM VRAM object
>> + * @gem: The GEM VRAM object to unmap
>> + * @vaddr: The mapping's base address
>> + */
>> +void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo, void *vaddr)
>> +{
>> + drm_gem_vram_kunmap(gbo);
>> + drm_gem_vram_unpin(gbo);
>> +}
>> +EXPORT_SYMBOL(drm_gem_vram_vunmap);
>> +
>> /**
>> * drm_gem_vram_fill_create_dumb() - \
>> Helper for implementing &struct drm_driver.dumb_create
>> @@ -595,17 +637,11 @@ static void drm_gem_vram_object_unpin(struct drm_gem_object *gem)
>> static void *drm_gem_vram_object_vmap(struct drm_gem_object *gem)
>> {
>> struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
>> - int ret;
>> void *base;
>>
>> - ret = drm_gem_vram_pin(gbo, 0);
>> - if (ret)
>> + base = drm_gem_vram_vmap(gbo);
>> + if (IS_ERR(base))
>> return NULL;
>> - base = drm_gem_vram_kmap(gbo, true, NULL);
>> - if (IS_ERR(base)) {
>> - drm_gem_vram_unpin(gbo);
>> - return NULL;
>> - }
>> return base;
>> }
>>
>> @@ -620,8 +656,7 @@ static void drm_gem_vram_object_vunmap(struct drm_gem_object *gem,
>> {
>> struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
>>
>> - drm_gem_vram_kunmap(gbo);
>> - drm_gem_vram_unpin(gbo);
>> + drm_gem_vram_vunmap(gbo, vaddr);
>> }
>>
>> /*
>> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
>> index b41d932eb53a..5192c169cec2 100644
>> --- a/include/drm/drm_gem_vram_helper.h
>> +++ b/include/drm/drm_gem_vram_helper.h
>> @@ -44,6 +44,16 @@ struct drm_gem_vram_object {
>> struct ttm_placement placement;
>> struct ttm_place placements[2];
>>
>> + /* TODO: Maybe implement a map counter.
>
> Kerneldoc is missing here. You can use the inline style so that the
> todo comment here is still included.
> -Daniel
Actually, this comment wasn't intended to be in the kernel docs. I guess
I integrate it into the structure's overall documentation then.
Best regards
Thomas
>> + *
>> + * So far, drivers based on VRAM helpers don't have overlapping
>> + * mapping operations. A driver temporarily maps an object and
>> + * unmaps it ASAP. This works well for fbdev emulation or cursors.
>> + *
>> + * If we ever have a driver with buffer objects that are mapped
>> + * by multiple code fragments concurrently, we may need a map
>> + * counter to get the mapping right.
>> + */
>> int pin_count;
>> };
>>
>> @@ -84,6 +94,8 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo);
>> void *drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
>> bool *is_iomem);
>> void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo);
>> +void *drm_gem_vram_vmap(struct drm_gem_vram_object *gbo);
>> +void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo, void *vaddr);
>>
>> int drm_gem_vram_fill_create_dumb(struct drm_file *file,
>> struct drm_device *dev,
>> --
>> 2.22.0
>>
>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
