On Mon, Oct 22, 2018 at 04:15:48PM +0200, Noralf Trønnes wrote:
>
> Den 17.10.2018 17.46, skrev Daniel Vetter:
> > On Wed, Oct 17, 2018 at 03:04:53PM +0200, Noralf Trønnes wrote:
> > > This adds a library for shmem backed GEM objects.
> > >
> > > v5:
> > > - Drop drm_gem_shmem_prime_mmap() (Daniel Vetter)
> > > - drm_gem_shmem_mmap(): Subtract drm_vma_node_start() to get the real
> > > vma->vm_pgoff
> > > - drm_gem_shmem_fault(): Use vmf->pgoff now that vma->vm_pgoff is correct
> > >
> > > v4:
> > > - Drop cache modes (Thomas Hellstrom)
> > > - Add a GEM attached vtable
> > >
> > > v3:
> > > - Grammar (Sam Ravnborg)
> > > - s/drm_gem_shmem_put_pages_unlocked/drm_gem_shmem_put_pages_locked/
> > > (Sam Ravnborg)
> > > - Add debug output in error path (Sam Ravnborg)
> > >
> > > Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > > ---
> > > Documentation/gpu/drm-kms-helpers.rst | 12 +
> > > drivers/gpu/drm/Kconfig | 6 +
> > > drivers/gpu/drm/Makefile | 1 +
> > > drivers/gpu/drm/drm_gem_shmem_helper.c | 551 +++++++++++++++++++++++++++++++++
> > > include/drm/drm_gem_shmem_helper.h | 153 +++++++++
> > > 5 files changed, 723 insertions(+)
> > > create mode 100644 drivers/gpu/drm/drm_gem_shmem_helper.c
> > > create mode 100644 include/drm/drm_gem_shmem_helper.h
> > >
>
> <snip>
>
> > > +static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
> > > +{
> > > + struct vm_area_struct *vma = vmf->vma;
> > > + struct drm_gem_object *obj = vma->vm_private_data;
> > > + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> > > + loff_t num_pages = obj->size >> PAGE_SHIFT;
> > > + struct page *page;
> > > +
> > > + if (vmf->pgoff > num_pages || WARN_ON_ONCE(!shmem->pages))
> > > + return VM_FAULT_SIGBUS;
> > > +
> > > + page = shmem->pages[vmf->pgoff];
> > > +
> > > + return vmf_insert_page(vma, vmf->address, page);
> > > +}
> > > +
> > > +static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
> > > +{
> > > + struct drm_gem_object *obj = vma->vm_private_data;
> > > + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> > > +
> > > + drm_gem_shmem_put_pages(shmem);
> > Imbalance get/put_pages here, if someone forks (which is what calls
> > vm_ops->open on an existing vma).
> >
> > > + drm_gem_vm_close(vma);
> > > +}
> > > +
> > > +const struct vm_operations_struct drm_gem_shmem_vm_ops = {
> > > + .fault = drm_gem_shmem_fault,
> > > + .open = drm_gem_vm_open,
> > > + .close = drm_gem_shmem_vm_close,
> > > +};
> > > +EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
> > > +
> > > +/**
> > > + * drm_gem_shmem_mmap - Memory-map a shmem GEM object
> > > + * @filp: File object
> > > + * @vma: VMA for the area to be mapped
> > > + *
> > > + * This function implements an augmented version of the GEM DRM file mmap
> > > + * operation for shmem objects. Drivers which employ the shmem helpers should
> > > + * use this function as their &file_operations.mmap handler in the DRM device file's
> > > + * file_operations structure.
> > > + *
> > > + * Instead of directly referencing this function, drivers should use the
> > > + * DEFINE_DRM_GEM_SHMEM_FOPS() macro.
> > > + *
> > > + * Returns:
> > > + * 0 on success or a negative error code on failure.
> > > + */
> > Between cma helpers, gem shmem helpers and the drm_gem_mmap we now have
> > quite a bit a confusion of mmap functions. I think it'd be good to update
> > the kerneldoc for drm_gem_mmap() to point at these here (both the shmem
> > and cma version), so that people prefer to use these helpers here. Direct
> > call to drm_gem_mmap would only be needed if you want to use your own
> > vm_ops.
> >
> > > +int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma)
> > > +{
> > > + struct drm_gem_shmem_object *shmem;
> > > + int ret;
> > > +
> > > + ret = drm_gem_mmap(filp, vma);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + shmem = to_drm_gem_shmem_obj(vma->vm_private_data);
> > > +
> > > + ret = drm_gem_shmem_get_pages(shmem);
> > > + if (ret) {
> > > + drm_gem_vm_close(vma);
> > > + return ret;
> > > + }
> > > +
> > > + /* VM_PFNMAP was set by drm_gem_mmap() */
> > > + vma->vm_flags &= ~VM_PFNMAP;
> > > + vma->vm_flags |= VM_MIXEDMAP;
> > > + vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> > > +
> > > + fput(vma->vm_file);
> > > + vma->vm_file = get_file(shmem->base.filp);
> > > + /* Remove the fake offset */
> > > + vma->vm_pgoff -= drm_vma_node_start(&shmem->base.vma_node);
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(drm_gem_shmem_mmap);
> > Looking through your mmap code there's 2 bits:
> > - shmem isn't coherent,
>
> You saved me in the last minute with that comment Daniel.
>
> I just remembered that on the Raspberry Pi I need to swap the bytes (RGB565,
> SPI big endian) before transfer because its SPI can't do 16-bit transfers.
> So I removed the swapping and sent the shmem buffer straight through to SPI.
> This resulted in fbcon looking erratic and "distorted", so it didn't work
> with the generic fbdev emulation shadow buffer being copied by the cpu to
> the shmem buffer which is DMA'ed out over SPI. Even though the SPI core is
> using the streaming DMA API.
> (apparently I've worked so much on DRM helper refactoring that I've
> forgotten how the tinydrm drivers work...)
>
> So it looks like I can't use shmem for the current tinydrm drivers, which
> all use SPI. Unless there's a solution to my problem, I guess I'll I have
> to postpone this shmem work until I can use it in a driver that I'm planning
> to do further down the road. It will compress the framebuffer before
> transfer over USB so no DMA on the shmem buffer.
>
> Do you want me to apply the first 3 patches anyway, even though the shmem
> stuff has to wait?
Yeah I guess back to drawing board.
It looks like there's interests from others (Christian from AMD) on the
vtables stuff. So just pushing that, including a todo.rst entry to convert
drivers over, sounds like a good idea.
>
> > if you try to actually buffer share this with
> > prime I expect it'll fail badly. We'd need begin/end_cpu_access
> > callbacks in dma_buf_ops to make this work. So not entirely sure how
> > much value there is in implementing the prime mmap stuff (but doesn't
> > hurt to have it).
>
> The generic fbdev emulation uses PRIME mmap.
Oops. Works for CMA, doesn't work so well for shmem (assuming we have
non-coherent dma somewhere).
Maybe we can assume that all shmem using drivers will employ some kind of
manual upload (i.e. dirty), and then we could call the flush functions
from there?
> > - shmem already has an mmap implemtation, you can just use that. Needs a
> > similar mmap forwarding trick as you've done for prime mmap, but instead
> > you forward to obj->base.filp. That allows you to cut away all the
> > vm_ops and everything. I guess we could clean this up in a follow-up,
> > since it doesn't have an effect on the interfaces exposed to drivers. At
> > least not a big one, drm_gem_shmem_vm_ops would disappear.
>
> This approach using the shmem mmap seemed to work, not sure if the vm_flags
> needs to be touched though.
> (I'm putting it up here for the record, increases the chance of me finding
> it when I pick up the shmem helper further down the road)
vm_flags touching shouldn't be necessary anymore, you don't use
vm_insert_pfn. Which is what requires the VM_PFNMAP/MIXEDMAP stuff,
depending upon which vm_insert* function you're using. Afaiui at least,
not a core -mm expert at all.
But yes, this is what I had in mind.
-Daniel
>
> int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma)
> {
> struct drm_gem_object *obj;
> int ret;
>
> ret = drm_gem_mmap(filp, vma);
> if (ret)
> return ret;
>
> obj = vma->vm_private_data;
>
> /* Remove the fake offset */
> vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>
> /* VM_PFNMAP was set by drm_gem_mmap() */
> vma->vm_flags &= ~VM_PFNMAP;
> vma->vm_flags |= VM_MIXEDMAP;
> vma->vm_page_prot = pgprot_decrypted(vm_get_page_prot(vma->vm_flags));
>
> fput(vma->vm_file);
> vma->vm_file = get_file(obj->filp);
>
> drm_gem_object_put_unlocked(obj);
>
> return call_mmap(vma->vm_file, vma);
> }
>
>
> Noralf.
>
>
> > > +
> > > +/**
> > > + * drm_gem_shmem_print_info() - Print &drm_gem_shmem_object info for debugfs
> > > + * @p: DRM printer
> > > + * @indent: Tab indentation level
> > > + * @obj: GEM object
> > Would be good to reference the hook this is meant for:
> > &drm_gem_object_funcs.print_info, same for all the others. For those
> > drivers that want to pick&choose.
> >
> > Cheers, Daniel
> >
> > > + */
> > > +void drm_gem_shmem_print_info(struct drm_printer *p, unsigned int indent,
> > > + const struct drm_gem_object *obj)
> > > +{
> > > + const struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> > > +
> > > + drm_printf_indent(p, indent, "pages_use_count=%u\n", shmem->pages_use_count);
> > > + drm_printf_indent(p, indent, "vmap_use_count=%u\n", shmem->vmap_use_count);
> > > + drm_printf_indent(p, indent, "vaddr=%p\n", shmem->vaddr);
> > > +}
> > > +EXPORT_SYMBOL(drm_gem_shmem_print_info);
> > > +
> > > +/**
> > > + * drm_gem_shmem_get_sg_table - Provide a scatter/gather table of pinned
> > > + * pages for a shmem GEM object
> > > + * @obj: GEM object
> > > + *
> > > + * This function exports a scatter/gather table suitable for PRIME usage by
> > > + * calling the standard DMA mapping API.
> > > + *
> > > + * Returns:
> > > + * A pointer to the scatter/gather table of pinned pages or NULL on failure.
> > > + */
> > > +struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_object *obj)
> > > +{
> > > + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> > > +
> > > + return drm_prime_pages_to_sg(shmem->pages, obj->size >> PAGE_SHIFT);
> > > +}
> > > +EXPORT_SYMBOL_GPL(drm_gem_shmem_get_sg_table);
> > > +
> > > +/**
> > > + * drm_gem_shmem_prime_import_sg_table - Produce a shmem GEM object from
> > > + * another driver's scatter/gather table of pinned pages
> > > + * @dev: Device to import into
> > > + * @attach: DMA-BUF attachment
> > > + * @sgt: Scatter/gather table of pinned pages
> > > + *
> > > + * This function imports a scatter/gather table exported via DMA-BUF by
> > > + * another driver. Drivers that use the shmem helpers should set this as their
> > > + * &drm_driver.gem_prime_import_sg_table callback.
> > > + *
> > > + * Returns:
> > > + * A pointer to a newly created GEM object or an ERR_PTR-encoded negative
> > > + * error code on failure.
> > > + */
> > > +struct drm_gem_object *
> > > +drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
> > > + struct dma_buf_attachment *attach,
> > > + struct sg_table *sgt)
> > > +{
> > > + size_t size = PAGE_ALIGN(attach->dmabuf->size);
> > > + size_t npages = size >> PAGE_SHIFT;
> > > + struct drm_gem_shmem_object *shmem;
> > > + int ret;
> > > +
> > > + shmem = drm_gem_shmem_create(dev, size);
> > > + if (IS_ERR(shmem))
> > > + return ERR_CAST(shmem);
> > > +
> > > + shmem->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
> > > + if (!shmem->pages) {
> > > + ret = -ENOMEM;
> > > + goto err_free_gem;
> > > + }
> > > +
> > > + ret = drm_prime_sg_to_page_addr_arrays(sgt, shmem->pages, NULL, npages);
> > > + if (ret < 0)
> > > + goto err_free_array;
> > > +
> > > + shmem->sgt = sgt;
> > > + shmem->pages_use_count = 1; /* Permanently pinned from our point of view */
> > > +
> > > + DRM_DEBUG_PRIME("size = %zu\n", size);
> > > +
> > > + return &shmem->base;
> > > +
> > > +err_free_array:
> > > + kvfree(shmem->pages);
> > > +err_free_gem:
> > > + drm_gem_object_put_unlocked(&shmem->base);
> > > +
> > > + return ERR_PTR(ret);
> > > +}
> > > +EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
> > > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> > > new file mode 100644
> > > index 000000000000..26b05e06407d
> > > --- /dev/null
> > > +++ b/include/drm/drm_gem_shmem_helper.h
> > > @@ -0,0 +1,153 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +
> > > +#ifndef __DRM_GEM_SHMEM_HELPER_H__
> > > +#define __DRM_GEM_SHMEM_HELPER_H__
> > > +
> > > +#include <linux/fs.h>
> > > +#include <linux/mm.h>
> > > +#include <linux/mutex.h>
> > > +
> > > +#include <drm/drm_file.h>
> > > +#include <drm/drm_gem.h>
> > > +#include <drm/drm_ioctl.h>
> > > +#include <drm/drm_prime.h>
> > > +
> > > +struct dma_buf_attachment;
> > > +struct drm_mode_create_dumb;
> > > +struct drm_printer;
> > > +struct sg_table;
> > > +
> > > +/**
> > > + * struct drm_gem_shmem_object - GEM object backed by shmem
> > > + */
> > > +struct drm_gem_shmem_object {
> > > + /**
> > > + * @base: Base GEM object
> > > + */
> > > + struct drm_gem_object base;
> > > +
> > > + /**
> > > + * @pages_lock: Protects the page table and use count
> > > + */
> > > + struct mutex pages_lock;
> > > +
> > > + /**
> > > + * @pages: Page table
> > > + */
> > > + struct page **pages;
> > > +
> > > + /**
> > > + * @pages_use_count:
> > > + *
> > > + * Reference count on the pages table.
> > > + * The pages are put when the count reaches zero.
> > > + */
> > > + unsigned int pages_use_count;
> > > +
> > > + /**
> > > + * @pages_mark_dirty_on_put:
> > > + *
> > > + * Mark pages as dirty when they are put.
> > > + */
> > > + unsigned int pages_mark_dirty_on_put : 1;
> > > +
> > > + /**
> > > + * @pages_mark_accessed_on_put:
> > > + *
> > > + * Mark pages as accessed when they are put.
> > > + */
> > > + unsigned int pages_mark_accessed_on_put : 1;
> > > +
> > > + /**
> > > + * @sgt: Scatter/gather table for imported PRIME buffers
> > > + */
> > > + struct sg_table *sgt;
> > > +
> > > + /**
> > > + * @vmap_lock: Protects the vmap address and use count
> > > + */
> > > + struct mutex vmap_lock;
> > > +
> > > + /**
> > > + * @vaddr: Kernel virtual address of the backing memory
> > > + */
> > > + void *vaddr;
> > > +
> > > + /**
> > > + * @vmap_use_count:
> > > + *
> > > + * Reference count on the virtual address.
> > > + * The address are un-mapped when the count reaches zero.
> > > + */
> > > + unsigned int vmap_use_count;
> > > +};
> > > +
> > > +#define to_drm_gem_shmem_obj(obj) \
> > > + container_of(obj, struct drm_gem_shmem_object, base)
> > > +
> > > +/**
> > > + * DEFINE_DRM_GEM_SHMEM_FOPS() - Macro to generate file operations for shmem drivers
> > > + * @name: name for the generated structure
> > > + *
> > > + * This macro autogenerates a suitable &struct file_operations for shmem based
> > > + * drivers, which can be assigned to &drm_driver.fops. Note that this structure
> > > + * cannot be shared between drivers, because it contains a reference to the
> > > + * current module using THIS_MODULE.
> > > + *
> > > + * Note that the declaration is already marked as static - if you need a
> > > + * non-static version of this you're probably doing it wrong and will break the
> > > + * THIS_MODULE reference by accident.
> > > + */
> > > +#define DEFINE_DRM_GEM_SHMEM_FOPS(name) \
> > > + static const struct file_operations name = {\
> > > + .owner = THIS_MODULE,\
> > > + .open = drm_open,\
> > > + .release = drm_release,\
> > > + .unlocked_ioctl = drm_ioctl,\
> > > + .compat_ioctl = drm_compat_ioctl,\
> > > + .poll = drm_poll,\
> > > + .read = drm_read,\
> > > + .llseek = noop_llseek,\
> > > + .mmap = drm_gem_shmem_mmap, \
> > > + }
> > > +
> > > +struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t size);
> > > +void drm_gem_shmem_free_object(struct drm_gem_object *obj);
> > > +
> > > +int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem);
> > > +void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem);
> > > +int drm_gem_shmem_pin(struct drm_gem_object *obj);
> > > +void drm_gem_shmem_unpin(struct drm_gem_object *obj);
> > > +void *drm_gem_shmem_vmap(struct drm_gem_object *obj);
> > > +void drm_gem_shmem_vunmap(struct drm_gem_object *obj, void *vaddr);
> > > +
> > > +int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
> > > + struct drm_mode_create_dumb *args);
> > > +
> > > +int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma);
> > > +
> > > +extern const struct vm_operations_struct drm_gem_shmem_vm_ops;
> > > +
> > > +void drm_gem_shmem_print_info(struct drm_printer *p, unsigned int indent,
> > > + const struct drm_gem_object *obj);
> > > +
> > > +struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_object *obj);
> > > +struct drm_gem_object *
> > > +drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
> > > + struct dma_buf_attachment *attach,
> > > + struct sg_table *sgt);
> > > +
> > > +/**
> > > + * DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations
> > > + *
> > > + * This macro provides a shortcut for setting the shmem GEM operations in
> > > + * the &drm_driver structure.
> > > + */
> > > +#define DRM_GEM_SHMEM_DRIVER_OPS \
> > > + .prime_handle_to_fd = drm_gem_prime_handle_to_fd, \
> > > + .prime_fd_to_handle = drm_gem_prime_fd_to_handle, \
> > > + .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, \
> > > + .gem_prime_mmap = drm_gem_prime_mmap, \
> > > + .dumb_create = drm_gem_shmem_dumb_create
> > > +
> > > +#endif /* __DRM_GEM_SHMEM_HELPER_H__ */
> > > --
> > > 2.15.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
