Re: [PATCH 8/9] drm: Add infrastructure for platform devices

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 25, 2020 at 02:00:10PM +0200, Thomas Zimmermann wrote:
> Platform devices might operate on firmware framebuffers, such as VESA or
> EFI. Before a native driver for the graphics hardware can take over the
> device, it has to remove any platform driver that operates on the firmware
> framebuffer. Platform helpers provide the infrastructure for platform
> drivers to acquire firmware framebuffers, and for native drivers to remove
> them lateron.
> 
> It works similar to the related fbdev mechanism. During initialization, the
> platform driver acquires the firmware framebuffer's I/O memory and provides
> a callback to be removed. The native driver later uses this inforamtion to
> remove any platform driver for it's framebuffer I/O memory.
> 
> The platform helper's removal code is integrated into the existing code for
> removing conflicting fraembuffers, so native drivers use it automatically.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

I have some ideas for how to do this a notch cleaner in the next patch.
Maybe best to discuss the actual implmenentation stuff there.

Aside from that usual nits:
- kerneldoc for these please, pulled into drm-kms.rst.
- naming isn't super ocd with drm_platform.c but that prefix not used, but
  I also don't have better ideas.
- I think the functions from drm_fb_helper.h for removing other
  framebuffers should be moved here, and function name prefix adjusted
  acoordingly

I'm wondering about the locking and deadlock potential here, is lockdep
all happy with this?

Cheers, Daniel

> ---
>  drivers/gpu/drm/Kconfig        |   6 ++
>  drivers/gpu/drm/Makefile       |   1 +
>  drivers/gpu/drm/drm_platform.c | 118 +++++++++++++++++++++++++++++++++
>  include/drm/drm_fb_helper.h    |  18 ++++-
>  include/drm/drm_platform.h     |  42 ++++++++++++
>  5 files changed, 184 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_platform.c
>  create mode 100644 include/drm/drm_platform.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index c4fd57d8b717..e9d6892f9d38 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -229,6 +229,12 @@ config DRM_SCHED
>  	tristate
>  	depends on DRM
>  
> +config DRM_PLATFORM_HELPER
> +	bool
> +	depends on DRM
> +	help
> +	  Helpers for DRM platform devices
> +
>  source "drivers/gpu/drm/i2c/Kconfig"
>  
>  source "drivers/gpu/drm/arm/Kconfig"
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 2c0e5a7e5953..8ceb21d0770a 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -32,6 +32,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
>  drm-$(CONFIG_PCI) += drm_pci.o
>  drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
>  drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> +drm-$(CONFIG_DRM_PLATFORM_HELPER) += drm_platform.o
>  
>  drm_vram_helper-y := drm_gem_vram_helper.o
>  obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o
> diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
> new file mode 100644
> index 000000000000..09a2f2a31aa5
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_platform.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +
> +#include <drm/drm_drv.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_platform.h>
> +
> +static LIST_HEAD(drm_apertures);
> +
> +static DEFINE_MUTEX(drm_apertures_lock);
> +
> +static bool overlap(resource_size_t base1, resource_size_t end1,
> +		    resource_size_t base2, resource_size_t end2)
> +{
> +	return (base1 < end2) && (end1 > base2);
> +}
> +
> +static struct drm_aperture *
> +drm_aperture_acquire(struct drm_device *dev,
> +		     resource_size_t base, resource_size_t size,
> +		     const struct drm_aperture_funcs *funcs)
> +{
> +	size_t end = base + size;
> +	struct list_head *pos;
> +	struct drm_aperture *ap;
> +
> +	mutex_lock(&drm_apertures_lock);
> +
> +	list_for_each(pos, &drm_apertures) {
> +		ap = container_of(pos, struct drm_aperture, lh);
> +		if (overlap(base, end, ap->base, ap->base + ap->size))
> +			return ERR_PTR(-EBUSY);
> +	}
> +
> +	ap = drmm_kzalloc(dev, sizeof(*ap), GFP_KERNEL);
> +	if (!ap)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ap->dev = dev;
> +	ap->base = base;
> +	ap->size = size;
> +	ap->funcs = funcs;
> +	INIT_LIST_HEAD(&ap->lh);
> +
> +	list_add(&ap->lh, &drm_apertures);
> +
> +	mutex_unlock(&drm_apertures_lock);
> +
> +	return ap;
> +}
> +
> +static void drm_aperture_release(struct drm_aperture *ap)
> +{
> +	bool kicked_out = ap->kicked_out;
> +
> +	if (!kicked_out)
> +		mutex_lock(&drm_apertures_lock);
> +
> +	list_del(&ap->lh);
> +	if (ap->funcs->release)
> +		ap->funcs->release(ap);
> +
> +	if (!kicked_out)
> +		mutex_unlock(&drm_apertures_lock);
> +}
> +
> +static void drm_aperture_acquire_release(struct drm_device *dev, void *ptr)
> +{
> +	struct drm_aperture *ap = ptr;
> +
> +	drm_aperture_release(ap);
> +}
> +
> +struct drm_aperture *
> +drmm_aperture_acquire(struct drm_device *dev,
> +		      resource_size_t base, resource_size_t size,
> +		      const struct drm_aperture_funcs *funcs)
> +{
> +	struct drm_aperture *ap;
> +	int ret;
> +
> +	ap = drm_aperture_acquire(dev, base, size, funcs);
> +	if (IS_ERR(ap))
> +		return ap;
> +	ret = drmm_add_action_or_reset(dev, drm_aperture_acquire_release, ap);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return ap;
> +}
> +EXPORT_SYMBOL(drmm_aperture_acquire);
> +
> +void drm_kickout_apertures_at(resource_size_t base, resource_size_t size)
> +{
> +	resource_size_t end = base + size;
> +	struct list_head *pos, *n;
> +
> +	mutex_lock(&drm_apertures_lock);
> +
> +	list_for_each_safe(pos, n, &drm_apertures) {
> +		struct drm_aperture *ap =
> +			container_of(pos, struct drm_aperture, lh);
> +
> +		if (!overlap(base, end, ap->base, ap->base + ap->size))
> +			continue;
> +
> +		ap->kicked_out = true;
> +		if (ap->funcs->kickout)
> +			ap->funcs->kickout(ap);
> +		else
> +			drm_dev_put(ap->dev);
> +	}
> +
> +	mutex_unlock(&drm_apertures_lock);
> +}
> +EXPORT_SYMBOL(drm_kickout_apertures_at);
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 306aa3a60be9..a919b78b1961 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -35,7 +35,9 @@ struct drm_fb_helper;
>  #include <drm/drm_client.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_device.h>
> +#include <drm/drm_platform.h>
>  #include <linux/kgdb.h>
> +#include <linux/pci.h>
>  #include <linux/vgaarb.h>
>  
>  enum mode_set_atomic {
> @@ -465,6 +467,11 @@ static inline int
>  drm_fb_helper_remove_conflicting_framebuffers(struct apertures_struct *a,
>  					      const char *name, bool primary)
>  {
> +	int i;
> +
> +	for (i = 0; i < a->count; ++i)
> +		drm_kickout_apertures_at(a->ranges[i].base, a->ranges[i].size);
> +
>  #if IS_REACHABLE(CONFIG_FB)
>  	return remove_conflicting_framebuffers(a, name, primary);
>  #else
> @@ -487,7 +494,16 @@ static inline int
>  drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
>  						  const char *name)
>  {
> -	int ret = 0;
> +	resource_size_t base, size;
> +	int bar, ret = 0;
> +
> +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> +		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
> +			continue;
> +		base = pci_resource_start(pdev, bar);
> +		size = pci_resource_len(pdev, bar);
> +		drm_kickout_apertures_at(base, size);
> +	}
>  
>  	/*
>  	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
> diff --git a/include/drm/drm_platform.h b/include/drm/drm_platform.h
> new file mode 100644
> index 000000000000..475e88ee1fbd
> --- /dev/null
> +++ b/include/drm/drm_platform.h
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +#ifndef _DRM_PLATFORM_H_
> +#define _DRM_PLATFORM_H_
> +
> +#include <linux/list.h>
> +#include <linux/types.h>
> +
> +struct drm_aperture;
> +struct drm_device;
> +
> +struct drm_aperture_funcs {
> +	void (*kickout)(struct drm_aperture *ap);
> +	void (*release)(struct drm_aperture *ap);
> +};
> +
> +struct drm_aperture {
> +	struct drm_device *dev;
> +	resource_size_t base;
> +	resource_size_t size;
> +
> +	const struct drm_aperture_funcs *funcs;
> +
> +	struct list_head lh;
> +	bool kicked_out;
> +};
> +
> +struct drm_aperture *
> +drmm_aperture_acquire(struct drm_device *dev,
> +		      resource_size_t base, resource_size_t size,
> +		      const struct drm_aperture_funcs *funcs);
> +
> +#if defined (CONFIG_DRM_PLATFORM_HELPER)
> +void drm_kickout_apertures_at(resource_size_t base, resource_size_t size);
> +#else
> +static inline void
> +drm_kickout_apertures_at(resource_size_t base, resource_size_t size)
> +{
> +}
> +#endif
> +
> +#endif
> -- 
> 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