[PATCH 6/6] drm/tinydrm: Support device unplug
Noralf Trønnes
noralf at tronnes.org
Wed Aug 30 16:31:36 UTC 2017
Den 28.08.2017 23.56, skrev Daniel Vetter:
> On Mon, Aug 28, 2017 at 07:17:48PM +0200, Noralf Trønnes wrote:
>> Support device unplugging to make tinydrm suitable for USB devices.
>>
>> Cc: David Lechner <david at lechnology.com>
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>> ---
>> drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 69 ++++++++++++++++++++++++-----
>> drivers/gpu/drm/tinydrm/mi0283qt.c | 4 ++
>> drivers/gpu/drm/tinydrm/mipi-dbi.c | 5 ++-
>> drivers/gpu/drm/tinydrm/repaper.c | 9 +++-
>> drivers/gpu/drm/tinydrm/st7586.c | 9 +++-
>> include/drm/tinydrm/tinydrm.h | 5 +++
>> 6 files changed, 87 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
>> index f11f4cd..3ccbcc5 100644
>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
>> @@ -32,6 +32,29 @@
>> * The driver allocates &tinydrm_device, initializes it using
>> * devm_tinydrm_init(), sets up the pipeline using tinydrm_display_pipe_init()
>> * and registers the DRM device using devm_tinydrm_register().
>> + *
>> + * Device unplug
>> + * -------------
>> + *
>> + * tinydrm supports device unplugging when there's still open DRM or fbdev file
>> + * handles.
>> + *
>> + * There are 2 ways for driver-device unbinding to happen:
>> + *
>> + * - The driver module is unloaded causing the driver to be unregistered.
>> + * This can't happen as long as there's open file handles because a reference
>> + * is taken on the module.
> Aside: you can do that, but then it works like a hw unplug, through the
> unbind property in sysfs.
The module is still pinned isn't it?
I can add sysfs unbind as a third way of triggering unbind:
* - The sysfs driver _unbind_ file can be used to unbind the driver
form the
* device. This can happen any time.
*
>> + *
>> + * - The device is removed (USB, Device Tree overlay).
>> + * This can happen at any time.
>> + *
>> + * The driver needs to protect device resources from access after the device is
>> + * gone. This is done checking drm_dev_is_unplugged(), typically in
>> + * &drm_framebuffer_funcs.dirty, &drm_simple_display_pipe_funcs.enable and
>> + * \.disable. Resources that doesn't face userspace and is only used with the
>> + * device can be setup using devm\_ functions, but &tinydrm_device must be
>> + * allocated using plain kzalloc() since it's lifetime can exceed that of the
>> + * device. tinydrm_release() will free the structure.
> So here's a bit a dragon: There's no prevention of is_unplugged racing
> against a drm_dev_unplug(). There's been various attempts to fixing this,
> but they're all somewhat ugly.
>
> Either way, that's a bug in the drm core :-)
>
>> */
>>
>> /**
>> @@ -138,6 +161,29 @@ static const struct drm_mode_config_funcs tinydrm_mode_config_funcs = {
>> .atomic_commit = drm_atomic_helper_commit,
>> };
>>
>> +/**
>> + * tinydrm_release - DRM driver release helper
>> + * @drm: DRM device
>> + *
>> + * This function cleans up and finalizes &drm_device and frees &tinydrm_device.
>> + *
>> + * Drivers must use this as their &drm_driver->release callback.
>> + */
>> +void tinydrm_release(struct drm_device *drm)
>> +{
>> + struct tinydrm_device *tdev = drm_to_tinydrm(drm);
>> +
>> + DRM_DEBUG_DRIVER("\n");
>> +
>> + drm_mode_config_cleanup(drm);
>> + drm_dev_fini(drm);
>> +
>> + mutex_destroy(&tdev->dirty_lock);
>> + kfree(tdev->fbdev_cma);
>> + kfree(tdev);
>> +}
>> +EXPORT_SYMBOL(tinydrm_release);
>> +
>> static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
>> const struct drm_framebuffer_funcs *fb_funcs,
>> struct drm_driver *driver)
>> @@ -160,8 +206,6 @@ static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
>>
>> static void tinydrm_fini(struct tinydrm_device *tdev)
>> {
>> - drm_mode_config_cleanup(&tdev->drm);
>> - mutex_destroy(&tdev->dirty_lock);
>> drm_dev_unref(&tdev->drm);
>> }
>>
>> @@ -178,8 +222,8 @@ static void devm_tinydrm_release(void *data)
>> * @driver: DRM driver
>> *
>> * This function initializes @tdev, the underlying DRM device and it's
>> - * mode_config. Resources will be automatically freed on driver detach (devres)
>> - * using drm_mode_config_cleanup() and drm_dev_unref().
>> + * mode_config. drm_dev_unref() is called on driver detach (devres) and when
>> + * all refs are dropped, tinydrm_release() is called.
>> *
>> * Returns:
>> * Zero on success, negative error code on failure.
>> @@ -226,14 +270,17 @@ static int tinydrm_register(struct tinydrm_device *tdev)
>>
>> static void tinydrm_unregister(struct tinydrm_device *tdev)
>> {
>> - struct drm_fbdev_cma *fbdev_cma = tdev->fbdev_cma;
>> -
>> drm_atomic_helper_shutdown(&tdev->drm);
>> - /* don't restore fbdev in lastclose, keep pipeline disabled */
>> - tdev->fbdev_cma = NULL;
>> - drm_dev_unregister(&tdev->drm);
>> - if (fbdev_cma)
>> - drm_fbdev_cma_fini(fbdev_cma);
>> +
>> + /* Get a ref that will be put in tinydrm_fini() */
>> + drm_dev_ref(&tdev->drm);
> Why do we need that private ref? Grabbing references in unregister code
> looks like a recipe for leaks ...
Yeah, it's better to take the second ref in devm_tinydrm_register().
The reason I need 2 refs is because tinydrm is set up with 2 devm_
functions. This way I can leave all error path cleanup in the hands of
devres.
static int driver_probe(...)
{
ret = devm_tinydrm_init(...);
if (ret)
return ret;
ret = tinydrm_display_pipe_init(...);
if (ret)
return ret;
drm_mode_config_reset(...);
return devm_tinydrm_register(...);
}
>> +
>> + drm_fbdev_cma_dev_unplug(tdev->fbdev_cma);
>> + drm_dev_unplug(&tdev->drm);
>> +
>> + /* Make sure framebuffer flushing is done */
>> + mutex_lock(&tdev->dirty_lock);
>> + mutex_unlock(&tdev->dirty_lock);
> Is this really needed? Or, doesn't it just paper over a driver bug you
> have already anyway, since native kms userspace can directly call
> fb->funcs->dirty too, and you already protect against that.
>
> This definitely looks like the fbdev helper is leaking implementation
> details to callers where it shouldn't do that.
Flushing can happen while drm_dev_unplug() is called, and when we leave
this function the device facing resources controlled by devres will be
removed. Thus I have to make sure any such flushing is done before
leaving so the next flush is stopped by the drm_dev_is_unplugged() check.
I don't see any other way of ensuring that.
I see now that I should move the call to drm_atomic_helper_shutdown() after
drm_dev_unplug() to properly protect the pipe .enable/.disable callbacks.
Noralf.
>> }
>>
>> static void devm_tinydrm_register_release(void *data)
>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> index 2465489..84ab8d1 100644
>> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
>> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> @@ -31,6 +31,9 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe,
>>
>> DRM_DEBUG_KMS("\n");
>>
>> + if (drm_dev_is_unplugged(&tdev->drm))
>> + return;
>> +
>> ret = regulator_enable(mipi->regulator);
>> if (ret) {
>> dev_err(dev, "Failed to enable regulator (%d)\n", ret);
>> @@ -133,6 +136,7 @@ static struct drm_driver mi0283qt_driver = {
>> DRIVER_ATOMIC,
>> .fops = &mi0283qt_fops,
>> TINYDRM_GEM_DRIVER_OPS,
>> + .release = tinydrm_release,
>> .lastclose = tinydrm_lastclose,
>> .debugfs_init = mipi_dbi_debugfs_init,
>> .name = "mi0283qt",
>> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> index f5b9b772..49d03ab 100644
>> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> @@ -209,7 +209,7 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
>>
>> mutex_lock(&tdev->dirty_lock);
>>
>> - if (!mipi->enabled)
>> + if (!mipi->enabled || drm_dev_is_unplugged(fb->dev))
>> goto out_unlock;
>>
>> /* fbdev can flush even when we're not interested */
>> @@ -314,6 +314,9 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
>>
>> DRM_DEBUG_KMS("\n");
>>
>> + if (drm_dev_is_unplugged(&tdev->drm))
>> + return;
>> +
>> mipi->enabled = false;
>>
>> if (mipi->backlight)
>> diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c
>> index b8fc8eb..3869d362 100644
>> --- a/drivers/gpu/drm/tinydrm/repaper.c
>> +++ b/drivers/gpu/drm/tinydrm/repaper.c
>> @@ -542,7 +542,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb,
>>
>> mutex_lock(&tdev->dirty_lock);
>>
>> - if (!epd->enabled)
>> + if (!epd->enabled || drm_dev_is_unplugged(fb->dev))
>> goto out_unlock;
>>
>> /* fbdev can flush even when we're not interested */
>> @@ -670,6 +670,9 @@ static void repaper_pipe_enable(struct drm_simple_display_pipe *pipe,
>>
>> DRM_DEBUG_DRIVER("\n");
>>
>> + if (drm_dev_is_unplugged(&tdev->drm))
>> + return;
>> +
>> /* Power up sequence */
>> gpiod_set_value_cansleep(epd->reset, 0);
>> gpiod_set_value_cansleep(epd->panel_on, 0);
>> @@ -803,6 +806,9 @@ static void repaper_pipe_disable(struct drm_simple_display_pipe *pipe)
>>
>> DRM_DEBUG_DRIVER("\n");
>>
>> + if (drm_dev_is_unplugged(&tdev->drm))
>> + return;
>> +
>> mutex_lock(&tdev->dirty_lock);
>> epd->enabled = false;
>> mutex_unlock(&tdev->dirty_lock);
>> @@ -894,6 +900,7 @@ static struct drm_driver repaper_driver = {
>> DRIVER_ATOMIC,
>> .fops = &repaper_fops,
>> TINYDRM_GEM_DRIVER_OPS,
>> + .release = tinydrm_release,
>> .name = "repaper",
>> .desc = "Pervasive Displays RePaper e-ink panels",
>> .date = "20170405",
>> diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
>> index bc2b905..b1bfc4e 100644
>> --- a/drivers/gpu/drm/tinydrm/st7586.c
>> +++ b/drivers/gpu/drm/tinydrm/st7586.c
>> @@ -120,7 +120,7 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb,
>>
>> mutex_lock(&tdev->dirty_lock);
>>
>> - if (!mipi->enabled)
>> + if (!mipi->enabled || drm_dev_is_unplugged(fb->dev))
>> goto out_unlock;
>>
>> /* fbdev can flush even when we're not interested */
>> @@ -184,6 +184,9 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
>>
>> DRM_DEBUG_KMS("\n");
>>
>> + if (drm_dev_is_unplugged(&tdev->drm))
>> + return;
>> +
>> mipi_dbi_hw_reset(mipi);
>> ret = mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
>> if (ret) {
>> @@ -252,6 +255,9 @@ static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
>>
>> DRM_DEBUG_KMS("\n");
>>
>> + if (drm_dev_is_unplugged(&tdev->drm))
>> + return;
>> +
>> if (!mipi->enabled)
>> return;
>>
>> @@ -319,6 +325,7 @@ static struct drm_driver st7586_driver = {
>> DRIVER_ATOMIC,
>> .fops = &st7586_fops,
>> TINYDRM_GEM_DRIVER_OPS,
>> + .release = tinydrm_release,
>> .lastclose = tinydrm_lastclose,
>> .debugfs_init = mipi_dbi_debugfs_init,
>> .name = "st7586",
>> diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h
>> index ded9817..4265f51 100644
>> --- a/include/drm/tinydrm/tinydrm.h
>> +++ b/include/drm/tinydrm/tinydrm.h
>> @@ -23,6 +23,10 @@
>> * @fbdev_cma: CMA fbdev structure
>> * @suspend_state: Atomic state when suspended
>> * @fb_funcs: Framebuffer functions used when creating framebuffers
>> + *
>> + * Drivers that embed &tinydrm_device must set it as the first member because
>> + * it is freed in tinydrm_release(). This means that the structure must be
>> + * allocated with kzalloc() and not the devm\_ version.
>> */
>> struct tinydrm_device {
>> struct drm_device drm;
>> @@ -94,6 +98,7 @@ struct drm_gem_object *
>> tinydrm_gem_cma_prime_import_sg_table(struct drm_device *drm,
>> struct dma_buf_attachment *attach,
>> struct sg_table *sgt);
>> +void tinydrm_release(struct drm_device *drm);
>> int devm_tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
>> const struct drm_framebuffer_funcs *fb_funcs,
>> struct drm_driver *driver);
>> --
>> 2.7.4
>>
More information about the dri-devel
mailing list