Re: [PATCH 9/9] drm/simplekms: Acquire memory aperture for framebuffer

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@suse.de

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