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

Hi

Am 28.09.20 um 10:50 schrieb Daniel Vetter:

On Mon, Sep 28, 2020 at 10:40 AM Thomas Zimmermann tzimmermann@suse.de wrote:

...

Hi

Am 29.06.20 um 11:27 schrieb Daniel Vetter:

...

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 was never really happy with the naming either. Maybe call it aperture helpers with drm_aperture_ and CONFIG_DRM_APERTURE_HELPER?

+1

...

...

• 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?

I haven't seen any complains, but I'll double check.

Might be worth it to cc Greg on this, since the device driver model has a lot of corner cases to make sure it doesn't get stuck here with hiararchical drivers/device getting removed while something else is probing them at the same time.

Oh, OK. I'll keep that in mind when sending out v2 of these patches.

Best regards Thomas

-Daniel

...

Best regards Thomas

...

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

-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer