Re: [PATCH v4 5/9] drm/client: Add client callbacks

On Tue, Jul 03, 2018 at 03:07:50PM +0200, Noralf Trønnes wrote:

Den 03.07.2018 09.46, skrev Daniel Vetter:

...

On Mon, Jul 02, 2018 at 03:54:29PM +0200, Noralf Trønnes wrote:

...

Add client callbacks and hook them up. Add a list of clients per drm_device.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

btw for reviewing it'd be simpler if you merge the 2 patches that add the client library, avoids me having to jump back&forth ..

Bunch of comments below still. -Daniel

...

---

drivers/gpu/drm/drm_client.c | 92 ++++++++++++++++++++++++++++++++++++- drivers/gpu/drm/drm_drv.c | 7 +++ drivers/gpu/drm/drm_fb_cma_helper.c | 2 +- drivers/gpu/drm/drm_fb_helper.c | 11 ++++- drivers/gpu/drm/drm_file.c | 3 ++ drivers/gpu/drm/drm_probe_helper.c | 3 ++ include/drm/drm_client.h | 75 +++++++++++++++++++++++++++++- include/drm/drm_device.h | 14 ++++++ 8 files changed, 201 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c index 9c9b8ac7aea3..f05ee98bd10c 100644 --- a/drivers/gpu/drm/drm_client.c +++ b/drivers/gpu/drm/drm_client.c @@ -4,6 +4,7 @@ */ #include <linux/list.h> +#include <linux/module.h> #include <linux/mutex.h> #include <linux/seq_file.h> #include <linux/slab.h> @@ -66,6 +67,7 @@ EXPORT_SYMBOL(drm_client_close);

• @dev: DRM device
• @client: DRM client
• @name: Client name

• • @funcs: DRM client functions (optional)
  • Use drm_client_put() to free the client.

@@ -73,24 +75,47 @@ EXPORT_SYMBOL(drm_client_close);

• Zero on success or negative error code on failure.

*/ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,

•    const char *name)
  

•    const char *name, const struct drm_client_funcs *funcs)
  

  {
• bool registered; int ret; if (!drm_core_check_feature(dev, DRIVER_MODESET) || !dev->driver->dumb_create || !dev->driver->gem_prime_vmap) return -ENOTSUPP;
• if (funcs && !try_module_get(funcs->owner))

• return -ENODEV;
  

• client->dev = dev; client->name = name;
• client->funcs = funcs; ret = drm_client_open(client); if (ret)

• return ret;
  

• goto err_put_module;
  

• mutex_lock(&dev->clientlist_mutex);
• registered = dev->registered;
• if (registered)

• list_add(&client->list, &dev->clientlist);
  

• mutex_unlock(&dev->clientlist_mutex);
• if (!registered) {

• ret = -ENODEV;
  

• goto err_close;
  

• } drm_dev_get(dev);

This only works if the caller holds a reference for us on dev already, or has some other guarantee that it won't disappear. Would be good to mention this in the kerneldoc.

...

return 0;

+err_close:

• drm_client_close(client);

+err_put_module:

• if (funcs)

• module_put(funcs->owner);
  

• return ret; } EXPORT_SYMBOL(drm_client_new);

@@ -116,9 +141,72 @@ void drm_client_release(struct drm_client_dev *client) drm_client_close(client); drm_dev_put(dev);

• if (client->funcs)

• module_put(client->funcs->owner);
  

  } EXPORT_SYMBOL(drm_client_release);

+void drm_client_dev_unregister(struct drm_device *dev) +{

• struct drm_client_dev *client, *tmp;
• if (!drm_core_check_feature(dev, DRIVER_MODESET))

• return;
  

• mutex_lock(&dev->clientlist_mutex);
• list_for_each_entry_safe(client, tmp, &dev->clientlist, list) {

• list_del(&client->list);
  

• if (client->funcs && client->funcs->unregister) {
  

• 	client->funcs->unregister(client);
  

Hm, not ->unregister is called while holding the lock. I thought that blows up for you?

It is fine now that we decided that the client can't remove itself. Only the driver can do it on drm_dev_unregister().

I was more wondering about creating an unecessary locking hierarchy complication. But through the ->hotplug and ->restore callbacks we already require that all client locks (which will also include anything related to fbdev and fbcon, hence also console_lock) must nest within dev->clientlist_mutex. That's the part I was worried about, but that's not a good concern really.

So all fine for me on 2nd thought.

...

...

• } else {
  

• 	drm_client_release(client);
  

• 	kfree(client);
  

• }
  

• }
• mutex_unlock(&dev->clientlist_mutex);

+}

Needs a bit of kerneldoc here since exported function. Drivers might also want to call this from their own hotplug handling.

drm_client_dev_hotplug() is only called by drm_kms_helper_hotplug_event(). The reason it's exported is because the helper can be built as a module.

Anything helpers do drivers should be able to override. Anything drivers should be able to use should have docs.

And e.g. drm_sysfs_hotplug_event is actually called by a driver namely vmwgfx, and I think these two functions are pretty much equivalent - one informs userspace clients about a hotplug, the other kernel clients about a hotplug.

Anyway, since my only major concern (the locking question) is cleared up after a bit more think I think this is good for an r-b, once the kerneldoc is all fixed up and the ->release callback gone:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

Cheers, Daniel

Noralf.

...

...

+void drm_client_dev_hotplug(struct drm_device *dev) +{

• struct drm_client_dev *client;
• int ret;
• if (!drm_core_check_feature(dev, DRIVER_MODESET))

• return;
  

• mutex_lock(&dev->clientlist_mutex);
• list_for_each_entry(client, &dev->clientlist, list) {

• if (!client->funcs || !client->funcs->hotplug)
  

• 	continue;
  

• ret = client->funcs->hotplug(client);
  

• DRM_DEV_DEBUG_KMS(dev->dev, "%s: ret=%d\n", client->name, ret);
  

• }
• mutex_unlock(&dev->clientlist_mutex);

+} +EXPORT_SYMBOL(drm_client_dev_hotplug);

+void drm_client_dev_restore(struct drm_device *dev) +{

• struct drm_client_dev *client;
• int ret;
• if (!drm_core_check_feature(dev, DRIVER_MODESET))

• return;
  

• mutex_lock(&dev->clientlist_mutex);
• list_for_each_entry(client, &dev->clientlist, list) {

• if (!client->funcs || !client->funcs->restore)
  

• 	continue;
  

• ret = client->funcs->restore(client);
  

• DRM_DEV_DEBUG_KMS(dev->dev, "%s: ret=%d\n", client->name, ret);
  

• if (!ret) /* The first one to return zero gets the privilege to restore */
  

• 	break;
  

• }
• mutex_unlock(&dev->clientlist_mutex);

+}

• static void drm_client_buffer_delete(struct drm_client_buffer *buffer) { struct drm_device *dev = buffer->client->dev;

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index e7ff0b03328b..6eb935bb2f92 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -34,6 +34,7 @@ #include <linux/slab.h> #include <linux/srcu.h> +#include <drm/drm_client.h> #include <drm/drm_drv.h> #include <drm/drmP.h> @@ -506,6 +507,7 @@ int drm_dev_init(struct drm_device *dev, INIT_LIST_HEAD(&dev->filelist); INIT_LIST_HEAD(&dev->filelist_internal);

• INIT_LIST_HEAD(&dev->clientlist); INIT_LIST_HEAD(&dev->ctxlist); INIT_LIST_HEAD(&dev->vmalist); INIT_LIST_HEAD(&dev->maplist);

@@ -515,6 +517,7 @@ int drm_dev_init(struct drm_device *dev, spin_lock_init(&dev->event_lock); mutex_init(&dev->struct_mutex); mutex_init(&dev->filelist_mutex);

• mutex_init(&dev->clientlist_mutex); mutex_init(&dev->ctxlist_mutex); mutex_init(&dev->master_mutex);

@@ -570,6 +573,7 @@ int drm_dev_init(struct drm_device *dev, err_free: mutex_destroy(&dev->master_mutex); mutex_destroy(&dev->ctxlist_mutex);

• mutex_destroy(&dev->clientlist_mutex); mutex_destroy(&dev->filelist_mutex); mutex_destroy(&dev->struct_mutex); return ret;

@@ -604,6 +608,7 @@ void drm_dev_fini(struct drm_device *dev) mutex_destroy(&dev->master_mutex); mutex_destroy(&dev->ctxlist_mutex);

• mutex_destroy(&dev->clientlist_mutex); mutex_destroy(&dev->filelist_mutex); mutex_destroy(&dev->struct_mutex); kfree(dev->unique);

@@ -859,6 +864,8 @@ void drm_dev_unregister(struct drm_device *dev) dev->registered = false;

• drm_client_dev_unregister(dev);
• if (drm_core_check_feature(dev, DRIVER_MODESET)) drm_modeset_unregister_all(dev);

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 5762a7c441e9..718c7f961d8a 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -181,7 +181,7 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, fb_helper = &fbdev_cma->fb_helper;

• ret = drm_client_new(dev, &fb_helper->client, "fbdev");

• ret = drm_client_new(dev, &fb_helper->client, "fbdev", NULL); if (ret) goto err_free;

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 0a0a577ebc3c..bea3a0cb324a 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -2989,8 +2989,15 @@ static void drm_fbdev_fb_destroy(struct fb_info *info) } drm_client_framebuffer_delete(fb_helper->buffer);

• drm_client_release(&fb_helper->client);
• kfree(fb_helper);

• /*

• * FIXME:
  

• * Remove conditional when all CMA drivers have been moved over to using
  

• * drm_fbdev_generic_setup().
  

• */
  

• if (fb_helper->client.funcs) {

• drm_client_release(&fb_helper->client);
  

• kfree(fb_helper);
  

• } } static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 66bb403dc8ab..ffa8dc35515f 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -35,6 +35,7 @@ #include <linux/slab.h> #include <linux/module.h> +#include <drm/drm_client.h> #include <drm/drm_file.h> #include <drm/drmP.h> @@ -444,6 +445,8 @@ void drm_lastclose(struct drm_device * dev) if (drm_core_check_feature(dev, DRIVER_LEGACY)) drm_legacy_dev_reinit(dev);

• drm_client_dev_restore(dev); } /**

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 527743394150..26be57e28a9d 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -33,6 +33,7 @@ #include <linux/moduleparam.h> #include <drm/drmP.h> +#include <drm/drm_client.h> #include <drm/drm_crtc.h> #include <drm/drm_fourcc.h> #include <drm/drm_crtc_helper.h> @@ -563,6 +564,8 @@ void drm_kms_helper_hotplug_event(struct drm_device *dev) drm_sysfs_hotplug_event(dev); if (dev->mode_config.funcs->output_poll_changed) dev->mode_config.funcs->output_poll_changed(dev);

• drm_client_dev_hotplug(dev); } EXPORT_SYMBOL(drm_kms_helper_hotplug_event);

diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h index e366f95d4414..02cbb02714d8 100644 --- a/include/drm/drm_client.h +++ b/include/drm/drm_client.h @@ -5,10 +5,66 @@ #include <linux/types.h> +struct drm_client_dev; struct drm_device; struct drm_file; struct drm_framebuffer; struct drm_gem_object; +struct module;

+/**

• • struct drm_client_funcs - DRM client callbacks
• */

+struct drm_client_funcs {

• /**

• * @owner: The module owner
  

• */
  

• struct module *owner;
• /**

• * @release:
  

• *
  

• * Called when the reference count is dropped to zero. Users of this
  

• * callback is responsible for calling drm_client_close() and freeing
  

• * the client structure.
  

• *
  

• * This callback is optional.
  

• */
  

• void (*release)(struct drm_client_dev *client);

Hm, is this no longer in use?

...

• /**

• * @unregister:
  

• *
  

• * Called when &drm_device is unregistered. The client should respond by
  

• * releasing it's resources using drm_client_put(). If it can't do so
  

• * due to resoruces being tied up, like fbdev with open file handles,
  

• * it should release it's resources as soon as possible.
  

This still talks about refcounting and _put ... needs a refresher.

...

• *
  

• * This callback is optional.
  

• */
  

• void (*unregister)(struct drm_client_dev *client);
• /**

• * @restore:
  

• *
  

• * Called on drm_lastclose(). The first client instance in the list that
  

• * returns zero gets the privilege to restore and no more clients are
  

• * called. This callback is not called after @unregister has been called.
  

• *
  

• * This callback is optional.
  

• */
  

• int (*restore)(struct drm_client_dev *client);
• /**

• * @hotplug:
  

• *
  

• * Called on drm_kms_helper_hotplug_event().
  

• * This callback is not called after @unregister has been called.
  

• *
  

• * This callback is optional.
  

• */
  

• int (*hotplug)(struct drm_client_dev *client);

+}; /**

• struct drm_client_dev - DRM client instance

@@ -24,6 +80,19 @@ struct drm_client_dev { */ const char *name;

• /**

• * @list:
  

• *
  

• * List of all clients of a DRM device, linked into
  

• * &drm_device.clientlist. Protected by &drm_device.clientlist_mutex.
  

• */
  

• struct list_head list;
• /**

• * @funcs: DRM client functions (optional)
  

• */
  

• const struct drm_client_funcs *funcs;
• /**
  • @file: DRM file
  */

@@ -31,9 +100,13 @@ struct drm_client_dev { }; int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,

•    const char *name);
  

•    const char *name, const struct drm_client_funcs *funcs);
  

  void drm_client_release(struct drm_client_dev *client);

+void drm_client_dev_unregister(struct drm_device *dev); +void drm_client_dev_hotplug(struct drm_device *dev); +void drm_client_dev_restore(struct drm_device *dev);

• /**
  • struct drm_client_buffer - DRM client buffer
  */

diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h index 9e29976d4e98..f9c6e0e3aec7 100644 --- a/include/drm/drm_device.h +++ b/include/drm/drm_device.h @@ -81,6 +81,20 @@ struct drm_device { */ struct list_head filelist_internal;

• /**

• * @clientlist_mutex:
  

• *
  

• * Protects @clientlist access.
  

• */
  

• struct mutex clientlist_mutex;
• /**

• * @clientlist:
  

• *
  

• * List of in-kernel clients. Protected by @clientlist_mutex.
  

• */
  

• struct list_head clientlist;
• /** \name Memory management */ /*@{ */ struct list_head maplist; /**< Linked list of regions */

-- 2.15.1

---

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