On Tue, Jun 26, 2018 at 3:41 PM, Noralf Trønnes <noralf@xxxxxxxxxxx> wrote:
>
> 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?
I think because of the unregister callback it makes little sense to
have clients without a function table. But you can also make the
try_module_get conditional on drm_client->funcs, if that makes more
sense to you. No opinion on my side really.
-Daniel
>
> 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@xxxxxxxxxxxxxxxxxxxxx
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
