From: "Noralf Trønnes" <noralf@tronnes.org>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org,
laurent.pinchart@ideasonboard.com,
dri-devel@lists.freedesktop.org, david@lechnology.com
Subject: Re: [PATCH v4 5/9] drm/client: Add client callbacks
Date: Tue, 3 Jul 2018 15:07:50 +0200 [thread overview]
Message-ID: <b91ec369-2a4e-3055-a40e-f26d763bbc53@tronnes.org> (raw)
In-Reply-To: <20180703074605.GF7880@phenom.ffwll.local>
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().
>> + } 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.
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2018-07-03 13:07 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-02 13:54 [PATCH v4 0/9] drm: Add generic fbdev emulation Noralf Trønnes
2018-07-02 13:54 ` [PATCH v4 1/9] drm: Begin an API for in-kernel clients Noralf Trønnes
2018-07-03 7:43 ` Daniel Vetter
2018-07-02 13:54 ` [PATCH v4 2/9] drm/fb-helper: Add generic fbdev emulation .fb_probe function Noralf Trønnes
2018-07-02 13:54 ` [PATCH v4 3/9] drm/pl111: Set .gem_prime_vmap and .gem_prime_mmap Noralf Trønnes
2018-07-02 13:54 ` [PATCH v4 4/9] drm/cma-helper: Use the generic fbdev emulation Noralf Trønnes
2018-07-02 13:54 ` [PATCH v4 5/9] drm/client: Add client callbacks Noralf Trønnes
2018-07-03 7:46 ` Daniel Vetter
2018-07-03 13:07 ` Noralf Trønnes [this message]
2018-07-03 13:18 ` Daniel Vetter
2018-07-02 13:54 ` [PATCH v4 6/9] drm/debugfs: Add internal client debugfs file Noralf Trønnes
2018-07-02 13:54 ` [PATCH v4 7/9] drm/fb-helper: Finish the generic fbdev emulation Noralf Trønnes
2018-07-02 13:54 ` [PATCH v4 8/9] drm/tinydrm: Use drm_fbdev_generic_setup() Noralf Trønnes
2018-07-02 15:52 ` David Lechner
2018-07-02 13:54 ` [PATCH v4 9/9] drm/cma-helper: Remove drm_fb_cma_fbdev_init_with_funcs() Noralf Trønnes
2018-07-02 15:57 ` David Lechner
2018-07-02 14:13 ` ✗ Fi.CI.CHECKPATCH: warning for drm: Add generic fbdev emulation Patchwork
2018-07-02 14:17 ` ✗ Fi.CI.SPARSE: " Patchwork
2018-07-02 14:28 ` ✓ Fi.CI.BAT: success " Patchwork
2018-07-02 15:40 ` ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b91ec369-2a4e-3055-a40e-f26d763bbc53@tronnes.org \
--to=noralf@tronnes.org \
--cc=daniel@ffwll.ch \
--cc=david@lechnology.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=laurent.pinchart@ideasonboard.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.