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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Sep 29, 2020 at 10:59:10AM +0200, Thomas Zimmermann 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@xxxxxxx>
> > 
> > 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.
> 
> Any specific reason for drm-kms?
> 
> The aperture helpers are used to manage ownership of memory and most
> drivers begin with drm_fb_helper_remove_conflicting_framebuffers(). It
> seems more approprite to put this into drm-internals as part of the
> driver initialization.

Simple because the only reason you might want to use this is for display.
In no other case do we load a firmware driver to make hw semi-useable.
Putting it where people might look for it and all that.
-Daniel

> 
> Best regards
> Thomas
> 
> > - 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
> >>
> > 
> 
> -- 
> 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
> 




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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux