[Intel-gfx] [PATCH v2 00/12] drm: Add generic fbdev emulation

[Noralf Trønnes]

Den 20.06.2018 11.34, skrev Daniel Vetter:
> On Mon, Jun 18, 2018 at 04:17:27PM +0200, Noralf Trønnes wrote:
>> This patchset adds generic fbdev emulation for drivers that supports GEM
>> based dumb buffers which support .gem_prime_vmap and gem_prime_mmap. An
>> API is begun to support in-kernel clients in general.
>>
>> Notable changes since version 1:
>>
>> - Rework client unregister code. I've used reference counting to manage
>>    the fact that both the client itself and the driver through
>>    drm_dev_unregister() can release the client. The client is now released
>>    using drm_client_put() instead of drm_client_free().
> I started reviewing the reworked client register/unregister stuff, and it
> looks good, except this kref stuff here for clients. I don't understand
> why you need this - as long as removal from dev->clientlist is properly
> protected by the mutex, I don't see how both the client and the device can
> release the client at the same time. Of course this means that both the
> device-trigger unregister and the client-triggered unregister must first
> grab the mutex, remove the client from the list, and only if that was done
> succesfully, clean up the client. If the client is already removed from
> the list (i.e. list_empty() is true) then you need to bail out to avoid
> double-freeing.

I just suck at this :/

Use case:
Unloading client module at the same time as the device is unplugged.

The client module calls drm_client_release():

void drm_client_release(struct drm_client_dev *client)
{
     struct drm_device *dev = client->dev;

     mutex_lock(&dev->clientlist_mutex);
     list_del(&client->list);
     drm_client_close(client);
     mutex_unlock(&dev->clientlist_mutex);
     drm_dev_put(dev);
}


drm_device_unregister() calls drm_client_dev_unregister():

void drm_client_dev_unregister(struct drm_device *dev)
{
     struct drm_client_dev *client;

     mutex_lock(&dev->clientlist_mutex);
     list_for_each_entry(client, &dev->clientlist, list) {
         if (client->funcs && client->funcs->unregister)
             client->funcs->unregister(client);
         else
             drm_client_release(client);
     }
     mutex_unlock(&dev->clientlist_mutex);
}


How do I do this without deadlocking and without operating on a
drm_client_dev structure that has been freed in the other codepath?


Noralf.

> I don't think there's a need to use a kref here. And kref has the tricky
> issue that you tempt everyone into constructing references loops between
> drm_device and drm_client (which require lots of jumping through hoops in
> your v1 to make sure you can break those reference loops).
>
>> - fbdev: Use a shadow buffer for framebuffers that have a dirty
>>    callback. This makes the fbdev client truly generic and useable for all
>>    drivers. There's a blitting penalty, but this is generic emulation after
>>    all. The reason for needing a shadow buffer is that deferred I/O only
>>    works with kmalloc/vmalloc buffers and not with shmem buffers
>>    (page->lru/mapping).
> Yeah I think that's the only feasible option. Everyone who cares more
> about fbdev performance can keep their driver-specific code. And for other
> drm_client users this shouldn't be a problem, since they know how to use
> dirty and flipping between multiple buffers to drive kms as it was
> designed. The issue really only exists for fbdev's assumption of a direct
> mmap of a dumb framebuffer, encoded into the uapi.
>
>> - Let tinydrm use the full fbdev client
> \o/
>
> Cheers, Daniel
>> Noralf.
>>
>> Changes since version 1:
>> - Make it possible to embed struct drm_client_dev and drop the private
>>    pointer
>> - Use kref reference counting to control client release since both the
>>    client and the driver can release.
>> - Add comment about using dma-buf as a possibility with some rework
>> - Move buffer NULL check to drm_client_framebuffer_delete()
>> - Move client name to struct drm_client_dev
>> - Move up drm_dev_get/put calls to make them more visible
>> - Move drm_client_dev.list definition to later patch that makes use of it
>>
>> - Embed drm_client at the beginning of drm_fb_helper to avoid a fragile
>>    transitional kfree hack in drm_client_release()
>> - Set owner in drm_fbdev_fb_ops
>> - Add kerneldoc to drm_fb_helper_generic_probe()
>>
>> - Remove unused functions
>> - Change name drm_client_funcs.lastclose -> .restore
>> - Change name drm_client_funcs.remove -> .unregister
>> - Rework unregister code
>>
>> - tinydrm: Use drm_fbdev_generic_setup() and remove
>>    drm_fb_cma_fbdev_init_with_funcs()
>>
>> David Herrmann (1):
>>    drm: provide management functions for drm_file
>>
>> Noralf Trønnes (11):
>>    drm/file: Don't set master on in-kernel clients
>>    drm: Make ioctls available for in-kernel clients
>>    drm: Begin an API for in-kernel clients
>>    drm/fb-helper: Add generic fbdev emulation .fb_probe function
>>    drm/pl111: Set .gem_prime_vmap and .gem_prime_mmap
>>    drm/cma-helper: Use the generic fbdev emulation
>>    drm/client: Add client callbacks
>>    drm/debugfs: Add internal client debugfs file
>>    drm/fb-helper: Finish the generic fbdev emulation
>>    drm/tinydrm: Use drm_fbdev_generic_setup()
>>    drm/cma-helper: Remove drm_fb_cma_fbdev_init_with_funcs()
>>
>>   Documentation/gpu/drm-client.rst            |  12 +
>>   Documentation/gpu/index.rst                 |   1 +
>>   drivers/gpu/drm/Makefile                    |   2 +-
>>   drivers/gpu/drm/drm_client.c                | 435 ++++++++++++++++++++++++++++
>>   drivers/gpu/drm/drm_crtc_internal.h         |  19 +-
>>   drivers/gpu/drm/drm_debugfs.c               |   7 +
>>   drivers/gpu/drm/drm_drv.c                   |   8 +
>>   drivers/gpu/drm/drm_dumb_buffers.c          |  33 ++-
>>   drivers/gpu/drm/drm_fb_cma_helper.c         | 380 +++---------------------
>>   drivers/gpu/drm/drm_fb_helper.c             | 330 ++++++++++++++++++++-
>>   drivers/gpu/drm/drm_file.c                  | 304 ++++++++++---------
>>   drivers/gpu/drm/drm_framebuffer.c           |  42 ++-
>>   drivers/gpu/drm/drm_internal.h              |   2 +
>>   drivers/gpu/drm/drm_ioctl.c                 |   4 +-
>>   drivers/gpu/drm/drm_probe_helper.c          |   3 +
>>   drivers/gpu/drm/pl111/pl111_drv.c           |   2 +
>>   drivers/gpu/drm/tinydrm/core/tinydrm-core.c |   3 +-
>>   drivers/gpu/drm/tinydrm/ili9225.c           |   1 -
>>   drivers/gpu/drm/tinydrm/mi0283qt.c          |   1 -
>>   drivers/gpu/drm/tinydrm/st7586.c            |   1 -
>>   drivers/gpu/drm/tinydrm/st7735r.c           |   1 -
>>   include/drm/drm_client.h                    | 156 ++++++++++
>>   include/drm/drm_device.h                    |  21 ++
>>   include/drm/drm_fb_cma_helper.h             |   6 -
>>   include/drm/drm_fb_helper.h                 |  38 +++
>>   25 files changed, 1298 insertions(+), 514 deletions(-)
>>   create mode 100644 Documentation/gpu/drm-client.rst
>>   create mode 100644 drivers/gpu/drm/drm_client.c
>>   create mode 100644 include/drm/drm_client.h
>>
>> -- 
>> 2.15.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx