[PATCH v2 04/10] drm/aperture: Add infrastructure for aperture ownership
Thomas Zimmermann
tzimmermann at suse.de
Fri Apr 9 07:06:56 UTC 2021
Hi
Am 08.04.21 um 11:48 schrieb Daniel Vetter:
>
> Maybe just me, but to avoid overstretching the attention spawn of doc
> readers I'd avoid this example here. And maybe make the recommendation
> stronger, e.g. "PCI device drivers can avoid open-coding
> remove_conflicting_framebuffers() by calling
> drm_fb_helper_remove_conflicting_pci_framebuffers()."
It's a tutorial. In my expectation, everyone just copies the tutorial
code and fills the gaps.
>
>> + *
>> + * .. code-block:: c
>> + *
>> + * static int probe(struct pci_dev *pdev)
>> + * {
>> + * int ret;
>> + *
>> + * // Remove any generic drivers...
>> + * ret = drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, "example driver");
>> + * if (ret)
>> + * return ret;
>> + *
>> + * // ... and initialize the hardware.
>> + * ...
>> + *
>> + * drm_dev_register();
>> + *
>> + * return 0;
>> + * }
>> + *
>> + * Drivers that are susceptible to being removed be other drivers, such as
>> + * generic EFI or VESA drivers, have to register themselves as owners of their
>> + * given framebuffer memory. Ownership of the framebuffer memory is achived
>> + * by calling devm_aperture_acquire(). On success, the driver is the owner
>> + * of the framebuffer range. The function fails if the framebuffer is already
>> + * by another driver. See below for an example.
>> + *
>> + * .. code-block:: c
>> + *
>> + * static struct drm_aperture_funcs ap_funcs = {
>> + * .detach = ...
>
> Is there really value in allowing/forcing drivers to set up their own
> detach ops? You already make this specific to struct drm_device, an
> implementation that just calls drm_dev_unplug feels like the right thing
> to do?
Is it that easy? simepldrm's detach function has code to synchronize
with concurrent hotplug removals. If we can use drm_dev_unplug() for
everything, I'm all for it.
Best regards
Thomas
>
> Or maybe we should tie this more into the struct device mode and force an
> unload that way? That way devm cleanup would work as one expects, and
> avoid the need for anything specific (hopefully) in this detach callback.
>
> Just feels a bit like we're reinventing half of the driver model here,
> badly.
>
>> + * };
>> + *
>> + * static int acquire_framebuffers(struct drm_device *dev, struct pci_dev *pdev)
>> + * {
>> + * resource_size_t start, len;
>> + * struct drm_aperture *ap;
>> + *
>> + * base = pci_resource_start(pdev, 0);
>> + * size = pci_resource_len(pdev, 0);
>> + *
>> + * ap = devm_acquire_aperture(dev, base, size, &ap_funcs);
>> + * if (IS_ERR(ap))
>> + * return PTR_ERR(ap);
>> + *
>> + * return 0;
>> + * }
>> + *
>> + * static int probe(struct pci_dev *pdev)
>> + * {
>> + * struct drm_device *dev;
>> + * int ret;
>> + *
>> + * // ... Initialize the device...
>> + * dev = devm_drm_dev_alloc();
>> + * ...
>> + *
>> + * // ... and acquire ownership of the framebuffer.
>> + * ret = acquire_framebuffers(dev, pdev);
>> + * if (ret)
>> + * return ret;
>> + *
>> + * drm_dev_register();
>> + *
>> + * return 0;
>> + * }
>> + *
>> + * The generic driver is now subject to forced removal by other drivers. This
>> + * is when the detach function in struct &drm_aperture_funcs comes into play.
>> + * When a driver calls drm_fb_helper_remove_conflicting_framebuffers() et al
>> + * for the registered framebuffer range, the DRM core calls struct
>> + * &drm_aperture_funcs.detach and the generic driver has to onload itself. It
>> + * may not access the device's registers, framebuffer memory, ROM, etc after
>> + * detach returned. If the driver supports hotplugging, detach can be treated
>> + * like an unplug event.
>> + *
>> + * .. code-block:: c
>> + *
>> + * static void detach_from_device(struct drm_device *dev,
>> + * resource_size_t base,
>> + * resource_size_t size)
>> + * {
>> + * // Signal unplug
>> + * drm_dev_unplug(dev);
>> + *
>> + * // Maybe do other clean-up operations
>> + * ...
>> + * }
>> + *
>> + * static struct drm_aperture_funcs ap_funcs = {
>> + * .detach = detach_from_device,
>> + * };
>> + */
>> +
>> +/**
>> + * struct drm_aperture - Represents a DRM framebuffer aperture
>> + *
>> + * This structure has no public fields.
>> + */
>> +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;
>> +};
>> +
>> +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 void devm_aperture_acquire_release(void *data)
>> +{
>> + struct drm_aperture *ap = data;
>> + bool detached = !ap->dev;
>> +
>> + if (!detached)
>
> Uh this needs a comment that if ap->dev is NULL then we're called from
> drm_aperture_detach_drivers() and hence the lock is already held.
>
>> + mutex_lock(&drm_apertures_lock);
>
> and an
>
> else
> locdep_assert_held(&drm_apertures_lock);
>
> here to check that. I was scratching my head first quite a bit how you'd
> solve the deadlock, this is a neat solution (much simpler than anything I
> came up with in my head). But needs comments.
>
>> +
>> + list_del(&ap->lh);
>> +
>> + if (!detached)
>> + mutex_unlock(&drm_apertures_lock);
>> +}
>> +
>> +/**
>> + * devm_aperture_acquire - Acquires ownership of a framebuffer on behalf of a DRM driver.
>> + * @dev: the DRM device to own the framebuffer memory
>> + * @base: the framebuffer's byte offset in physical memory
>> + * @size: the framebuffer size in bytes
>> + * @funcs: callback functions
>> + *
>> + * Installs the given device as the new owner. The function fails if the
>> + * framebuffer range, or parts of it, is currently owned by another driver.
>> + * To evict current owners, callers should use
>> + * drm_fb_helper_remove_conflicting_framebuffers() et al. before calling this
>> + * function. Acquired apertures are released automatically if the underlying
>> + * device goes away.
>> + *
>> + * Returns:
>> + * An instance of struct &drm_aperture on success, or a pointer-encoded
>> + * errno value otherwise.
>> + */
>> +struct drm_aperture *
>> +devm_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;
>> + int ret;
>> +
>> + 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 = devm_kzalloc(dev->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);
>> +
>> + ret = devm_add_action_or_reset(dev->dev, devm_aperture_acquire_release, ap);
>> + if (ret)
>> + return ERR_PTR(ret);
>> +
>> + return ap;
>> +}
>> +EXPORT_SYMBOL(devm_aperture_acquire);
>> +
>> +void drm_aperture_detach_drivers(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);
>> + struct drm_device *dev = ap->dev;
>> +
>> + if (!overlap(base, end, ap->base, ap->base + ap->size))
>> + continue;
>> +
>> + ap->dev = NULL; /* detach from device */
>> + if (drm_WARN_ON(dev, !ap->funcs->detach))
>> + continue;
>> + ap->funcs->detach(dev, ap->base, ap->size);
>> + }
>> +
>> + mutex_unlock(&drm_apertures_lock);
>> +}
>> +EXPORT_SYMBOL(drm_aperture_detach_drivers);
>
> Is this just exported because of the inline functions in the headers? Imo
> better to make them proper functions (they're big after your patch¬
> perf critical, so not good candidates for inlining anyway).
>
>> diff --git a/include/drm/drm_aperture.h b/include/drm/drm_aperture.h
>> index 13766efe9517..696cec75ef78 100644
>> --- a/include/drm/drm_aperture.h
>> +++ b/include/drm/drm_aperture.h
>> @@ -4,8 +4,30 @@
>> #define _DRM_APERTURE_H_
>>
>> #include <linux/fb.h>
>> +#include <linux/pci.h>
>> #include <linux/vgaarb.h>
>>
>> +struct drm_aperture;
>> +struct drm_device;
>> +
>> +struct drm_aperture_funcs {
>> + void (*detach)(struct drm_device *dev, resource_size_t base, resource_size_t size);
>> +};
>> +
>> +struct drm_aperture *
>> +devm_aperture_acquire(struct drm_device *dev,
>> + resource_size_t base, resource_size_t size,
>> + const struct drm_aperture_funcs *funcs);
>> +
>> +#if defined(CONFIG_DRM_APERTURE)
>> +void drm_aperture_detach_drivers(resource_size_t base, resource_size_t size);
>> +#else
>> +static inline void
>> +drm_aperture_detach_drivers(resource_size_t base, resource_size_t size)
>> +{
>> +}
>> +#endif
>> +
>> /**
>> * drm_fb_helper_remove_conflicting_framebuffers - remove firmware-configured framebuffers
>> * @a: memory range, users of which are to be removed
>> @@ -20,6 +42,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_aperture_detach_drivers(a->ranges[i].base, a->ranges[i].size);
>> +
>> #if IS_REACHABLE(CONFIG_FB)
>> return remove_conflicting_framebuffers(a, name, primary);
>> #else
>> @@ -43,7 +70,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_aperture_detach_drivers(base, size);
>> + }
>>
>> /*
>> * WARNING: Apparently we must kick fbdev drivers before vgacon,
>> --
>> 2.30.1
>>
>
--
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20210409/09a879c0/attachment.sig>
More information about the dri-devel
mailing list