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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux