On Thu, Jun 25, 2020 at 02:00:11PM +0200, Thomas Zimmermann wrote:
> We register the simplekms device with the DRM platform helpers. A
> native driver for the graphics hardware will kickout the simplekms
> driver before taking over the device.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
> drivers/gpu/drm/tiny/Kconfig | 1 +
> drivers/gpu/drm/tiny/simplekms.c | 94 +++++++++++++++++++++++++++++++-
> 2 files changed, 92 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> index 50dbde8bdcb2..a47ed337a7fe 100644
> --- a/drivers/gpu/drm/tiny/Kconfig
> +++ b/drivers/gpu/drm/tiny/Kconfig
> @@ -33,6 +33,7 @@ config DRM_SIMPLEKMS
> depends on DRM
> select DRM_GEM_SHMEM_HELPER
> select DRM_KMS_HELPER
> + select DRM_PLATFORM_HELPER
> help
> DRM driver for simple platform-provided framebuffers.
>
> diff --git a/drivers/gpu/drm/tiny/simplekms.c b/drivers/gpu/drm/tiny/simplekms.c
> index ae5d3cbadbe8..a903a4e0100a 100644
> --- a/drivers/gpu/drm/tiny/simplekms.c
> +++ b/drivers/gpu/drm/tiny/simplekms.c
> @@ -5,6 +5,7 @@
> #include <linux/platform_data/simplefb.h>
> #include <linux/platform_device.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/spinlock.h>
>
> #include <drm/drm_atomic_state_helper.h>
> #include <drm/drm_connector.h>
> @@ -17,6 +18,7 @@
> #include <drm/drm_gem_shmem_helper.h>
> #include <drm/drm_managed.h>
> #include <drm/drm_modeset_helper_vtables.h>
> +#include <drm/drm_platform.h>
> #include <drm/drm_probe_helper.h>
> #include <drm/drm_simple_kms_helper.h>
>
> @@ -36,6 +38,12 @@
> #define SIMPLEKMS_MODE(hd, vd) \
> DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd))
>
> +/*
> + * Protects the platform device's drvdata against
> + * concurrent manipulation.
> + */
> +static DEFINE_SPINLOCK(simplekms_drvdata_lock);
> +
> /*
> * Helpers for simplefb
> */
> @@ -211,6 +219,7 @@ struct simplekms_device {
> unsigned int pitch;
>
> /* memory management */
> + struct drm_aperture *aperture;
> struct resource *mem;
> void __iomem *screen_base;
>
> @@ -224,6 +233,8 @@ static struct simplekms_device *simplekms_device_of_dev(struct drm_device *dev)
> return container_of(dev, struct simplekms_device, dev);
> }
>
> +static void simplekms_device_cleanup(struct simplekms_device *sdev);
> +
> /*
> * Hardware
> */
> @@ -514,22 +525,72 @@ static int simplekms_device_init_fb(struct simplekms_device *sdev)
> * Memory management
> */
>
> +static void simplekms_aperture_kickout(struct drm_aperture *ap)
> +{
> + struct drm_device *dev = ap->dev;
> + struct simplekms_device *sdev = simplekms_device_of_dev(dev);
> + struct platform_device *pdev = sdev->pdev;
> +
> + if (WARN_ON(!sdev->aperture))
> + return; /* BUG: driver already got kicked out */
> +
> + drm_dev_unregister(dev);
>From a semantic pov I think the platform driver getting kicked out is more
like a hotunplug, so drm_dev_unplug(dev); here is imo better.
That then also gives you a nice drm_dev_enter/exit to sprinkle over the
various driver callbacks, instead of the racy ->aperture check reinvented
wheel here.
I also wonder whether we couldn't go full driver model for these platform
devices, and instead of this here call a core driver model function to
force the unbding of the driver. Only change we'd need it that our
->remove hook uses drm_dev_unplug().
Also I guess this nice plan doesn't work if efifb or vesafb don't have a
platform_device of their own that the drm_platform.c code could use to
nuke platform drivers. But if they have, we could just use
device_driver_detach() from drm_platform.c and wouldn't need any of this.
Worst case efi and vesa drm drivers could instantiate the platform device
they bind against themselves ...
I think that would be a lot cleaner than hand-rolling our own hotunplug
infrastructure here.
Adding Greg in case we're missing anything here.
> +
> + sdev->aperture = NULL; /* memory is released by platform helpers */
> +
> + spin_lock(&simplekms_drvdata_lock);
> + sdev = platform_get_drvdata(pdev);
> + platform_set_drvdata(pdev, NULL); /* required; see simplekms_remove() */
> + spin_unlock(&simplekms_drvdata_lock);
> +
> + /*
> + * Return if a concurrent simplekms_remove() cleans up the
> + * device. See simplekms_remove().
> + */
> + if (!sdev)
> + return;
> +
> + /*
> + * After the aperture has been released, there's no reason
> + * to keep the DRM device around.
> + */
> + simplekms_device_cleanup(sdev);
Uh, you already unregistered, this unregisters again. Maybe a bit too much
:-)
> +}
> +
> +static const struct drm_aperture_funcs simplekms_aperture_funcs = {
> + .kickout = simplekms_aperture_kickout,
> +};
> +
> static int simplekms_device_init_mm(struct simplekms_device *sdev)
> {
> + struct drm_device *dev = &sdev->dev;
> struct platform_device *pdev = sdev->pdev;
> struct resource *mem;
> + struct drm_aperture *ap;
> void __iomem *screen_base;
> + int ret;
>
> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!mem)
> return -EINVAL;
>
> + ap = drmm_aperture_acquire(dev, mem->start, resource_size(mem),
> + &simplekms_aperture_funcs);
> + if (IS_ERR(ap)) {
> + ret = PTR_ERR(ap);
> + drm_err(dev,
> + "could not acquire memory range [0x%llx:0x%llx]: "
> + "error %d\n", mem->start, mem->end, ret);
> + return ret;
> + }
> +
> screen_base = devm_ioremap_wc(&pdev->dev, mem->start,
> resource_size(mem));
> if (!screen_base)
> return -ENOMEM;
>
> sdev->mem = mem;
> + sdev->aperture = ap;
> sdev->screen_base = screen_base;
>
> return 0;
> @@ -625,6 +686,9 @@ simplekms_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
> struct drm_framebuffer *fb = state->fb;
> void *vmap;
>
> + if (!sdev->aperture)
> + return;
> +
> vmap = drm_gem_shmem_vmap(fb->obj[0]);
> if (!vmap)
> return;
> @@ -645,6 +709,9 @@ simplekms_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
> struct drm_rect clip;
> void *vmap;
>
> + if (!sdev->aperture)
> + return;
> +
> if (!drm_atomic_helper_damage_merged(old_plane_state, state, &clip))
> return;
>
> @@ -716,11 +783,12 @@ static int simplekms_device_init_modeset(struct simplekms_device *sdev)
> * Init / Cleanup
> */
>
> -static void simplekms_device_cleanup(struct simplekms_device* sdev)
> +static void simplekms_device_cleanup(struct simplekms_device *sdev)
> {
> struct drm_device *dev = &sdev->dev;
>
> - drm_dev_unregister(dev);
> + if (dev->registered)
> + drm_dev_unregister(dev);
> }
>
> static struct simplekms_device *
> @@ -797,7 +865,27 @@ static int simplekms_probe(struct platform_device *pdev)
>
> static int simplekms_remove(struct platform_device *pdev)
> {
> - struct simplekms_device *sdev = platform_get_drvdata(pdev);
> + struct simplekms_device *sdev;
> +
> + spin_lock(&simplekms_drvdata_lock);
> + sdev = platform_get_drvdata(pdev);
> + platform_set_drvdata(pdev, NULL);
> + spin_unlock(&simplekms_drvdata_lock);
> +
> + /*
> + * The platform driver shares its reference to dev with the
> + * platform helpers for apertures. That reference is either
> + * released here when unloading the driver; or it's released
> + * when the driver gets kicked out by another driver. In the
> + * latter case, the aperture release routine clears the data
> + * field of the platform device.
> + *
> + * Therefore, sdev being NULL is a valid state if the driver
> + * has been kicked out by another DRM driver. In this case,
> + * it's all been cleaned up and we can return immediately.
> + */
> + if (!sdev)
> + return 0;
>
> simplekms_device_cleanup(sdev);
>
> --
> 2.27.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
