[Intel-gfx] [PATCH v2 00/12] drm: Add generic fbdev emulation
Noralf Trønnes
noralf at tronnes.org
Thu Jun 21 17:19:29 UTC 2018
Den 21.06.2018 09.14, skrev Daniel Vetter:
> On Wed, Jun 20, 2018 at 05:28:10PM +0200, Noralf Trønnes wrote:
>> 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.
> Do we really want to be able to unload client modules? Atm you can't
> unload the drm fbdev emulation either while a driver is still using it.
> Dropping that requirement would make things even simpler (you'd just need
> to add an owner field to drm_client and a try_module_get when registering
> it, bailing out if that fails).
>
> What's the use-case you have in mind that requires that you can unload a
> client driver module? Does that even work with the shuffling we've done on
> the register side of things?
When I first started on this client API, the client could unload itself
and I had a sysfs file that would remove clients for a particular
drm_device. This would mean that unloading a driver would require clients
to be removed first by writing to the sysfs file.
Then I started to look at the possibility that the driver could remove
clients automatically on driver unload. I have wrecked my brain trying to
make it race free, but it gets very complicated as you have shown in your
example. So I think we'll just avoid this complexity altogether.
So, now the question is, who gets to remove clients. The client or the
driver?
The common pattern is that clients can come and go on their own choosing.
It's what I would expect, that the client can be unloaded.
The reason I looked into auto unloading when the driver where going away,
was so developers shouldn't have to do the extra step to unload the driver.
Now I see a third case, and that's plug/unplug USB devices. If the driver
can't remove the client, we can end up with lots hanging drm_device and
drm_client_dev instances that have to be removed manually, which is
really not an option.
So I think we remove clients on driver/device removal. This means that to
unload a client, the driver(s) would have to be unloaded first.
I'll add an owner field to drm_client_funcs as you suggested and work out
the details.
Thanks for helping me get this as simple and straightforward as possible.
Noralf.
>>> The client module calls drm_client_release():
>>>
>>> void drm_client_release(struct drm_client_dev *client)
>>> {
>>> struct drm_device *dev = client->dev;
> Not sure (without reading your patches again) why we have drm_client_close
> here and ->unregister/drm_client_release below. But the trick is roughly
> to do
> client = NULL;
>
> mutex_lock();
> client = find_it();
> if (client)
> list_del();
> mute_unlock();
>
> if (!client)
> continue; /* or early return, whatever makes sense */
>
> drm_client_unregister(client);
>
> This way if you race there's:
> - only one thread will win, since the removal from the list is locked
> - no deadlocks, because the actual cleanup is done outside of the locks
>
> The problem is applying this trick to each situation, since you need to
> make sure that you get them all. Since you want to be able to unregister
> from 2 different lists, with each their own locks, you need to nest the
> above pattern in clever ways. In the client unregister function:
>
> mutex_lock(fbdev_clients_lock);
> client = list_first(fbdev_clients_list);
> if (client)
> list_del();
>
> mutex_lock(client->dev);
> if (!list_empty(client->dev_list))
> list_del();
> else
> /* someone else raced us, give up */
> client = NULL;
> mutex_unlock(client->dev);
> mutex_unlock(fbdev_clients_lock);
>
> if (!client)
> continue; /* or early return, whatever makes sense */
>
> drm_client_unregister(client);
>
> This way you know that as long as you hold the fbdevs_clients_lock client
> can't disappear, so you can look at client->dev (which also won't
> disappear, because the client can't disappear), which allows you to take
> the per-device client look to check whether you've race with removing.
>
> On the per-device client remove function we can't just do the same trick,
> because that would be a locking inversion. Instead we need careful
> ordering:
>
>
> mutex_lock(client->dev);
> if (!list_empty(client->dev_list))
> list_del();
> else
> /* someone else raced us, give up */
> client = NULL;
> mutex_unlock(client->dev);
>
> if (!client)
> continue; /* or early return, whatever makes sense */
>
> /* we've won the race and must do the cleanup, but first we need
> * to stop use-after-free */
>
> mutex_lock(fbdev_clients_lock);
> if (!list_empty(client->fbdev_list))
> list_del();
> else
> /* we raced and the other thread did the list removal
> * already, but will have backed off by now */
> mutex_unlock(fbdev_clients_lock);
>
> /* no one can get at the client structure anymore, it's safe to
> * clean it up */
>
> drm_client_unregister(client);
>
> Lots of complexity for a feature we didn't have yet and that I don't think
> we need really, but it is doable :-)
>
>>> 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.
> For fbdev I think kref'ing it makes sense. But probably better to do that
> in the structure that contains the drm_client, since I think this is very
> much an fbdev problem, not a general drm_client problem.
>
> Cheers, Daniel
>
>> 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
>>>
More information about the dri-devel
mailing list