On Fri, Apr 09, 2021 at 09:06:56AM +0200, Thomas Zimmermann wrote:
> 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.
Sure, but we also have default functions for most common cases, so most
people just end up copypasting the single function call. Feels like
overkill to have a tutorial for that.
Imo tutorial/pseudo-code are good if there's more involved code flow that
many places need to copypaste and customize. Or to show how different
functions work together collectively. This doesn't quite feel like it's
clearing that bar.
And please don't get me wrong, solid docs is great. It's just that I think
we need to have reader's attention span in mind too (and mine personally
might be on the extremely short side here) to make sure our docs are
effective at conveying information.
> > > + *
> > > + * .. 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.
Uh, I should have looked at the code instead of just asking silly
questions :-)
Now I'm even more scared, and also more convinced that we're recreating a
bad version of some of the core driver model concepts.
I think the ideal option here would be if drm_aperture could unload
(unbind really) the platform driver for us, through the driver model. Then
there's only one place that keeps track whether the driver is unbound or
not. I'm not sure whether this can be done fully generic on a struct
device, or whether we need special code for each type. Since atm we only
have simpledrm we can just specialize on platform_device and it's good
enough.
I think best here would be to Cc: gregkh on this patch and the simpledrm
->detach implementatation, and ask for his feedback as driver model
maintainer. Maybe if you could hack together the platform_device unbind
path as proof of concept would be even better.
Either way, this is really tricky.
-Daniel
>
> 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
>
--
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
