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

[Daniel Vetter <daniel@ffwll.ch>]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel