| Message ID | 20160826000056.12806-1-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 26, 2016 at 2:00 AM, David Herrmann <dh.herrmann@gmail.com> wrote: > Provide a generic DRM helper that evicts all conflicting firmware > framebuffers, devices, and drivers. The new helper is called > drm_evict_firmware(), and takes a flagset controlling which firmware to > kick out. > > This is meant to be used by drivers in their ->probe() callbacks of their > parent bus, before calling into drm_dev_register(). > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > --- > Hey > > This is just compile-tested for now. I just want to get some comments on the > design. I decided to drop the sysfb infrastructure and rather just use this > generic helper. It keeps things simple and should work just fine for all > reasonable use-cases. > > This will work with SimpleDRM out-of-the-box on x86. > > Architectures with dynamic simple-framebuffer devices are not supported yet. I > actually have no idea what the strategy there is? Can the DeviceTree people come > up with something? Am I supposed to call of_platform_depopulate()? Or > of_detach_node()? Or what? > Looking at drivers/of/{platform,dynamic}.c, I cannot see how this is supposed to > work at all. Who protects platform_device_del() from being called in parallel? > Also: Does any platform make use the of 'display:' entry in 'simple-framebuffer' > DT nodes? If yes, how do I get access to it? And why don't vc4/sun4i make use of > it, rather than falling back to remove_conflicting_framebuffers()? If we revamp this I'd like to make it a lot more automatic. Atm all drivers need to explicitly figure out what they need to kick (and as you noticed, many seem to get it wrong). But in most cases it's the platform code that would know this best. What about drm_evict_firmware_pci(struct pci_device *pci); which then: - does the remove_conflicting_framebuffer dance with all the pci bars - if the device class is VGA, also nuke vgacon (not just when it's the currently decoding VGA device, since that might change later on) Similar for other types of devices (i.e. drm_evict_firmware_platform for arm/of). Or maybe we can just take a struct device *dev and cast suitably within the helper to hide it all? This is already what correct drivers are doing, and we can still expose the internals if there's ever a case where things work differently. A few more comments below. > Thanks > David > > drivers/gpu/drm/drm_drv.c | 257 ++++++++++++++++++++++++++++++++++++++++++++++ > include/drm/drmP.h | 13 +++ > 2 files changed, 270 insertions(+) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 0773547..581c342 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -26,12 +26,16 @@ > * DEALINGS IN THE SOFTWARE. > */ > > +#include <linux/console.h> > #include <linux/debugfs.h> > +#include <linux/fb.h> > #include <linux/fs.h> > #include <linux/module.h> > #include <linux/moduleparam.h> > #include <linux/mount.h> > +#include <linux/of.h> > #include <linux/slab.h> > +#include <linux/vt.h> > #include <drm/drmP.h> > #include "drm_crtc_internal.h" > #include "drm_legacy.h" > @@ -881,6 +885,259 @@ void drm_dev_unregister(struct drm_device *dev) > } > EXPORT_SYMBOL(drm_dev_unregister); > > +struct drm_evict_ctx { > + struct apertures_struct *ap; > + unsigned int flags; > +}; > + > +static bool drm_evict_match_resource(struct drm_evict_ctx *ctx, > + struct resource *mem) > +{ > + struct aperture *g; > + unsigned int i; > + > + for (i = 0; i < ctx->ap->count; ++i) { > + g = &ctx->ap->ranges[i]; > + > + if (mem->start == g->base) > + return true; > + if (mem->start >= g->base && mem->end < g->base + g->size) > + return true; > + if ((ctx->flags & DRM_EVICT_VBE) && mem->start == 0xA0000) > + return true; > + } > + > + return false; > +} > + > +static int drm_evict_platform_device(struct device *dev, void *userdata) > +{ > + struct drm_evict_ctx *ctx = userdata; > + struct platform_device *pdev = to_platform_device(dev); > + struct resource *mem; > + > + /* > + * We are only interested in static devices here (i.e., they carry > + * information via a statically allocated platform data field). Any > + * dynamically allocated devices have separate ownership models and > + * must be handled via their respective management calls. > + */ > + if (!dev_get_platdata(&pdev->dev)) > + return 0; > + if (!pdev->name) > + return 0; > + > + if (!strcmp(pdev->name, "simple-framebuffer")) { > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!mem) > + return 0; > + if (!drm_evict_match_resource(ctx, mem)) > + return 0; > + > + platform_device_del(pdev); > + } > + > + return 0; > +} Shouldn't the device framework and iores allocator make sure that such conflicts are handled? Assuming of course that we use all the iores_mem resources/pci bars to kick out other framebuffers. Or when exactly can we end up with overlapping iores_mem which don't result in a device conflict that the core takes care of already? > + > +static int drm_evict_platform(struct drm_evict_ctx *ctx) > +{ > + /* > + * Early-boot architecture setup and boot-loarders sometimes create > + * preliminary platform devices with a generic framebuffer setup. This > + * allows graphics access during boot-up, without a real graphics > + * driver loaded. However, once a real graphics driver takes over, we > + * have to destroy those platform devices. In the legacy fbdev case, we > + * just used to unload the fbdev driver. However, to make sure any kind > + * of driver is unloaded, the platform-eviction code here simply > + * removes any conflicting platform device directly. This causes any > + * bound driver to be detached, and then removes the device entirely so > + * it cannot be bound to later on. > + * > + * Please note that any such platform device must be registered by > + * early architecture setup code. If they are registered after regular > + * DRM drivers, this will fail horribly. > + */ > + > + static DEFINE_MUTEX(lock); > + int ret; > + > + /* > + * In case of static platform-devices, we must iterate the bus and > + * remove them manually. We know that we're the only code that might > + * remove them, so a simple static lock serializes all calls here. We > + * must make sure our eviction callback leaves any non-static platform > + * devices untouched, though. > + * > + * XXX: ARM and other OF users should really make sure to add their own > + * eviction code here, possibly using the 'display' DT-handle as > + * defined by the 'simple-framebuffer' OF-node. > + */ > + mutex_lock(&lock); > + ret = bus_for_each_dev(&platform_bus_type, NULL, ctx, > + drm_evict_platform_device); > + mutex_unlock(&lock); > + return ret; > +} > + > +static int drm_evict_fbdev(struct drm_evict_ctx *ctx) > +{ > + /* > + * We have 2 different cases where fbdev drivers can operate on the > + * same devices as DRM drivers: > + * > + * o Two drivers exist for the same device, one DRM driver and one > + * fbdev driver (e.g., nvidiafb and nouveau). In those cases, both > + * drivers bind to the same real hw-device and driver core takes > + * care of any conflicts. > + * > + * o A generic fbdev driver uses some pseudo-device to provide > + * early-boot graphics access or some other kind of generic display > + * driver. At the same time, a DRM driver exists that can run the > + * real hardware (e.g., vesafb/efifb and i915). > + * > + * If possible, driver core takes care of any conflicts and prevents > + * two drivers from being bound to the same device. However, in some > + * cases this is not possible. We then use > + * remove_conflicting_framebuffers() to match the apertures of the DRM > + * device against all existing fbdev devices, evicting any conflicting > + * fbdev drivers. Additionally, if requested, any VBE driver is evicted > + * as well. > + * > + * Note that this breaks horribly if an fbdev driver is probed after a > + * DRM driver. So make sure whenever such conflicts are possible, the > + * fbdev drivers must be probed in early boot. If you cannot guarantee > + * that, you better make sure that the driver core prevents both > + * drivers from being probed in parallel. > + */ > + > +#if !defined(CONFIG_FB) > + return 0; > +#else > + return remove_conflicting_framebuffers(ctx->ap, "drm", > + !!(ctx->flags & DRM_EVICT_VBE)); > +#endif > +} > + > +static int drm_evict_vgacon(void) > +{ > + /* > + * The VGACON console driver pokes at VGA registers randomly. If a DRM > + * driver cannot keep the VGA support alive, it better makes sure to > + * unload VGACON before probing. > + * > + * Unloading VGACON requires us to first force dummycon to take over > + * from vgacon (but only if vgacon is really in use), followed by a > + * deregistration of vgacon. Note that this prevents vgacon from being > + * used again after the DRM driver is unloaded. But that is usually > + * fine, since VGA state is rarely restored on driver-unload, anyway. > + * > + * Note that we rely on VGACON to be probed in early boot (actually > + * done by ARCH setup code). If it is probed after DRM drivers, this > + * will fail horribly. You better make sure VGACON is probed early and > + * DRM drivers are probed as normal modules. > + */ > + > +#if !defined(CONFIG_VGA_CONSOLE) > + return 0; > +#elif !defined(CONFIG_DUMMY_CONSOLE) > +# warning "Dummy console is disabled, but required to kick out VGACON" > + return -ENODEV; We should add a select DUMMY_CONSOLE if VT or similar to our Kconfig to avoid this ever happening. > +#else > + int ret = 0; > + > + DRM_INFO("Replacing VGA console driver\n"); > + > + console_lock(); > + if (con_is_bound(&vga_con)) > + ret = do_take_over_console(&dummy_con, 0, > + MAX_NR_CONSOLES - 1, 1); > + if (ret == 0) { > + ret = do_unregister_con_driver(&vga_con); > + if (ret == -ENODEV) /* ignore "already unregistered" */ > + ret = 0; > + } > + console_unlock(); > + > + return ret; > +#endif > +} > + > +/** > + * drm_evict_firmware - remove any conflicting firmware-framebuffers > + * @ap: apertures claimed by driver, or NULL > + * @flags: eviction flags > + * > + * This function evicts any conflicting firmware-framebuffers and their bound > + * drivers, according to the data given as @ap and @flags. > + * > + * Depending on @flags, the following operations are performed: > + * > + * DRM_EVICT_FBDEV: Any fbdev drivers that overlap @ap are unloaded. > + * Furthermore, if DRM_EVICT_VBE is given as well, any fbdev > + * driver that maps the VGA region is unloaded. > + * > + * DRM_EVICT_PLATFORM: Firmware framebuffer platform devices (eg., > + * 'simple-framebuffer') that overlap @ap are removed > + * from the system, causing drivers to be unbound. > + * If DRM_EVICT_VBE is given, this also affects devices > + * that map the VGA region. > + * > + * DRM_EVICT_VGACON: The vgacon console driver is unbound and unregistered. > + * > + * The caller must provide their apertures as @ap. If it is NULL, an empty set > + * is assumed, except if DRM_EVICT_ANYWHERE is given, in which case a fake > + * aperture across the entire address range is used. > + * > + * RETURNS: > + * 0 on success, negative error code on failure. > + */ > +int drm_evict_firmware(struct apertures_struct *ap, unsigned int flags) > +{ > + struct drm_evict_ctx ctx; > + struct apertures_struct *allocated_ap = NULL; > + int ret; > + > + WARN_ON(ap && (flags & DRM_EVICT_ANYWHERE)); > + > + if (!ap) { > + allocated_ap = alloc_apertures(!!(flags & DRM_EVICT_ANYWHERE)); > + if (!allocated_ap) > + return -ENOMEM; > + > + ap = allocated_ap; > + > + if (flags & DRM_EVICT_ANYWHERE) { > + ap->ranges[0].base = 0; > + ap->ranges[0].size = ~0; > + } > + } > + > + ctx.ap = ap; > + ctx.flags = flags; > + > + if (flags & DRM_EVICT_PLATFORM) { > + ret = drm_evict_platform(&ctx); > + if (ret < 0) > + return ret; > + } > + > + if (flags & DRM_EVICT_FBDEV) { > + ret = drm_evict_fbdev(&ctx); > + if (ret < 0) > + return ret; > + } > + > + if (flags & DRM_EVICT_VGACON) { > + ret = drm_evict_vgacon(); > + if (ret < 0) > + return ret; > + } > + > + kfree(allocated_ap); > + return 0; > +} > + > /* > * DRM Core > * The DRM core module initializes all global DRM objects and makes them > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 2197ab1..6c8bcf5 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -88,6 +88,7 @@ struct drm_gem_object; > struct drm_master; > struct drm_vblank_crtc; > > +struct apertures_struct; > struct device_node; > struct videomode; > struct reservation_object; > @@ -971,6 +972,18 @@ void drm_dev_unref(struct drm_device *dev); > int drm_dev_register(struct drm_device *dev, unsigned long flags); > void drm_dev_unregister(struct drm_device *dev); > > +enum { > + DRM_EVICT_FBDEV = (1U << 0), > + DRM_EVICT_PLATFORM = (1U << 1), > + DRM_EVICT_VGACON = (1U << 2), > + DRM_EVICT_VBE = (1U << 3), > + DRM_EVICT_ANYWHERE = (1U << 4), > + > + DRM_EVICT_DEFAULT = DRM_EVICT_FBDEV, > +}; > + > +int drm_evict_firmware(struct apertures_struct *ap, unsigned int flags); > + > struct drm_minor *drm_minor_acquire(unsigned int minor_id); > void drm_minor_release(struct drm_minor *minor); > > -- > 2.9.3 >
Hi, On 26-08-16 02:00, David Herrmann wrote: > Provide a generic DRM helper that evicts all conflicting firmware > framebuffers, devices, and drivers. The new helper is called > drm_evict_firmware(), and takes a flagset controlling which firmware to > kick out. > > This is meant to be used by drivers in their ->probe() callbacks of their > parent bus, before calling into drm_dev_register(). > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > --- > Hey > > This is just compile-tested for now. I just want to get some comments on the > design. I decided to drop the sysfb infrastructure and rather just use this > generic helper. It keeps things simple and should work just fine for all > reasonable use-cases. > > This will work with SimpleDRM out-of-the-box on x86. > > Architectures with dynamic simple-framebuffer devices are not supported yet. I > actually have no idea what the strategy there is? Can the DeviceTree people come > up with something? Am I supposed to call of_platform_depopulate()? Or > of_detach_node()? Or what? I'm not sure we would want to remove the device at all, we certainly should not be removing the dt_node from the devicetree IMHO. Having that around to see how the bootloader set things up is really useful for debugging and normally we should never modify the devicetree as set up by the bootloader. Why not just unbind the driver from the platform device? That should be enough. Regards, Hans > Looking at drivers/of/{platform,dynamic}.c, I cannot see how this is supposed to > work at all. Who protects platform_device_del() from being called in parallel? > Also: Does any platform make use the of 'display:' entry in 'simple-framebuffer' > DT nodes? If yes, how do I get access to it? And why don't vc4/sun4i make use of > it, rather than falling back to remove_conflicting_framebuffers()? > > Thanks > David > > drivers/gpu/drm/drm_drv.c | 257 ++++++++++++++++++++++++++++++++++++++++++++++ > include/drm/drmP.h | 13 +++ > 2 files changed, 270 insertions(+) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 0773547..581c342 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -26,12 +26,16 @@ > * DEALINGS IN THE SOFTWARE. > */ > > +#include <linux/console.h> > #include <linux/debugfs.h> > +#include <linux/fb.h> > #include <linux/fs.h> > #include <linux/module.h> > #include <linux/moduleparam.h> > #include <linux/mount.h> > +#include <linux/of.h> > #include <linux/slab.h> > +#include <linux/vt.h> > #include <drm/drmP.h> > #include "drm_crtc_internal.h" > #include "drm_legacy.h" > @@ -881,6 +885,259 @@ void drm_dev_unregister(struct drm_device *dev) > } > EXPORT_SYMBOL(drm_dev_unregister); > > +struct drm_evict_ctx { > + struct apertures_struct *ap; > + unsigned int flags; > +}; > + > +static bool drm_evict_match_resource(struct drm_evict_ctx *ctx, > + struct resource *mem) > +{ > + struct aperture *g; > + unsigned int i; > + > + for (i = 0; i < ctx->ap->count; ++i) { > + g = &ctx->ap->ranges[i]; > + > + if (mem->start == g->base) > + return true; > + if (mem->start >= g->base && mem->end < g->base + g->size) > + return true; > + if ((ctx->flags & DRM_EVICT_VBE) && mem->start == 0xA0000) > + return true; > + } > + > + return false; > +} > + > +static int drm_evict_platform_device(struct device *dev, void *userdata) > +{ > + struct drm_evict_ctx *ctx = userdata; > + struct platform_device *pdev = to_platform_device(dev); > + struct resource *mem; > + > + /* > + * We are only interested in static devices here (i.e., they carry > + * information via a statically allocated platform data field). Any > + * dynamically allocated devices have separate ownership models and > + * must be handled via their respective management calls. > + */ > + if (!dev_get_platdata(&pdev->dev)) > + return 0; > + if (!pdev->name) > + return 0; > + > + if (!strcmp(pdev->name, "simple-framebuffer")) { > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!mem) > + return 0; > + if (!drm_evict_match_resource(ctx, mem)) > + return 0; > + > + platform_device_del(pdev); > + } > + > + return 0; > +} > + > +static int drm_evict_platform(struct drm_evict_ctx *ctx) > +{ > + /* > + * Early-boot architecture setup and boot-loarders sometimes create > + * preliminary platform devices with a generic framebuffer setup. This > + * allows graphics access during boot-up, without a real graphics > + * driver loaded. However, once a real graphics driver takes over, we > + * have to destroy those platform devices. In the legacy fbdev case, we > + * just used to unload the fbdev driver. However, to make sure any kind > + * of driver is unloaded, the platform-eviction code here simply > + * removes any conflicting platform device directly. This causes any > + * bound driver to be detached, and then removes the device entirely so > + * it cannot be bound to later on. > + * > + * Please note that any such platform device must be registered by > + * early architecture setup code. If they are registered after regular > + * DRM drivers, this will fail horribly. > + */ > + > + static DEFINE_MUTEX(lock); > + int ret; > + > + /* > + * In case of static platform-devices, we must iterate the bus and > + * remove them manually. We know that we're the only code that might > + * remove them, so a simple static lock serializes all calls here. We > + * must make sure our eviction callback leaves any non-static platform > + * devices untouched, though. > + * > + * XXX: ARM and other OF users should really make sure to add their own > + * eviction code here, possibly using the 'display' DT-handle as > + * defined by the 'simple-framebuffer' OF-node. > + */ > + mutex_lock(&lock); > + ret = bus_for_each_dev(&platform_bus_type, NULL, ctx, > + drm_evict_platform_device); > + mutex_unlock(&lock); > + return ret; > +} > + > +static int drm_evict_fbdev(struct drm_evict_ctx *ctx) > +{ > + /* > + * We have 2 different cases where fbdev drivers can operate on the > + * same devices as DRM drivers: > + * > + * o Two drivers exist for the same device, one DRM driver and one > + * fbdev driver (e.g., nvidiafb and nouveau). In those cases, both > + * drivers bind to the same real hw-device and driver core takes > + * care of any conflicts. > + * > + * o A generic fbdev driver uses some pseudo-device to provide > + * early-boot graphics access or some other kind of generic display > + * driver. At the same time, a DRM driver exists that can run the > + * real hardware (e.g., vesafb/efifb and i915). > + * > + * If possible, driver core takes care of any conflicts and prevents > + * two drivers from being bound to the same device. However, in some > + * cases this is not possible. We then use > + * remove_conflicting_framebuffers() to match the apertures of the DRM > + * device against all existing fbdev devices, evicting any conflicting > + * fbdev drivers. Additionally, if requested, any VBE driver is evicted > + * as well. > + * > + * Note that this breaks horribly if an fbdev driver is probed after a > + * DRM driver. So make sure whenever such conflicts are possible, the > + * fbdev drivers must be probed in early boot. If you cannot guarantee > + * that, you better make sure that the driver core prevents both > + * drivers from being probed in parallel. > + */ > + > +#if !defined(CONFIG_FB) > + return 0; > +#else > + return remove_conflicting_framebuffers(ctx->ap, "drm", > + !!(ctx->flags & DRM_EVICT_VBE)); > +#endif > +} > + > +static int drm_evict_vgacon(void) > +{ > + /* > + * The VGACON console driver pokes at VGA registers randomly. If a DRM > + * driver cannot keep the VGA support alive, it better makes sure to > + * unload VGACON before probing. > + * > + * Unloading VGACON requires us to first force dummycon to take over > + * from vgacon (but only if vgacon is really in use), followed by a > + * deregistration of vgacon. Note that this prevents vgacon from being > + * used again after the DRM driver is unloaded. But that is usually > + * fine, since VGA state is rarely restored on driver-unload, anyway. > + * > + * Note that we rely on VGACON to be probed in early boot (actually > + * done by ARCH setup code). If it is probed after DRM drivers, this > + * will fail horribly. You better make sure VGACON is probed early and > + * DRM drivers are probed as normal modules. > + */ > + > +#if !defined(CONFIG_VGA_CONSOLE) > + return 0; > +#elif !defined(CONFIG_DUMMY_CONSOLE) > +# warning "Dummy console is disabled, but required to kick out VGACON" > + return -ENODEV; > +#else > + int ret = 0; > + > + DRM_INFO("Replacing VGA console driver\n"); > + > + console_lock(); > + if (con_is_bound(&vga_con)) > + ret = do_take_over_console(&dummy_con, 0, > + MAX_NR_CONSOLES - 1, 1); > + if (ret == 0) { > + ret = do_unregister_con_driver(&vga_con); > + if (ret == -ENODEV) /* ignore "already unregistered" */ > + ret = 0; > + } > + console_unlock(); > + > + return ret; > +#endif > +} > + > +/** > + * drm_evict_firmware - remove any conflicting firmware-framebuffers > + * @ap: apertures claimed by driver, or NULL > + * @flags: eviction flags > + * > + * This function evicts any conflicting firmware-framebuffers and their bound > + * drivers, according to the data given as @ap and @flags. > + * > + * Depending on @flags, the following operations are performed: > + * > + * DRM_EVICT_FBDEV: Any fbdev drivers that overlap @ap are unloaded. > + * Furthermore, if DRM_EVICT_VBE is given as well, any fbdev > + * driver that maps the VGA region is unloaded. > + * > + * DRM_EVICT_PLATFORM: Firmware framebuffer platform devices (eg., > + * 'simple-framebuffer') that overlap @ap are removed > + * from the system, causing drivers to be unbound. > + * If DRM_EVICT_VBE is given, this also affects devices > + * that map the VGA region. > + * > + * DRM_EVICT_VGACON: The vgacon console driver is unbound and unregistered. > + * > + * The caller must provide their apertures as @ap. If it is NULL, an empty set > + * is assumed, except if DRM_EVICT_ANYWHERE is given, in which case a fake > + * aperture across the entire address range is used. > + * > + * RETURNS: > + * 0 on success, negative error code on failure. > + */ > +int drm_evict_firmware(struct apertures_struct *ap, unsigned int flags) > +{ > + struct drm_evict_ctx ctx; > + struct apertures_struct *allocated_ap = NULL; > + int ret; > + > + WARN_ON(ap && (flags & DRM_EVICT_ANYWHERE)); > + > + if (!ap) { > + allocated_ap = alloc_apertures(!!(flags & DRM_EVICT_ANYWHERE)); > + if (!allocated_ap) > + return -ENOMEM; > + > + ap = allocated_ap; > + > + if (flags & DRM_EVICT_ANYWHERE) { > + ap->ranges[0].base = 0; > + ap->ranges[0].size = ~0; > + } > + } > + > + ctx.ap = ap; > + ctx.flags = flags; > + > + if (flags & DRM_EVICT_PLATFORM) { > + ret = drm_evict_platform(&ctx); > + if (ret < 0) > + return ret; > + } > + > + if (flags & DRM_EVICT_FBDEV) { > + ret = drm_evict_fbdev(&ctx); > + if (ret < 0) > + return ret; > + } > + > + if (flags & DRM_EVICT_VGACON) { > + ret = drm_evict_vgacon(); > + if (ret < 0) > + return ret; > + } > + > + kfree(allocated_ap); > + return 0; > +} > + > /* > * DRM Core > * The DRM core module initializes all global DRM objects and makes them > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 2197ab1..6c8bcf5 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -88,6 +88,7 @@ struct drm_gem_object; > struct drm_master; > struct drm_vblank_crtc; > > +struct apertures_struct; > struct device_node; > struct videomode; > struct reservation_object; > @@ -971,6 +972,18 @@ void drm_dev_unref(struct drm_device *dev); > int drm_dev_register(struct drm_device *dev, unsigned long flags); > void drm_dev_unregister(struct drm_device *dev); > > +enum { > + DRM_EVICT_FBDEV = (1U << 0), > + DRM_EVICT_PLATFORM = (1U << 1), > + DRM_EVICT_VGACON = (1U << 2), > + DRM_EVICT_VBE = (1U << 3), > + DRM_EVICT_ANYWHERE = (1U << 4), > + > + DRM_EVICT_DEFAULT = DRM_EVICT_FBDEV, > +}; > + > +int drm_evict_firmware(struct apertures_struct *ap, unsigned int flags); > + > struct drm_minor *drm_minor_acquire(unsigned int minor_id); > void drm_minor_release(struct drm_minor *minor); > >
Hi On Fri, Aug 26, 2016 at 9:57 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 26-08-16 02:00, David Herrmann wrote: >> >> Provide a generic DRM helper that evicts all conflicting firmware >> framebuffers, devices, and drivers. The new helper is called >> drm_evict_firmware(), and takes a flagset controlling which firmware to >> kick out. >> >> This is meant to be used by drivers in their ->probe() callbacks of their >> parent bus, before calling into drm_dev_register(). >> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> >> --- >> Hey >> >> This is just compile-tested for now. I just want to get some comments on >> the >> design. I decided to drop the sysfb infrastructure and rather just use >> this >> generic helper. It keeps things simple and should work just fine for all >> reasonable use-cases. >> >> This will work with SimpleDRM out-of-the-box on x86. >> >> Architectures with dynamic simple-framebuffer devices are not supported >> yet. I >> actually have no idea what the strategy there is? Can the DeviceTree >> people come >> up with something? Am I supposed to call of_platform_depopulate()? Or >> of_detach_node()? Or what? > > > I'm not sure we would want to remove the device at all, we certainly should > not > be removing the dt_node from the devicetree IMHO. Having that around to see > how > the bootloader set things up is really useful for debugging and normally we > should never modify the devicetree as set up by the bootloader. > > Why not just unbind the driver from the platform device? That should be > enough. That will leave IORESOURCE_MEM around, causing conflicts if re-used/claimed by other devices/drivers. Furthermore, it is really fragile leaving the device around, without any control over possible future driver probing. Thanks David
Hi, On 26-08-16 10:01, David Herrmann wrote: > Hi > > On Fri, Aug 26, 2016 at 9:57 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 26-08-16 02:00, David Herrmann wrote: >>> >>> Provide a generic DRM helper that evicts all conflicting firmware >>> framebuffers, devices, and drivers. The new helper is called >>> drm_evict_firmware(), and takes a flagset controlling which firmware to >>> kick out. >>> >>> This is meant to be used by drivers in their ->probe() callbacks of their >>> parent bus, before calling into drm_dev_register(). >>> >>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> >>> --- >>> Hey >>> >>> This is just compile-tested for now. I just want to get some comments on >>> the >>> design. I decided to drop the sysfb infrastructure and rather just use >>> this >>> generic helper. It keeps things simple and should work just fine for all >>> reasonable use-cases. >>> >>> This will work with SimpleDRM out-of-the-box on x86. >>> >>> Architectures with dynamic simple-framebuffer devices are not supported >>> yet. I >>> actually have no idea what the strategy there is? Can the DeviceTree >>> people come >>> up with something? Am I supposed to call of_platform_depopulate()? Or >>> of_detach_node()? Or what? >> >> >> I'm not sure we would want to remove the device at all, we certainly should >> not >> be removing the dt_node from the devicetree IMHO. Having that around to see >> how >> the bootloader set things up is really useful for debugging and normally we >> should never modify the devicetree as set up by the bootloader. >> >> Why not just unbind the driver from the platform device? That should be >> enough. > > That will leave IORESOURCE_MEM around, causing conflicts if > re-used/claimed by other devices/drivers. Furthermore, it is really > fragile leaving the device around, without any control over possible > future driver probing. Ah, good point. On ARM this currently typically is reserved by the bootloader so never touched by the kernel at all, not even when the simplefb is no longer used, actually returning this memory to the kernel after unbinding the simplefb / destroying the simplefb platform-dev would be really good to do. We should probably figure out how that should be done before getting rid of remove_conflicting_framebuffers... (sorry). Regards, Hans
Hi, On Fri, Aug 26, 2016 at 10:43:55AM +0200, Hans de Goede wrote: > >>I'm not sure we would want to remove the device at all, we > >>certainly should not be removing the dt_node from the devicetree > >>IMHO. Having that around to see how the bootloader set things up > >>is really useful for debugging and normally we should never modify > >>the devicetree as set up by the bootloader. > >> > >>Why not just unbind the driver from the platform device? That > >>should be enough. > > > >That will leave IORESOURCE_MEM around, causing conflicts if > >re-used/claimed by other devices/drivers. Furthermore, it is really > >fragile leaving the device around, without any control over > >possible future driver probing. > > Ah, good point. On ARM this currently typically is reserved by the bootloader > so never touched by the kernel at all, not even when the simplefb is no longer > used, actually returning this memory to the kernel after unbinding the simplefb / > destroying the simplefb platform-dev would be really good to do. We should > probably figure out how that should be done before getting rid of > remove_conflicting_framebuffers... (sorry). That would be rather easy to do. The firmware could generate a reserved-memory node instead of passing a smaller memory size to the kernel. That way, the kernel will know that it's actual ram that it can reclaim. Maxime
Hi, On 26-08-16 10:58, Maxime Ripard wrote: > Hi, > > On Fri, Aug 26, 2016 at 10:43:55AM +0200, Hans de Goede wrote: >>>> I'm not sure we would want to remove the device at all, we >>>> certainly should not be removing the dt_node from the devicetree >>>> IMHO. Having that around to see how the bootloader set things up >>>> is really useful for debugging and normally we should never modify >>>> the devicetree as set up by the bootloader. >>>> >>>> Why not just unbind the driver from the platform device? That >>>> should be enough. >>> >>> That will leave IORESOURCE_MEM around, causing conflicts if >>> re-used/claimed by other devices/drivers. Furthermore, it is really >>> fragile leaving the device around, without any control over >>> possible future driver probing. >> >> Ah, good point. On ARM this currently typically is reserved by the bootloader >> so never touched by the kernel at all, not even when the simplefb is no longer >> used, actually returning this memory to the kernel after unbinding the simplefb / >> destroying the simplefb platform-dev would be really good to do. We should >> probably figure out how that should be done before getting rid of >> remove_conflicting_framebuffers... (sorry). > > That would be rather easy to do. The firmware could generate a > reserved-memory node instead of passing a smaller memory size to the > kernel. That way, the kernel will know that it's actual ram that it > can reclaim. So when would the kernel reclaim the RAM then? We do not want it to reclaim before loading the simplefb / simpledrm driver. And the real drm driver will likely be a module, so if the kernel reclaims just before executing /sbin/init that would be too early, unless we can somehow manually make it do another reclaim pass when the simplefb gets disabled ... Regards, Hans
On Fri, 26 Aug 2016, David Herrmann <dh.herrmann@gmail.com> wrote: > Provide a generic DRM helper that evicts all conflicting firmware > framebuffers, devices, and drivers. The new helper is called > drm_evict_firmware(), and takes a flagset controlling which firmware to > kick out. Reading the subject, I thought this was somehow about the firmware themselves, not about their framebuffers... My bikeshed is to make that clear in the function name too. BR, Jani.
On Thu, Aug 25, 2016 at 7:00 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > Provide a generic DRM helper that evicts all conflicting firmware > framebuffers, devices, and drivers. The new helper is called > drm_evict_firmware(), and takes a flagset controlling which firmware to > kick out. > > This is meant to be used by drivers in their ->probe() callbacks of their > parent bus, before calling into drm_dev_register(). > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > --- > Hey > > This is just compile-tested for now. I just want to get some comments on the > design. I decided to drop the sysfb infrastructure and rather just use this > generic helper. It keeps things simple and should work just fine for all > reasonable use-cases. > > This will work with SimpleDRM out-of-the-box on x86. > > Architectures with dynamic simple-framebuffer devices are not supported yet. I > actually have no idea what the strategy there is? Can the DeviceTree people come > up with something? Am I supposed to call of_platform_depopulate()? If of_platform_populate was used to create the device, then yes call of_platform_depopulate. In this case, I think it wasn't. If of_platform_device_create was used, then platform_device_del. > Or > of_detach_node()? Or what? No. Only the struct device and its resources should need to be destroyed. The node should remain. > Looking at drivers/of/{platform,dynamic}.c, I cannot see how this is supposed to > work at all. Who protects platform_device_del() from being called in parallel? Not sure. In parallel to what? On most systems, nodes never go away and on those that do it is only a few things that get hotplugged. That's changing with DT overlays now, so there certainly could be some issues. > Also: Does any platform make use the of 'display:' entry in 'simple-framebuffer' > DT nodes? If yes, how do I get access to it? And why don't vc4/sun4i make use of > it, rather than falling back to remove_conflicting_framebuffers()? No idea. Rob
On Fri, Aug 26, 2016 at 11:02:17AM +0200, Hans de Goede wrote: > Hi, > > On 26-08-16 10:58, Maxime Ripard wrote: > >Hi, > > > >On Fri, Aug 26, 2016 at 10:43:55AM +0200, Hans de Goede wrote: > >>>>I'm not sure we would want to remove the device at all, we > >>>>certainly should not be removing the dt_node from the devicetree > >>>>IMHO. Having that around to see how the bootloader set things up > >>>>is really useful for debugging and normally we should never modify > >>>>the devicetree as set up by the bootloader. > >>>> > >>>>Why not just unbind the driver from the platform device? That > >>>>should be enough. > >>> > >>>That will leave IORESOURCE_MEM around, causing conflicts if > >>>re-used/claimed by other devices/drivers. Furthermore, it is really > >>>fragile leaving the device around, without any control over > >>>possible future driver probing. > >> > >>Ah, good point. On ARM this currently typically is reserved by the bootloader > >>so never touched by the kernel at all, not even when the simplefb is no longer > >>used, actually returning this memory to the kernel after unbinding the simplefb / > >>destroying the simplefb platform-dev would be really good to do. We should > >>probably figure out how that should be done before getting rid of > >>remove_conflicting_framebuffers... (sorry). > > > >That would be rather easy to do. The firmware could generate a > >reserved-memory node instead of passing a smaller memory size to the > >kernel. That way, the kernel will know that it's actual ram that it > >can reclaim. > > So when would the kernel reclaim the RAM then? When we kickout the framebuffer driver? Maxime
Hi, On 26-08-16 14:52, Maxime Ripard wrote: > On Fri, Aug 26, 2016 at 11:02:17AM +0200, Hans de Goede wrote: >> Hi, >> >> On 26-08-16 10:58, Maxime Ripard wrote: >>> Hi, >>> >>> On Fri, Aug 26, 2016 at 10:43:55AM +0200, Hans de Goede wrote: >>>>>> I'm not sure we would want to remove the device at all, we >>>>>> certainly should not be removing the dt_node from the devicetree >>>>>> IMHO. Having that around to see how the bootloader set things up >>>>>> is really useful for debugging and normally we should never modify >>>>>> the devicetree as set up by the bootloader. >>>>>> >>>>>> Why not just unbind the driver from the platform device? That >>>>>> should be enough. >>>>> >>>>> That will leave IORESOURCE_MEM around, causing conflicts if >>>>> re-used/claimed by other devices/drivers. Furthermore, it is really >>>>> fragile leaving the device around, without any control over >>>>> possible future driver probing. >>>> >>>> Ah, good point. On ARM this currently typically is reserved by the bootloader >>>> so never touched by the kernel at all, not even when the simplefb is no longer >>>> used, actually returning this memory to the kernel after unbinding the simplefb / >>>> destroying the simplefb platform-dev would be really good to do. We should >>>> probably figure out how that should be done before getting rid of >>>> remove_conflicting_framebuffers... (sorry). >>> >>> That would be rather easy to do. The firmware could generate a >>> reserved-memory node instead of passing a smaller memory size to the >>> kernel. That way, the kernel will know that it's actual ram that it >>> can reclaim. >> >> So when would the kernel reclaim the RAM then? > > When we kickout the framebuffer driver? Yes that is when we _want_ it to reclaim the RAM, my question was when it will _actually_ happen ? I'm not familiar with the reserved-memory implementation. Does your answer mean that some driver must make an explicit call to get the memory reclaimed ? Regards, Hans
Hi, On Fri, Aug 26, 2016 at 02:00:56AM +0200, David Herrmann wrote: > Also: Does any platform make use the of 'display:' entry in 'simple-framebuffer' > DT nodes? If yes, how do I get access to it? And why don't vc4/sun4i make use of > it, rather than falling back to remove_conflicting_framebuffers()? For the sun4i part, I'd say because I wasn't aware of it :) A better argument would be that most of our device tree don't have it at the moment, and we might boot on older device trees. So we need a fallback solution anyway. Maxime
On Fri, Aug 26, 2016 at 02:58:51PM +0200, Hans de Goede wrote: > Hi, > > On 26-08-16 14:52, Maxime Ripard wrote: > >On Fri, Aug 26, 2016 at 11:02:17AM +0200, Hans de Goede wrote: > >>Hi, > >> > >>On 26-08-16 10:58, Maxime Ripard wrote: > >>>Hi, > >>> > >>>On Fri, Aug 26, 2016 at 10:43:55AM +0200, Hans de Goede wrote: > >>>>>>I'm not sure we would want to remove the device at all, we > >>>>>>certainly should not be removing the dt_node from the devicetree > >>>>>>IMHO. Having that around to see how the bootloader set things up > >>>>>>is really useful for debugging and normally we should never modify > >>>>>>the devicetree as set up by the bootloader. > >>>>>> > >>>>>>Why not just unbind the driver from the platform device? That > >>>>>>should be enough. > >>>>> > >>>>>That will leave IORESOURCE_MEM around, causing conflicts if > >>>>>re-used/claimed by other devices/drivers. Furthermore, it is really > >>>>>fragile leaving the device around, without any control over > >>>>>possible future driver probing. > >>>> > >>>>Ah, good point. On ARM this currently typically is reserved by the bootloader > >>>>so never touched by the kernel at all, not even when the simplefb is no longer > >>>>used, actually returning this memory to the kernel after unbinding the simplefb / > >>>>destroying the simplefb platform-dev would be really good to do. We should > >>>>probably figure out how that should be done before getting rid of > >>>>remove_conflicting_framebuffers... (sorry). > >>> > >>>That would be rather easy to do. The firmware could generate a > >>>reserved-memory node instead of passing a smaller memory size to the > >>>kernel. That way, the kernel will know that it's actual ram that it > >>>can reclaim. > >> > >>So when would the kernel reclaim the RAM then? > > > >When we kickout the framebuffer driver? > > Yes that is when we _want_ it to reclaim the RAM, my question was when > it will _actually_ happen ? I'm not familiar with the reserved-memory > implementation. Does your answer mean that some driver must make an > explicit call to get the memory reclaimed ? The reserved-memory implementation is relying on memblock. I don't think there is a function yet to remove a reserved memory region, but its implementation would use memblock_free I guess. Maxime
Hi Rob On Fri, Aug 26, 2016 at 2:36 PM, Rob Herring <robh+dt@kernel.org> wrote: > On Thu, Aug 25, 2016 at 7:00 PM, David Herrmann <dh.herrmann@gmail.com> wrote: >> Provide a generic DRM helper that evicts all conflicting firmware >> framebuffers, devices, and drivers. The new helper is called >> drm_evict_firmware(), and takes a flagset controlling which firmware to >> kick out. >> >> This is meant to be used by drivers in their ->probe() callbacks of their >> parent bus, before calling into drm_dev_register(). >> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> >> --- >> Hey >> >> This is just compile-tested for now. I just want to get some comments on the >> design. I decided to drop the sysfb infrastructure and rather just use this >> generic helper. It keeps things simple and should work just fine for all >> reasonable use-cases. >> >> This will work with SimpleDRM out-of-the-box on x86. >> >> Architectures with dynamic simple-framebuffer devices are not supported yet. I >> actually have no idea what the strategy there is? Can the DeviceTree people come >> up with something? Am I supposed to call of_platform_depopulate()? > > If of_platform_populate was used to create the device, then yes call > of_platform_depopulate. In this case, I think it wasn't. If > of_platform_device_create was used, then platform_device_del. > >> Or >> of_detach_node()? Or what? > > No. Only the struct device and its resources should need to be > destroyed. The node should remain. > >> Looking at drivers/of/{platform,dynamic}.c, I cannot see how this is supposed to >> work at all. Who protects platform_device_del() from being called in parallel? > > Not sure. In parallel to what? On most systems, nodes never go away > and on those that do it is only a few things that get hotplugged. > That's changing with DT overlays now, so there certainly could be some > issues. Two tasks calling platform_device_del() in parallel on the same device will not work. Meaning, calling of_platform_device_destroy() in parallel does not work either. Same for of_platform_depopulate(). Same for of_node_detach(). Sure, all those functions are not meant to be called in parallel by multiple tasks. They are rather meant to have a single holder which preferably is the one instantiating and destroying the node/device/foobar. But the framebuffer eviction code somehow needs to trigger the removal, and thus needs some hook, that can be called in parallel by any driver that is loaded. I can make sure the simple-framebuffer eviction code is locked properly. That'll work, for now. But if someone ends up providing simple-framebuffer devices via overlays or any other dynamic mechanism, they will race the removal. And it will be completely non-obvious to them. I really don't want to be responsible to have submitted that code. If anyone cares for proper device hand-over on ARM, please come up with an idea to fix it. Otherwise, I will just limit the simpledrm code to x86. IOW the device handover code somehow needs to know who was responsible for the instantiation of the simple-framebuffer device, so it can tell them to remove it again. On x86 there is only one place where those can be instantiated. But on OF-based systems, it can be dynamically instantiated in many places right now. Thanks David
On Tue, Aug 30, 2016 at 2:30 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi Rob > > On Fri, Aug 26, 2016 at 2:36 PM, Rob Herring <robh+dt@kernel.org> wrote: >> On Thu, Aug 25, 2016 at 7:00 PM, David Herrmann <dh.herrmann@gmail.com> wrote: >>> Provide a generic DRM helper that evicts all conflicting firmware >>> framebuffers, devices, and drivers. The new helper is called >>> drm_evict_firmware(), and takes a flagset controlling which firmware to >>> kick out. >>> >>> This is meant to be used by drivers in their ->probe() callbacks of their >>> parent bus, before calling into drm_dev_register(). >>> >>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> >>> --- >>> Hey >>> >>> This is just compile-tested for now. I just want to get some comments on the >>> design. I decided to drop the sysfb infrastructure and rather just use this >>> generic helper. It keeps things simple and should work just fine for all >>> reasonable use-cases. >>> >>> This will work with SimpleDRM out-of-the-box on x86. >>> >>> Architectures with dynamic simple-framebuffer devices are not supported yet. I >>> actually have no idea what the strategy there is? Can the DeviceTree people come >>> up with something? Am I supposed to call of_platform_depopulate()? >> >> If of_platform_populate was used to create the device, then yes call >> of_platform_depopulate. In this case, I think it wasn't. If >> of_platform_device_create was used, then platform_device_del. >> >>> Or >>> of_detach_node()? Or what? >> >> No. Only the struct device and its resources should need to be >> destroyed. The node should remain. >> >>> Looking at drivers/of/{platform,dynamic}.c, I cannot see how this is supposed to >>> work at all. Who protects platform_device_del() from being called in parallel? >> >> Not sure. In parallel to what? On most systems, nodes never go away >> and on those that do it is only a few things that get hotplugged. >> That's changing with DT overlays now, so there certainly could be some >> issues. > > Two tasks calling platform_device_del() in parallel on the same device > will not work. Meaning, calling of_platform_device_destroy() in > parallel does not work either. Same for of_platform_depopulate(). Same > for of_node_detach(). Changes to DT nodes and struct device's are completely separate from a DT core perspective ATM. A caller is responsible adding devices when nodes are added, removing the devices, then removing the nodes. The only overlays currently supported require a driver to load them and handle any transitions. DT is still far from dynamic in the sense that any random node can come and go. > Sure, all those functions are not meant to be called in parallel by > multiple tasks. They are rather meant to have a single holder which > preferably is the one instantiating and destroying the > node/device/foobar. But the framebuffer eviction code somehow needs to > trigger the removal, and thus needs some hook, that can be called in > parallel by any driver that is loaded. > > I can make sure the simple-framebuffer eviction code is locked > properly. That'll work, for now. But if someone ends up providing > simple-framebuffer devices via overlays or any other dynamic > mechanism, they will race the removal. No doubt someone will come up with some usecase wanting to do that, but IMO that is not a supported usecase and should not be. simple-framebuffer is for providing a firmware setup framebuffer. > And it will be completely > non-obvious to them. I really don't want to be responsible to have > submitted that code. If anyone cares for proper device hand-over on > ARM, please come up with an idea to fix it. Otherwise, I will just > limit the simpledrm code to x86. > > IOW the device handover code somehow needs to know who was responsible > for the instantiation of the simple-framebuffer device, so it can tell > them to remove it again. On x86 there is only one place where those > can be instantiated. But on OF-based systems, it can be dynamically > instantiated in many places right now. What do you mean by all over the place? It is only in simplefb_init ATM. I haven't looked at what simpledrm is doing, but we can move the device creation to of_platform_default_populate_init if we need a single spot. Rob
Hi David, On Tue, Aug 30, 2016 at 09:30:44PM +0200, David Herrmann wrote: > Hi Rob > IOW the device handover code somehow needs to know who was responsible > for the instantiation of the simple-framebuffer device, so it can tell > them to remove it again. On x86 there is only one place where those > can be instantiated. But on OF-based systems, it can be dynamically > instantiated in many places right now. I don't think that's true. There's the assumption that the bootloader will have set up the framebuffer and Linux just takes over. Even on ARM, you'll need to at least reserve the memory, grab the clocks and so on, and it needs to happen way before you can load overlays (for the buffer even way before the simple-framebuffer driver is probed). I don't see how we could use overlays with simple-framebuffer, unless we apply those overlays before linux starts, but then Linux basically doesn't care if it was an overlay or not, it's treated as a single DT. Maxime
Hi On Tue, Aug 30, 2016 at 10:58 PM, Rob Herring <robh+dt@kernel.org> wrote: > On Tue, Aug 30, 2016 at 2:30 PM, David Herrmann <dh.herrmann@gmail.com> wrote: >> Sure, all those functions are not meant to be called in parallel by >> multiple tasks. They are rather meant to have a single holder which >> preferably is the one instantiating and destroying the >> node/device/foobar. But the framebuffer eviction code somehow needs to >> trigger the removal, and thus needs some hook, that can be called in >> parallel by any driver that is loaded. >> >> I can make sure the simple-framebuffer eviction code is locked >> properly. That'll work, for now. But if someone ends up providing >> simple-framebuffer devices via overlays or any other dynamic >> mechanism, they will race the removal. > > No doubt someone will come up with some usecase wanting to do that, > but IMO that is not a supported usecase and should not be. > simple-framebuffer is for providing a firmware setup framebuffer. Sure. Any sensible simple-framebuffer use would follow what we have now. But it feels wrong to me that if some node is added that just happens to have "simple-framebuffer" as name, suddenly things will go wrong. I mean, yeah, DT is not a userspace API, but I still would like the code to catch misuses rather than fail. It is an API after all. Or is that being overly pedantic? >> And it will be completely >> non-obvious to them. I really don't want to be responsible to have >> submitted that code. If anyone cares for proper device hand-over on >> ARM, please come up with an idea to fix it. Otherwise, I will just >> limit the simpledrm code to x86. >> >> IOW the device handover code somehow needs to know who was responsible >> for the instantiation of the simple-framebuffer device, so it can tell >> them to remove it again. On x86 there is only one place where those >> can be instantiated. But on OF-based systems, it can be dynamically >> instantiated in many places right now. > > What do you mean by all over the place? It is only in simplefb_init > ATM. I haven't looked at what simpledrm is doing, but we can move the > device creation to of_platform_default_populate_init if we need a > single spot. Currently I see at least 3 paths that might add such nodes: - of_platform_populate() - of_node_attach() (via the notifier) - simplefb_init() Should I just ignore anything but simplefb_init()? I understand that it's the only one used by normal code-paths, but isn't it kinda ugly to silently introduce race conditions if a node just happens to be introduced via one of the other methods? Or are errors in the DT not meant to be caught? Thanks David
On Tue, Aug 30, 2016 at 4:12 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi > > On Tue, Aug 30, 2016 at 10:58 PM, Rob Herring <robh+dt@kernel.org> wrote: >> On Tue, Aug 30, 2016 at 2:30 PM, David Herrmann <dh.herrmann@gmail.com> wrote: >>> Sure, all those functions are not meant to be called in parallel by >>> multiple tasks. They are rather meant to have a single holder which >>> preferably is the one instantiating and destroying the >>> node/device/foobar. But the framebuffer eviction code somehow needs to >>> trigger the removal, and thus needs some hook, that can be called in >>> parallel by any driver that is loaded. >>> >>> I can make sure the simple-framebuffer eviction code is locked >>> properly. That'll work, for now. But if someone ends up providing >>> simple-framebuffer devices via overlays or any other dynamic >>> mechanism, they will race the removal. >> >> No doubt someone will come up with some usecase wanting to do that, >> but IMO that is not a supported usecase and should not be. >> simple-framebuffer is for providing a firmware setup framebuffer. > > Sure. Any sensible simple-framebuffer use would follow what we have > now. But it feels wrong to me that if some node is added that just > happens to have "simple-framebuffer" as name, suddenly things will go > wrong. I mean, yeah, DT is not a userspace API, but I still would like > the code to catch misuses rather than fail. It is an API after all. Or > is that being overly pedantic? The kernel could blow up in all sorts of interesting ways if random nodes were created. IMO, it is not the kernel's job to be a DT validator. If it is, it is doing a horrible job. And don't get me wrong, we do need something to do that. That is all orthogonal to dynamic devices and the issues there. >>> And it will be completely >>> non-obvious to them. I really don't want to be responsible to have >>> submitted that code. If anyone cares for proper device hand-over on >>> ARM, please come up with an idea to fix it. Otherwise, I will just >>> limit the simpledrm code to x86. >>> >>> IOW the device handover code somehow needs to know who was responsible >>> for the instantiation of the simple-framebuffer device, so it can tell >>> them to remove it again. On x86 there is only one place where those >>> can be instantiated. But on OF-based systems, it can be dynamically >>> instantiated in many places right now. >> >> What do you mean by all over the place? It is only in simplefb_init >> ATM. I haven't looked at what simpledrm is doing, but we can move the >> device creation to of_platform_default_populate_init if we need a >> single spot. > > Currently I see at least 3 paths that might add such nodes: > > - of_platform_populate() If we assume the node is only under chosen, then that would require a call to of_platform_populate with /chosen as the root which shouldn't happen. More generally the assumption is of_platform_populate is only called once from for the root node and only on sub-trees by the driver of sub-tree's root device. > - of_node_attach() (via the notifier) A node getting attached would be harmless. Nothing really would happen until a handler calls of_platform_populate. So this is the same as the 1st case. > - simplefb_init() Actually, I'd prefer to move it out of there and into the core. I don't think drivers should look at /chosen and only the core and arch code should. Of course, the only enforcing of that is policy. For overlays though, there's been some discussion on limiting where overlays can be applied. > Should I just ignore anything but simplefb_init()? I understand that > it's the only one used by normal code-paths, but isn't it kinda ugly > to silently introduce race conditions if a node just happens to be > introduced via one of the other methods? Or are errors in the DT not > meant to be caught? In general there's no limit to how wrong a DT could be. I could write a DT that causes every DT enabled driver in the kernel to probe (that would be an interesting test case). The kernel is limited in knowing what is correct, and the whole point of DT is to move that information out of the kernel. This is case is just one compatible string out of thousands and location in the tree is just one thing to check. This is a major reason why there is not yet a userspace interface for applying DT overlays as who knows what random crap could be in the DT. We're nervous about what could happen from frying h/w to creating security holes. Evidently the ACPI folks were not so nervous and added an interface. Rob
Hey Rob! On Wed, Aug 31, 2016 at 1:01 AM, Rob Herring <robh+dt@kernel.org> wrote: > On Tue, Aug 30, 2016 at 4:12 PM, David Herrmann <dh.herrmann@gmail.com> wrote: >> Currently I see at least 3 paths that might add such nodes: >> >> - of_platform_populate() > > If we assume the node is only under chosen, then that would require a > call to of_platform_populate with /chosen as the root which shouldn't > happen. More generally the assumption is of_platform_populate is only > called once from for the root node and only on sub-trees by the driver > of sub-tree's root device. I see. >> - of_node_attach() (via the notifier) > > A node getting attached would be harmless. Nothing really would happen > until a handler calls of_platform_populate. So this is the same as the > 1st case. drivers/of/platform.c has a notifier callback and registers the platform devices when added. It has a check that the parent is a populated bus, which I guess is what you refer to? >> - simplefb_init() > > Actually, I'd prefer to move it out of there and into the core. I > don't think drivers should look at /chosen and only the core and arch > code should. Of course, the only enforcing of that is policy. For > overlays though, there's been some discussion on limiting where > overlays can be applied. > >> Should I just ignore anything but simplefb_init()? I understand that >> it's the only one used by normal code-paths, but isn't it kinda ugly >> to silently introduce race conditions if a node just happens to be >> introduced via one of the other methods? Or are errors in the DT not >> meant to be caught? > > In general there's no limit to how wrong a DT could be. I could write > a DT that causes every DT enabled driver in the kernel to probe (that > would be an interesting test case). The kernel is limited in knowing > what is correct, and the whole point of DT is to move that information > out of the kernel. This is case is just one compatible string out of > thousands and location in the tree is just one thing to check. > > This is a major reason why there is not yet a userspace interface for > applying DT overlays as who knows what random crap could be in the DT. > We're nervous about what could happen from frying h/w to creating > security holes. Evidently the ACPI folks were not so nervous and added > an interface. I see. Thanks a lot for the clarifications! I will put the of_platform_device_destroy() right next to the x86 handler, making sure it is properly locked. I'm not so sure about the of_chosen instantiation, though. x86 has the instantiation in arch/x86/kernel/sysfb_simplefb.c, so maybe an equivalent for arm would be fine? Thanks David
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 0773547..581c342 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -26,12 +26,16 @@ * DEALINGS IN THE SOFTWARE. */ +#include <linux/console.h> #include <linux/debugfs.h> +#include <linux/fb.h> #include <linux/fs.h> #include <linux/module.h> #include <linux/moduleparam.h> #include <linux/mount.h> +#include <linux/of.h> #include <linux/slab.h> +#include <linux/vt.h> #include <drm/drmP.h> #include "drm_crtc_internal.h" #include "drm_legacy.h" @@ -881,6 +885,259 @@ void drm_dev_unregister(struct drm_device *dev) } EXPORT_SYMBOL(drm_dev_unregister); +struct drm_evict_ctx { + struct apertures_struct *ap; + unsigned int flags; +}; + +static bool drm_evict_match_resource(struct drm_evict_ctx *ctx, + struct resource *mem) +{ + struct aperture *g; + unsigned int i; + + for (i = 0; i < ctx->ap->count; ++i) { + g = &ctx->ap->ranges[i]; + + if (mem->start == g->base) + return true; + if (mem->start >= g->base && mem->end < g->base + g->size) + return true; + if ((ctx->flags & DRM_EVICT_VBE) && mem->start == 0xA0000) + return true; + } + + return false; +} + +static int drm_evict_platform_device(struct device *dev, void *userdata) +{ + struct drm_evict_ctx *ctx = userdata; + struct platform_device *pdev = to_platform_device(dev); + struct resource *mem; + + /* + * We are only interested in static devices here (i.e., they carry + * information via a statically allocated platform data field). Any + * dynamically allocated devices have separate ownership models and + * must be handled via their respective management calls. + */ + if (!dev_get_platdata(&pdev->dev)) + return 0; + if (!pdev->name) + return 0; + + if (!strcmp(pdev->name, "simple-framebuffer")) { + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!mem) + return 0; + if (!drm_evict_match_resource(ctx, mem)) + return 0; + + platform_device_del(pdev); + } + + return 0; +} + +static int drm_evict_platform(struct drm_evict_ctx *ctx) +{ + /* + * Early-boot architecture setup and boot-loarders sometimes create + * preliminary platform devices with a generic framebuffer setup. This + * allows graphics access during boot-up, without a real graphics + * driver loaded. However, once a real graphics driver takes over, we + * have to destroy those platform devices. In the legacy fbdev case, we + * just used to unload the fbdev driver. However, to make sure any kind + * of driver is unloaded, the platform-eviction code here simply + * removes any conflicting platform device directly. This causes any + * bound driver to be detached, and then removes the device entirely so + * it cannot be bound to later on. + * + * Please note that any such platform device must be registered by + * early architecture setup code. If they are registered after regular + * DRM drivers, this will fail horribly. + */ + + static DEFINE_MUTEX(lock); + int ret; + + /* + * In case of static platform-devices, we must iterate the bus and + * remove them manually. We know that we're the only code that might + * remove them, so a simple static lock serializes all calls here. We + * must make sure our eviction callback leaves any non-static platform + * devices untouched, though. + * + * XXX: ARM and other OF users should really make sure to add their own + * eviction code here, possibly using the 'display' DT-handle as + * defined by the 'simple-framebuffer' OF-node. + */ + mutex_lock(&lock); + ret = bus_for_each_dev(&platform_bus_type, NULL, ctx, + drm_evict_platform_device); + mutex_unlock(&lock); + return ret; +} + +static int drm_evict_fbdev(struct drm_evict_ctx *ctx) +{ + /* + * We have 2 different cases where fbdev drivers can operate on the + * same devices as DRM drivers: + * + * o Two drivers exist for the same device, one DRM driver and one + * fbdev driver (e.g., nvidiafb and nouveau). In those cases, both + * drivers bind to the same real hw-device and driver core takes + * care of any conflicts. + * + * o A generic fbdev driver uses some pseudo-device to provide + * early-boot graphics access or some other kind of generic display + * driver. At the same time, a DRM driver exists that can run the + * real hardware (e.g., vesafb/efifb and i915). + * + * If possible, driver core takes care of any conflicts and prevents + * two drivers from being bound to the same device. However, in some + * cases this is not possible. We then use + * remove_conflicting_framebuffers() to match the apertures of the DRM + * device against all existing fbdev devices, evicting any conflicting + * fbdev drivers. Additionally, if requested, any VBE driver is evicted + * as well. + * + * Note that this breaks horribly if an fbdev driver is probed after a + * DRM driver. So make sure whenever such conflicts are possible, the + * fbdev drivers must be probed in early boot. If you cannot guarantee + * that, you better make sure that the driver core prevents both + * drivers from being probed in parallel. + */ + +#if !defined(CONFIG_FB) + return 0; +#else + return remove_conflicting_framebuffers(ctx->ap, "drm", + !!(ctx->flags & DRM_EVICT_VBE)); +#endif +} + +static int drm_evict_vgacon(void) +{ + /* + * The VGACON console driver pokes at VGA registers randomly. If a DRM + * driver cannot keep the VGA support alive, it better makes sure to + * unload VGACON before probing. + * + * Unloading VGACON requires us to first force dummycon to take over + * from vgacon (but only if vgacon is really in use), followed by a + * deregistration of vgacon. Note that this prevents vgacon from being + * used again after the DRM driver is unloaded. But that is usually + * fine, since VGA state is rarely restored on driver-unload, anyway. + * + * Note that we rely on VGACON to be probed in early boot (actually + * done by ARCH setup code). If it is probed after DRM drivers, this + * will fail horribly. You better make sure VGACON is probed early and + * DRM drivers are probed as normal modules. + */ + +#if !defined(CONFIG_VGA_CONSOLE) + return 0; +#elif !defined(CONFIG_DUMMY_CONSOLE) +# warning "Dummy console is disabled, but required to kick out VGACON" + return -ENODEV; +#else + int ret = 0; + + DRM_INFO("Replacing VGA console driver\n"); + + console_lock(); + if (con_is_bound(&vga_con)) + ret = do_take_over_console(&dummy_con, 0, + MAX_NR_CONSOLES - 1, 1); + if (ret == 0) { + ret = do_unregister_con_driver(&vga_con); + if (ret == -ENODEV) /* ignore "already unregistered" */ + ret = 0; + } + console_unlock(); + + return ret; +#endif +} + +/** + * drm_evict_firmware - remove any conflicting firmware-framebuffers + * @ap: apertures claimed by driver, or NULL + * @flags: eviction flags + * + * This function evicts any conflicting firmware-framebuffers and their bound + * drivers, according to the data given as @ap and @flags. + * + * Depending on @flags, the following operations are performed: + * + * DRM_EVICT_FBDEV: Any fbdev drivers that overlap @ap are unloaded. + * Furthermore, if DRM_EVICT_VBE is given as well, any fbdev + * driver that maps the VGA region is unloaded. + * + * DRM_EVICT_PLATFORM: Firmware framebuffer platform devices (eg., + * 'simple-framebuffer') that overlap @ap are removed + * from the system, causing drivers to be unbound. + * If DRM_EVICT_VBE is given, this also affects devices + * that map the VGA region. + * + * DRM_EVICT_VGACON: The vgacon console driver is unbound and unregistered. + * + * The caller must provide their apertures as @ap. If it is NULL, an empty set + * is assumed, except if DRM_EVICT_ANYWHERE is given, in which case a fake + * aperture across the entire address range is used. + * + * RETURNS: + * 0 on success, negative error code on failure. + */ +int drm_evict_firmware(struct apertures_struct *ap, unsigned int flags) +{ + struct drm_evict_ctx ctx; + struct apertures_struct *allocated_ap = NULL; + int ret; + + WARN_ON(ap && (flags & DRM_EVICT_ANYWHERE)); + + if (!ap) { + allocated_ap = alloc_apertures(!!(flags & DRM_EVICT_ANYWHERE)); + if (!allocated_ap) + return -ENOMEM; + + ap = allocated_ap; + + if (flags & DRM_EVICT_ANYWHERE) { + ap->ranges[0].base = 0; + ap->ranges[0].size = ~0; + } + } + + ctx.ap = ap; + ctx.flags = flags; + + if (flags & DRM_EVICT_PLATFORM) { + ret = drm_evict_platform(&ctx); + if (ret < 0) + return ret; + } + + if (flags & DRM_EVICT_FBDEV) { + ret = drm_evict_fbdev(&ctx); + if (ret < 0) + return ret; + } + + if (flags & DRM_EVICT_VGACON) { + ret = drm_evict_vgacon(); + if (ret < 0) + return ret; + } + + kfree(allocated_ap); + return 0; +} + /* * DRM Core * The DRM core module initializes all global DRM objects and makes them diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 2197ab1..6c8bcf5 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -88,6 +88,7 @@ struct drm_gem_object; struct drm_master; struct drm_vblank_crtc; +struct apertures_struct; struct device_node; struct videomode; struct reservation_object; @@ -971,6 +972,18 @@ void drm_dev_unref(struct drm_device *dev); int drm_dev_register(struct drm_device *dev, unsigned long flags); void drm_dev_unregister(struct drm_device *dev); +enum { + DRM_EVICT_FBDEV = (1U << 0), + DRM_EVICT_PLATFORM = (1U << 1), + DRM_EVICT_VGACON = (1U << 2), + DRM_EVICT_VBE = (1U << 3), + DRM_EVICT_ANYWHERE = (1U << 4), + + DRM_EVICT_DEFAULT = DRM_EVICT_FBDEV, +}; + +int drm_evict_firmware(struct apertures_struct *ap, unsigned int flags); + struct drm_minor *drm_minor_acquire(unsigned int minor_id); void drm_minor_release(struct drm_minor *minor);
Provide a generic DRM helper that evicts all conflicting firmware framebuffers, devices, and drivers. The new helper is called drm_evict_firmware(), and takes a flagset controlling which firmware to kick out. This is meant to be used by drivers in their ->probe() callbacks of their parent bus, before calling into drm_dev_register(). Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- Hey This is just compile-tested for now. I just want to get some comments on the design. I decided to drop the sysfb infrastructure and rather just use this generic helper. It keeps things simple and should work just fine for all reasonable use-cases. This will work with SimpleDRM out-of-the-box on x86. Architectures with dynamic simple-framebuffer devices are not supported yet. I actually have no idea what the strategy there is? Can the DeviceTree people come up with something? Am I supposed to call of_platform_depopulate()? Or of_detach_node()? Or what? Looking at drivers/of/{platform,dynamic}.c, I cannot see how this is supposed to work at all. Who protects platform_device_del() from being called in parallel? Also: Does any platform make use the of 'display:' entry in 'simple-framebuffer' DT nodes? If yes, how do I get access to it? And why don't vc4/sun4i make use of it, rather than falling back to remove_conflicting_framebuffers()? Thanks David drivers/gpu/drm/drm_drv.c | 257 ++++++++++++++++++++++++++++++++++++++++++++++ include/drm/drmP.h | 13 +++ 2 files changed, 270 insertions(+)