Re: [PATCH 8/9] drm/client: Add client callbacks

On Wed, May 23, 2018 at 04:34:10PM +0200, Noralf Trønnes wrote:

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

Patch nit, but I think the debugfs stuff would be nice if split out.

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

drivers/gpu/drm/drm_client.c | 246 +++++++++++++++++++++++++++++++++++- drivers/gpu/drm/drm_debugfs.c | 7 + drivers/gpu/drm/drm_drv.c | 8 ++ drivers/gpu/drm/drm_fb_cma_helper.c | 2 +- drivers/gpu/drm/drm_file.c | 3 + drivers/gpu/drm/drm_probe_helper.c | 3 + include/drm/drm_client.h | 65 +++++++++- include/drm/drm_device.h | 14 ++ 8 files changed, 345 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c index 0919aea7dddd..c495fcee2058 100644 --- a/drivers/gpu/drm/drm_client.c +++ b/drivers/gpu/drm/drm_client.c @@ -66,11 +66,13 @@ static void drm_client_free_file(struct drm_client_dev *client) /**

• drm_client_new - Create a DRM client
• @dev: DRM device

• • @funcs: DRM client functions
  • Returns:
  • Pointer to a client or an error pointer on failure.
  */

-struct drm_client_dev *drm_client_new(struct drm_device *dev) +struct drm_client_dev * +drm_client_new(struct drm_device *dev, const struct drm_client_funcs *funcs) { struct drm_client_dev *client; int ret; @@ -84,6 +86,7 @@ struct drm_client_dev *drm_client_new(struct drm_device *dev) return ERR_PTR(-ENOMEM);

client->dev = dev;

• client->funcs = funcs;

  ret = drm_client_alloc_file(client); if (ret) {

@@ -91,21 +94,231 @@ struct drm_client_dev *drm_client_new(struct drm_device *dev) return ERR_PTR(ret); }

• /*

• * TODO:
  

• * Temporary hack until all CMA drivers have been moved over to using
  

• * drm_fbdev_generic_setup().
  

• */
  

I think clients without callbacks make perfect sense, imo no need for a TODO. Just explain in the kernel-doc that the callbacks are optional.

• if (!funcs)

• return client;
  

• mutex_lock(&dev->clientlist_mutex);
• list_add(&client->list, &dev->clientlist);
• mutex_unlock(&dev->clientlist_mutex);
• return client;

} EXPORT_SYMBOL(drm_client_new);

+/**

• • drm_client_new_from_id - Create a DRM client from a minor id
• • @minor_id: DRM minor id
• • @funcs: DRM client functions
• • Returns:
• • Pointer to a client or an error pointer on failure.
• */

+struct drm_client_dev * +drm_client_new_from_id(unsigned int minor_id, const struct drm_client_funcs *funcs) +{

• struct drm_client_dev *client;
• struct drm_minor *minor;
• minor = drm_minor_acquire(minor_id);
• if (IS_ERR(minor))

• return ERR_CAST(minor);
  

• client = drm_client_new(minor->dev, funcs);
• drm_minor_release(minor);
• return client;

+} +EXPORT_SYMBOL(drm_client_new_from_id);

I don't get why we need this new function ...

/**

• drm_client_free - Free DRM client resources
• @client: DRM client

• • This is called automatically on client removal unless the client returns
• • non-zero in the &drm_client_funcs->remove callback. The fbdev client does
• • this when it can't close &drm_file because userspace has an open fd.
• • Note:
• • If the client can't release it's resources on remove, it needs to hold a
• • reference on the driver module to prevent the code from going away.

We need a full reference on the drm_device I think, not just the code. I thought drm_file provides that for any drm_client?

*/ void drm_client_free(struct drm_client_dev *client) {

• DRM_DEV_DEBUG_KMS(client->dev->dev, "\n");

Both above bits probably should be in the patch that introduces these?

drm_client_free_file(client); kfree(client); } EXPORT_SYMBOL(drm_client_free);

+static void drm_client_remove_locked(struct drm_client_dev *client) +{

• list_del(&client->list);
• if (!client->funcs->remove || !client->funcs->remove(client))

• drm_client_free(client);
  

Ugh, I don't like this semantics of ->remove. Imo you really only want an ->unregister here, an _not want to automatically free the client. Automaticlly freeing the client prevents embedded, and all around smells a bit like midlayer mistake.

Without the automatic freeing you also don't need the return value, and you can have a more standard void return type for your unregister function.

We might need to make sure the drm_dev_unregister doesn't free up the drm_device while we do that, so might need a temporary reference. But I don't think we need anything else to avoid locking headaches.

+}

+static void drm_client_remove_safe(struct drm_device *dev,

• 		   struct drm_client_dev *client)
  

+{

• struct drm_client_dev *iter;
• mutex_lock(&dev->clientlist_mutex);
• list_for_each_entry(iter, &dev->clientlist, list) {

• if (iter == client) {
  

• 	drm_client_remove_locked(client);
  

• 	break;
  

• }
  

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

+}

+/**

• • drm_client_remove - Remove client
• • @client: Client
• • Remove the DRM client.
• */

+void drm_client_remove(struct drm_client_dev *client) +{

• struct drm_device *dev;
• if (!client)

• return;
  

• dev = client->dev;
• /* Hold a reference since the client might drop the last one */
• drm_dev_get(dev);
• drm_client_remove_safe(dev, client);
• drm_dev_put(dev);

+} +EXPORT_SYMBOL(drm_client_remove);

No idea what's this for, seems unused. Please remove - we can readd later on.

+struct drm_client_remove_defer {

• struct list_head list;
• struct drm_device *dev;
• struct drm_client_dev *client;

+};

+static LIST_HEAD(drm_client_remove_defer_list); +static DEFINE_MUTEX(drm_client_remove_defer_list_lock);

+static void drm_client_remove_defer_work_fn(struct work_struct *work) +{

• struct drm_client_remove_defer *defer, *tmp;
• mutex_lock(&drm_client_remove_defer_list_lock);
• list_for_each_entry_safe(defer, tmp, &drm_client_remove_defer_list, list) {

• drm_client_remove_safe(defer->dev, defer->client);
  

• drm_dev_put(defer->dev);
  

• list_del(&defer->list);
  

• kfree(defer);
  

• }
• mutex_unlock(&drm_client_remove_defer_list_lock);

+}

+static DECLARE_WORK(drm_client_remove_defer_work, drm_client_remove_defer_work_fn);

+/**

• • drm_client_remove_defer - Deferred client removal
• • @client: Client
• • Defer client removal to a worker. This makes it possible for a client running
• • in a worker to remove itself.
• • Returns:
• • Zero on success, or -ENOMEM on allocation failure.
• */

+int drm_client_remove_defer(struct drm_client_dev *client) +{

• struct drm_client_remove_defer *defer;
• if (!client)

• return 0;
  

• defer = kzalloc(sizeof(*defer), GFP_KERNEL);
• if (!defer)

• return -ENOMEM;
  

• defer->dev = client->dev;
• defer->client = client;
• drm_dev_get(client->dev);
• mutex_lock(&drm_client_remove_defer_list_lock);
• list_add(&defer->list, &drm_client_remove_defer_list);
• mutex_unlock(&drm_client_remove_defer_list_lock);
• schedule_work(&drm_client_remove_defer_work);
• return 0;

+} +EXPORT_SYMBOL(drm_client_remove_defer);

+void drm_client_exit(void) +{

• flush_work(&drm_client_remove_defer_work);

+}

All the above deferred client removal seems unused. And I can't come up with a reason for why we need this. Please remove, we can add it when we need it later on.

+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)

• drm_client_remove_locked(client);
  

• mutex_unlock(&dev->clientlist_mutex);

+}

+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->hotplug)
  

• 	continue;
  

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

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

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

+} +EXPORT_SYMBOL(drm_client_dev_hotplug);

+void drm_client_dev_lastclose(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->lastclose)
  

• 	continue;
  

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

Hm, it's called from lastclose context, but conceptually this isn't what's happening at all. I'd call both the callback and the function here ->restore respectively drm_client_dev_restore. Makes it much clearer what's going on, and why this one here is special.

• DRM_DEV_DEBUG_KMS(dev->dev, "%s: ret=%d\n", client->funcs->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; @@ -218,6 +431,9 @@ static int drm_client_buffer_addfb(struct drm_client_buffer *buffer, /* drop the reference we picked up in framebuffer lookup */ drm_framebuffer_put(buffer->fb);

• if (client->funcs)

• strscpy(buffer->fb->comm, client->funcs->name, TASK_COMM_LEN);
  

Hm, small nit: I'd keep the client name in drm_client_dev, not in the funcs structure. That way you could dynamically assign it if needed. The important part is to keep the functions separate from anything that can change, so that we can put it into protected ro memory.

Feel free to ignore this :-)

• return 0;

}

@@ -265,3 +481,31 @@ void drm_client_framebuffer_delete(struct drm_client_buffer *buffer) drm_client_buffer_delete(buffer); } EXPORT_SYMBOL(drm_client_framebuffer_delete);

+#ifdef CONFIG_DEBUG_FS +static int drm_client_debugfs_internal_clients(struct seq_file *m, void *data) +{

• struct drm_info_node *node = m->private;
• struct drm_device *dev = node->minor->dev;
• struct drm_printer p = drm_seq_file_printer(m);
• struct drm_client_dev *client;
• mutex_lock(&dev->clientlist_mutex);
• list_for_each_entry(client, &dev->clientlist, list)

• drm_printf(&p, "%s\n", client->funcs->name);
  

• mutex_unlock(&dev->clientlist_mutex);
• return 0;

+}

+static const struct drm_info_list drm_client_debugfs_list[] = {

• { "internal_clients", drm_client_debugfs_internal_clients, 0 },

+};

+int drm_client_debugfs_init(struct drm_minor *minor) +{

• return drm_debugfs_create_files(drm_client_debugfs_list,

• 			ARRAY_SIZE(drm_client_debugfs_list),
  

• 			minor->debugfs_root, minor);
  

+} +#endif diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c index b2482818fee8..50a20bfc07ea 100644 --- a/drivers/gpu/drm/drm_debugfs.c +++ b/drivers/gpu/drm/drm_debugfs.c @@ -28,6 +28,7 @@ #include <linux/slab.h> #include <linux/export.h>

+#include <drm/drm_client.h> #include <drm/drm_debugfs.h> #include <drm/drm_edid.h> #include <drm/drm_atomic.h> @@ -164,6 +165,12 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, DRM_ERROR("Failed to create framebuffer debugfs file\n"); return ret; }

• ret = drm_client_debugfs_init(minor);
  

• if (ret) {
  

• 	DRM_ERROR("Failed to create client debugfs file\n");
  

• 	return ret;
  

• }
  

  }

  if (dev->driver->debugfs_init) {

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 67ac793a7108..d7c1ceebd0de 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);

@@ -854,6 +859,8 @@ void drm_dev_unregister(struct drm_device *dev) { struct drm_map_list *r_list, *list_temp;

• drm_client_dev_unregister(dev);
• if (drm_core_check_feature(dev, DRIVER_LEGACY)) drm_lastclose(dev);

@@ -959,6 +966,7 @@ static const struct file_operations drm_stub_fops = {

static void drm_core_exit(void) {

• drm_client_exit(); unregister_chrdev(DRM_MAJOR, "drm"); debugfs_remove(drm_debugfs_root); drm_sysfs_destroy();

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 76e2f7977779..aff3c215d9e5 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -180,7 +180,7 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, if (!fbdev_cma) return ERR_PTR(-ENOMEM);

• client = drm_client_new(dev);

• client = drm_client_new(dev, NULL); if (IS_ERR(client)) { ret = PTR_ERR(client); goto err_free;

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 55505378df47..bcc688e58776 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>

@@ -443,6 +444,8 @@ void drm_lastclose(struct drm_device * dev)

if (drm_core_check_feature(dev, DRIVER_LEGACY)) drm_legacy_dev_reinit(dev);

• drm_client_dev_lastclose(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 11379eaf3118..011a87ad3406 100644 --- a/include/drm/drm_client.h +++ b/include/drm/drm_client.h @@ -5,11 +5,56 @@

#include <linux/types.h>

+struct drm_client_dev; struct drm_device; struct drm_framebuffer; struct drm_gem_object; struct drm_minor;

+/**

• • struct drm_client_funcs - DRM client callbacks
• */

+struct drm_client_funcs {

• /**

• * @name:
  

• *
  

• * Name of the client. Mandatory.
  

• */
  

• const char *name;
• /**

• * @remove:
  

• *
  

• * Called when a &drm_device is unregistered or the client is
  

• * unregistered. If zero is returned drm_client_free() is called
  

• * automatically. If the client can't drop it's resources it should
  

• * return non-zero and call drm_client_free() later.
  

• *
  

• * This callback is optional.
  

• */
  

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

s/remove/unregister/ and void return type. See above.

Also we should document here that once ->unregister is call, we will no longer call ->restore nor ->hotplug.

• /**

• * @lastclose:
  

• *
  

• * 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 optional.
  

• */
  

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

I'd call this restore. Makes it more obvious what it's for.

• /**

• * @hotplug:
  

• *
  

• * Called on drm_kms_helper_hotplug_event().
  

• *
  

• * This callback is optional.
  

• */
  

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

+};

/**

• struct drm_client_dev - DRM client instance

*/ @@ -27,6 +72,11 @@ struct drm_client_dev { */ struct drm_device *dev;

• /**

• * @funcs: DRM client functions
  

• */
  

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

@@ -39,7 +89,20 @@ struct drm_client_dev { };

void drm_client_free(struct drm_client_dev *client); -struct drm_client_dev *drm_client_new(struct drm_device *dev); +struct drm_client_dev * +drm_client_new(struct drm_device *dev, const struct drm_client_funcs *funcs); +struct drm_client_dev * +drm_client_new_from_id(unsigned int minor_id, const struct drm_client_funcs *funcs); +void drm_client_remove(struct drm_client_dev *client); +int drm_client_remove_defer(struct drm_client_dev *client);

+void drm_client_exit(void);

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

+int drm_client_debugfs_init(struct drm_minor *minor);

/**

• 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 */

lgtm overall, just a few nits wrt naming stuff, and splitting things out that we dont (yet) need. Plus the unregister semantics don't look fully thought out to me yet. -Daniel

-- 2.15.1

---

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