Re: [PATCH v3 1/4] drm: add pgprot callback to drm_gem_object_funcs

From: Thomas Zimmermann
Date: Wed Dec 11 2019 - 08:21:21 EST


Hi

Am 11.12.19 um 13:38 schrieb Daniel Vetter:
> On Wed, Dec 11, 2019 at 01:19:53PM +0100, Gerd Hoffmann wrote:
>> The callback allows drivers and helpers to tweak pgprot for mappings.
>> This is especially helpful when using shmem helpers. It allows drivers
>> to switch mappings from writecombine (default) to something else (cached
>> for example) on a per-object base without having to supply their own
>> mmap() and vmap() functions.
>>
>> The patch also adds two implementations for the callback, for cached and
>> writecombine mappings, and the drm_gem_pgprot() function to update
>> pgprot for a given object, using the new &drm_gem_object_funcs.pgprot
>> callback if available.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@xxxxxxxxxx>
>> ---
>> include/drm/drm_gem.h | 15 +++++++++++++
>> drivers/gpu/drm/drm_gem.c | 46 ++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 60 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>> index 0b375069cd48..5beef7226e69 100644
>> --- a/include/drm/drm_gem.h
>> +++ b/include/drm/drm_gem.h
>> @@ -163,6 +163,17 @@ struct drm_gem_object_funcs {
>> */
>> int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);
>>
>> + /**
>> + * @pgprot:
>> + *
>> + * Tweak pgprot as needed, typically used to set cache bits.
>> + *
>> + * This callback is optional.
>> + *
>> + * If unset drm_gem_pgprot_wc() will be used.
>> + */
>> + pgprot_t (*pgprot)(struct drm_gem_object *obj, pgprot_t prot);
>
> I kinda prefer v1, mostly because this is a huge can of worms, and solving
> this properly is going to be real hard (and will necessarily involve
> dma-buf and dma-api and probably more). Charging ahead here just risks
> that we dig ourselves into a corner. You're v1 is maybe not the most
> clean, but just a few code bits here&there should be more flexible and
> easier to hack on and experiment around with.
> -Daniel

I agree; at least patch v1 is known to be a sound approach. The others
might fall on our feet at some point. Sorry, Gerd, if my proposals added
lots of work for you.

Best regards
Thomas

>
>> +
>> /**
>> * @vm_ops:
>> *
>> @@ -350,6 +361,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>> struct vm_area_struct *vma);
>> int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
>>
>> +pgprot_t drm_gem_pgprot_cached(struct drm_gem_object *obj, pgprot_t prot);
>> +pgprot_t drm_gem_pgprot_wc(struct drm_gem_object *obj, pgprot_t prot);
>> +pgprot_t drm_gem_pgprot(struct drm_gem_object *obj, pgprot_t prot);
>> +
>> /**
>> * drm_gem_object_get - acquire a GEM buffer object reference
>> * @obj: GEM buffer object
>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> index 56f42e0f2584..1c468fe8e342 100644
>> --- a/drivers/gpu/drm/drm_gem.c
>> +++ b/drivers/gpu/drm/drm_gem.c
>> @@ -1119,7 +1119,8 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>> return -EINVAL;
>>
>> vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
>> - vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>> + vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>> + vma->vm_page_prot = drm_gem_pgprot(obj, vma->vm_page_prot);
>> vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>> }
>>
>> @@ -1210,6 +1211,49 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>> }
>> EXPORT_SYMBOL(drm_gem_mmap);
>>
>> +/**
>> + * drm_gem_mmap - update pgprot for objects needing a cachable mapping.
>> + * @obj: the GEM object.
>> + * @prot: page attributes.
>> + *
>> + * This function can be used as &drm_gem_object_funcs.pgprot callback.
>> + */
>> +pgprot_t drm_gem_pgprot_cached(struct drm_gem_object *obj, pgprot_t prot)
>> +{
>> + return prot;
>> +}
>> +EXPORT_SYMBOL(drm_gem_pgprot_cached);
>> +
>> +/**
>> + * drm_gem_mmap - update pgprot for objects needing a wc mapping.
>> + * @obj: the GEM object.
>> + * @prot: page attributes.
>> + *
>> + * This function can be used as &drm_gem_object_funcs.pgprot callback.
>> + */
>> +pgprot_t drm_gem_pgprot_wc(struct drm_gem_object *obj, pgprot_t prot)
>> +{
>> + return pgprot_writecombine(prot);
>> +}
>> +EXPORT_SYMBOL(drm_gem_pgprot_wc);
>> +
>> +/**
>> + * drm_gem_mmap - update pgprot for a given gem object.
>> + * @obj: the GEM object.
>> + * @prot: page attributes.
>> + *
>> + * This function updates pgprot according to the needs of the given
>> + * object. If present &drm_gem_object_funcs.pgprot callback will be
>> + * used, otherwise drm_gem_pgprot_wc() is called.
>> + */
>> +pgprot_t drm_gem_pgprot(struct drm_gem_object *obj, pgprot_t prot)
>> +{
>> + if (obj->funcs->pgprot)
>> + return obj->funcs->pgprot(obj, prot);
>> + return drm_gem_pgprot_wc(obj, prot);
>> +}
>> +EXPORT_SYMBOL(drm_gem_pgprot);
>> +
>> void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
>> const struct drm_gem_object *obj)
>> {
>> --
>> 2.18.1
>>
>

--
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

Attachment: signature.asc
Description: OpenPGP digital signature