[Intel-gfx] [PATCH v2 00/12] drm: Add generic fbdev emulation
Noralf Trønnes
noralf at tronnes.org
Tue Jun 26 13:41:08 UTC 2018
Den 21.06.2018 19.19, skrev Noralf Trønnes:
>
> 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.
Currently drm_client_dev->funcs is optional, but should it be mandatory
now that drm_client_funcs gets an owner field?
Noralf.
>
> 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
>>>>
>
> _______________________________________________
> 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