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

[Noralf Trønnes]

Den 20.06.2018 15.52, skrev 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?
>

There's one more quirk that I forgot:
If fbdev can't release the client on .unregister due to open fd's, the
list entry should be removed but releasing resources is deferred to
the last fd being closed.

Noralf.

>
> 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
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>