[PATCH v3 1/3] drm: add SimpleDRM driver
Noralf Trønnes
noralf at tronnes.org
Tue Aug 16 19:38:13 UTC 2016
Den 16.08.2016 17:25, skrev Daniel Vetter:
> On Tue, Aug 16, 2016 at 02:58:38PM +0200, Noralf Trønnes wrote:
>> Den 15.08.2016 08:59, skrev Daniel Vetter:
>>> On Sun, Aug 14, 2016 at 06:52:04PM +0200, Noralf Trønnes wrote:
>>>> The SimpleDRM driver binds to simple-framebuffer devices and provides a
>>>> DRM/KMS API. It provides only a single CRTC+encoder+connector combination
>>>> plus one initial mode.
>>>>
>>>> Userspace can create dumb-buffers which can be blit into the real
>>>> framebuffer similar to UDL. No access to the real framebuffer is allowed
>>>> (compared to earlier version of this driver) to avoid security issues.
>>>> Furthermore, this way we can support arbitrary modes as long as we have a
>>>> conversion-helper.
>>>>
>>>> The driver was originally written by David Herrmann in 2014.
>>>> My main contribution is to make use of drm_simple_kms_helper and
>>>> rework the probe path to avoid use of the deprecated drm_platform_init()
>>>> and drm_driver.{load,unload}().
>>>> Additions have also been made for later changes to the Device Tree binding
>>>> document, like support for clocks, regulators and having the node under
>>>> /chosen. This code was lifted verbatim from simplefb.c.
>>>>
>>>> Cc: dh.herrmann at gmail.com
>>>> Cc: libv at skynet.be
>>>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>> <snip>
>>
>>>> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_gem.c b/drivers/gpu/drm/simpledrm/simpledrm_gem.c
>>>> new file mode 100644
>>>> index 0000000..81bd7c5
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/simpledrm/simpledrm_gem.c
>> <snip>
>>
>>>> +struct sdrm_gem_object *sdrm_gem_alloc_object(struct drm_device *ddev,
>>>> + size_t size)
>>>> +{
>>>> + struct sdrm_gem_object *obj;
>>>> +
>>>> + WARN_ON(!size || (size & ~PAGE_MASK) != 0);
>>>> +
>>>> + obj = kzalloc(sizeof(*obj), GFP_KERNEL);
>>>> + if (!obj)
>>>> + return NULL;
>>>> +
>>>> + drm_gem_private_object_init(ddev, &obj->base, size);
>>>> + return obj;
>>>> +}
>>>> +
>>>> +void sdrm_gem_free_object(struct drm_gem_object *gobj)
>>>> +{
>>>> + struct sdrm_gem_object *obj = to_sdrm_bo(gobj);
>>>> + struct drm_device *ddev = gobj->dev;
>>>> +
>>>> + if (obj->pages) {
>>>> + /* kill all user-space mappings */
>>>> + drm_vma_node_unmap(&gobj->vma_node,
>>>> + ddev->anon_inode->i_mapping);
>>>> + sdrm_gem_put_pages(obj);
>>>> + }
>>>> +
>>>> + if (gobj->import_attach)
>>>> + drm_prime_gem_destroy(gobj, obj->sg);
>>>> +
>>>> + drm_gem_free_mmap_offset(gobj);
>>>> + drm_gem_object_release(gobj);
>>>> + kfree(obj);
>>>> +}
>>>> +
>>>> +int sdrm_dumb_create(struct drm_file *dfile, struct drm_device *ddev,
>>>> + struct drm_mode_create_dumb *args)
>>>> +{
>>>> + struct sdrm_gem_object *obj;
>>>> + int r;
>>>> +
>>>> + if (args->flags)
>>>> + return -EINVAL;
>>>> +
>>>> + /* overflow checks are done by DRM core */
>>>> + args->pitch = (args->bpp + 7) / 8 * args->width;
>>>> + args->size = PAGE_ALIGN(args->pitch * args->height);
>>>> +
>>>> + obj = sdrm_gem_alloc_object(ddev, args->size);
>>>> + if (!obj)
>>>> + return -ENOMEM;
>>>> +
>>>> + r = drm_gem_handle_create(dfile, &obj->base, &args->handle);
>>>> + if (r) {
>>>> + drm_gem_object_unreference_unlocked(&obj->base);
>>>> + return r;
>>>> + }
>>>> +
>>>> + /* handle owns a reference */
>>>> + drm_gem_object_unreference_unlocked(&obj->base);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +int sdrm_dumb_destroy(struct drm_file *dfile, struct drm_device *ddev,
>>>> + uint32_t handle)
>>>> +{
>>>> + return drm_gem_handle_delete(dfile, handle);
>>>> +}
>>> I wonder whether some dumb+gem helpers wouldn't be useful to roll out. At
>>> least drm_dumb_gem_destroy.c would be pretty simple.
>> There's already a drm_gem_dumb_destroy() in drm_gem.c
>>
>> The drm_driver->gem_create_object callback makes it possible to do a
>> generic dumb create:
>>
>> int drm_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
>> struct drm_mode_create_dumb *args)
>> {
>> struct drm_gem_object *obj;
>> int ret;
>>
>> if (!dev->driver->gem_create_object)
>> return -EINVAL;
>>
>> args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
>> args->size = args->pitch * args->height;
>>
>> obj = dev->driver->gem_create_object(dev, args->size);
>> if (!obj)
>> return -ENOMEM;
>>
>> ret = drm_gem_handle_create(file, obj, &args->handle);
>> drm_gem_object_unreference_unlocked(obj);
>>
>> return ret;
>> }
>> EXPORT_SYMBOL(drm_gem_dumb_create);
>>
>> struct drm_gem_object *sdrm_gem_create_object(struct drm_device *ddev,
>> size_t size)
>> {
>> struct sdrm_gem_object *sobj;
>>
>> sobj = sdrm_gem_alloc_object(ddev, size);
>> if (!sobj)
>> return ERR_PTR(-ENOMEM);
>>
>> return &sobj->base;
>> }
>>
>>>> +
>>>> +int sdrm_dumb_map_offset(struct drm_file *dfile, struct drm_device *ddev,
>>>> + uint32_t handle, uint64_t *offset)
>>>> +{
>>>> + struct drm_gem_object *gobj;
>>>> + int r;
>>>> +
>>>> + mutex_lock(&ddev->struct_mutex);
>>> There's still some struct mutex here.
>>>
>>>> +
>>>> + gobj = drm_gem_object_lookup(dfile, handle);
>>>> + if (!gobj) {
>>>> + r = -ENOENT;
>>>> + goto out_unlock;
>>>> + }
>>>> +
>>>> + r = drm_gem_create_mmap_offset(gobj);
>>>> + if (r)
>>>> + goto out_unref;
>>>> +
>>>> + *offset = drm_vma_node_offset_addr(&gobj->vma_node);
>>>> +
>>>> +out_unref:
>>>> + drm_gem_object_unreference(gobj);
>>>> +out_unlock:
>>>> + mutex_unlock(&ddev->struct_mutex);
>>>> + return r;
>>>> +}
>>>> +
>> Maybe this addition to drm_gem.c as well:
>>
>> int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
>> uint32_t handle, uint64_t *offset)
>> {
>> struct drm_gem_object *obj;
>> int ret;
>>
>> obj = drm_gem_object_lookup(file, handle);
>> if (!obj)
>> return -ENOENT;
>>
>> ret = drm_gem_create_mmap_offset(obj);
>> if (ret)
>> goto out_unref;
>>
>> *offset = drm_vma_node_offset_addr(&obj->vma_node);
>>
>> out_unref:
>> drm_gem_object_unreference_unlocked(obj);
>>
>> return ret;
>> }
> Yeah, sounds good for adding the above two. Feel free to roll them out to
> drivers (or not).
>
>>>> +int sdrm_drm_mmap(struct file *filp, struct vm_area_struct *vma)
>>>> +{
>>>> + struct drm_file *priv = filp->private_data;
>>>> + struct drm_device *dev = priv->minor->dev;
>>>> + struct drm_vma_offset_node *node;
>>>> + struct drm_gem_object *gobj;
>>>> + struct sdrm_gem_object *obj;
>>>> + size_t size, i, num;
>>>> + int r;
>>>> +
>>>> + if (drm_device_is_unplugged(dev))
>>>> + return -ENODEV;
>>>> +
>>>> + drm_vma_offset_lock_lookup(dev->vma_offset_manager);
>>>> + node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
>>>> + vma->vm_pgoff,
>>>> + vma_pages(vma));
>>>> + drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
>>>> +
>>>> + if (!drm_vma_node_is_allowed(node, filp))
>>>> + return -EACCES;
>>>> +
>>>> + gobj = container_of(node, struct drm_gem_object, vma_node);
>>>> + obj = to_sdrm_bo(gobj);
>>>> + size = drm_vma_node_size(node) << PAGE_SHIFT;
>>>> + if (size < vma->vm_end - vma->vm_start)
>>>> + return r;
>>>> +
>>>> + r = sdrm_gem_get_pages(obj);
>>>> + if (r < 0)
>>>> + return r;
>>>> +
>>>> + /* prevent dmabuf-imported mmap to user-space */
>>>> + if (!obj->pages)
>>>> + return -EACCES;
>>>> +
>>>> + vma->vm_flags |= VM_DONTEXPAND;
>>>> + vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>>>> +
>>>> + num = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
>>>> + for (i = 0; i < num; ++i) {
>>>> + r = vm_insert_page(vma, vma->vm_start + i * PAGE_SIZE,
>>>> + obj->pages[i]);
>>>> + if (r < 0) {
>>>> + if (i > 0)
>>>> + zap_vma_ptes(vma, vma->vm_start, i * PAGE_SIZE);
>>>> + return r;
>>>> + }
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>> Why can't we just redirect to the underlying shmem mmap here (after
>>> argument checking)?
>> I don't know what that means, but looking at vc4 it seems I can use
>> drm_gem_mmap() to remove some boilerplate.
>>
>> int sdrm_drm_mmap(struct file *filp, struct vm_area_struct *vma)
>> {
>> unsigned long vm_flags = vma->vm_flags;
>> struct sdrm_gem_object *sobj;
>> struct drm_gem_object *obj;
>> size_t i, num;
>> int r;
>>
>> r = drm_gem_mmap(filp, vma);
>> if (r)
>> return r;
>>
>> obj = vma->vm_private_data;
>>
>> sobj = to_sdrm_bo(obj);
>>
>> r = sdrm_gem_get_pages(obj);
>> if (r < 0)
>> return r;
>>
>> /* prevent dmabuf-imported mmap to user-space */
>> if (!obj->pages)
>> return -EACCES;
>>
>> /* drm_gem_mmap() sets flags that we don't want */
>> vma->vm_flags = vm_flags | VM_DONTEXPAND;
>> vma->vm_page_prot =
>> pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>>
>> num = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
>> for (i = 0; i < num; ++i) {
>> r = vm_insert_page(vma, vma->vm_start + i * PAGE_SIZE,
>> obj->pages[i]);
>> if (r < 0) {
>> if (i > 0)
>> zap_vma_ptes(vma, vma->vm_start, i * PAGE_SIZE);
>> return r;
>> }
>> }
>>
>> return 0;
>> }
>>
>> drm_gem_mmap() requires drm_driver->gem_vm_ops to be set:
>>
>> const struct vm_operations_struct sdrm_gem_vm_ops = {
>> .open = drm_gem_vm_open,
>> .close = drm_gem_vm_close,
>> };
>>
>> And browsing the code a bit more shows that tegra, udl, etnaviv and vgem
>> does the vm_insert_page() thing in the vm_operations_struct->fault()
>> handler.
>>
>> So maybe this makes sense for simpledrm as well:
>>
>> static int sdrm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>> {
>> struct drm_gem_object *obj = vma->vm_private_data;
>> struct sdrm_gem_object *sobj = to_sdrm_bo(obj);
>> loff_t num_pages;
>> pgoff_t offset;
>> int r;
>>
>> if (!sobj->pages)
>> return VM_FAULT_SIGBUS;
>>
>> offset = ((unsigned long)vmf->virtual_address - vma->vm_start) >>
>> PAGE_SHIFT;
>> num_pages = DIV_ROUND_UP(obj->base.size, PAGE_SIZE);
>> if (page_offset > num_pages)
>> return VM_FAULT_SIGBUS;
>>
>> r = vm_insert_page(vma, (unsigned long)vmf->virtual_address,
>> sobj->pages[offset]);
>> switch (r) {
>> case -EAGAIN:
>> case 0:
>> case -ERESTARTSYS:
>> case -EINTR:
>> case -EBUSY:
>> return VM_FAULT_NOPAGE;
>>
>> case -ENOMEM:
>> return VM_FAULT_OOM;
>> }
>>
>> return VM_FAULT_SIGBUS;
>> }
> That's still a lot for what amounts to reimplementing mmap on shmem, but
> badly. What I mean with redirecting is pointing the entire ->mmap
> operation to the mmap implementation for the underlying mmap. Roughly:
>
> /* normal gem mmap checks first */
>
> /* redirect to shmem mmap */
> vma->vm_file = obj->filp;
> vma->vm_pgoff = 0;
>
> return obj->filp->f_op->mmap(obj->filp, vma);
>
> Much less code ;-)
obj->filp is NULL in my case.
And looking at the docs, that's expected since I have driver specific
backing?
/**
* @filp:
*
* SHMEM file node used as backing storage for swappable buffer objects.
* GEM also supports driver private objects with driver-specific backing
* storage (contiguous CMA memory, special reserved blocks). In this
* case @filp is NULL.
*/
Noralf.
More information about the dri-devel
mailing list