| Message ID | 20191211121957.18637-2-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/virtio: fix mmap page attributes | expand |
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@redhat.com> > --- > 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 > + > /** > * @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 >
> > + /** > > + * @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. Hmm. Second vote for v1. Problem with v1 is that it covers mmap() only, not vmap() ... cheers, Gerd
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@redhat.com> >> --- >> 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 >> >
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); + /** * @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) {
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@redhat.com> --- include/drm/drm_gem.h | 15 +++++++++++++ drivers/gpu/drm/drm_gem.c | 46 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 60 insertions(+), 1 deletion(-)