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

[Thomas Zimmermann]

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