[RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal
Thomas Zimmermann
tzimmermann at suse.de
Fri Oct 22 19:05:49 UTC 2021
Hi,
thanks for sending the patch out quickly.
Am 22.10.21 um 16:40 schrieb Javier Martinez Canillas:
> The simpledrm driver allows to use the frame buffer that was set-up by the
> firmware. This gives early video output before the platform DRM driver is
> probed and takes over.
>
> But it would be useful to have a way to disable this take over by the real
> DRM drivers. For example, there may be bugs in the DRM drivers that could
> cause the display output to not work correctly.
>
> For those cases, it would be good to keep the simpledrm driver instead and
> at least get a working display as set-up by the firmware.
>
> Let's add a drm.remove_fb boolean kernel command line parameter, that when
> set to false will prevent the conflicting framebuffers to being removed.
>
> Since the drivers call drm_aperture_remove_conflicting_framebuffers() very
> early in their probe callback, this will cause the drivers' probe to fail.
>
> Thanks to Neal Gompa for the suggestion and Thomas Zimmermann for the idea
> on how this could be implemented.
>
> Suggested-by: Neal Gompa <ngompa13 at gmail.com>
> Signed-off-by: Javier Martinez Canillas <javierm at redhat.com>
> ---
> Hello,
>
> I'm sending this as an RFC because I wasn't sure about the correct name for
> this module parameter, and also if 'remove_fb=0' is intitutive or instead a
> parameter that's enabled is preferred (i.e: 'disable_fb_removal=1').
>
> Best regards,
> Javier
>
> drivers/gpu/drm/drm_aperture.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_aperture.c b/drivers/gpu/drm/drm_aperture.c
> index 74bd4a76b253..0b454c8f7465 100644
> --- a/drivers/gpu/drm/drm_aperture.c
> +++ b/drivers/gpu/drm/drm_aperture.c
> @@ -14,6 +14,11 @@
> #include <drm/drm_drv.h>
> #include <drm/drm_print.h>
>
> +static bool drm_aperture_remove_fb = true;
Global variables should default to zero if somehow possible. This way,
they can all be stored in the BSS segment and backed by a single shared
zero-filled page. Otherwise they require actual memory. In the worst
case, you'd allocate a full page to hold a single boolean.
> +module_param_named(remove_fb, drm_aperture_remove_fb, bool, 0600);
> +MODULE_PARM_DESC(remove_fb,
> + "Allow conflicting framebuffers removal [default=true]");
> +
And with variables set to zero, a command-line parameter enables
non-default behavior (i.e., "drm-param=1"). That more logical than the
other way around IMHO.
> /**
> * DOC: overview
> *
> @@ -283,6 +288,9 @@ static void drm_aperture_detach_drivers(resource_size_t base, resource_size_t si
> * This function removes graphics device drivers which use memory range described by
> * @base and @size.
> *
> + * The conflicting framebuffers removal can be disabled by setting the drm.remove_fb=0 kernel
> + * command line option. When this is disabled, the function will return an -EINVAL errno code.
Please use -EBUSY for the error. That's what the acquire function
returns in case of a conflict.
> + *
> * Returns:
> * 0 on success, or a negative errno code otherwise
> */
> @@ -292,7 +300,12 @@ int drm_aperture_remove_conflicting_framebuffers(resource_size_t base, resource_
> #if IS_REACHABLE(CONFIG_FB)
Rather not split up this block. It's better style to put the
fbdev-related code into a helper and call it unconditionally.
static drm_aperture_remove_conflicting_fbdev_framebuffers()
{
#if (FB)
...
#endif
return 0;
}
> struct apertures_struct *a;
> int ret;
> +#endif
> +
> + if (!drm_aperture_remove_fb)
> + return -EINVAL;
There's still the question of the semantics of this parameter. It's a
bit fuzzy.
If you use 'disable_handover' (as you mentioned in another mail), it
would mean that only the handover itself is disabled. So if simpledrm is
not bound to the device, then a native driver should load. That would be
hard to implement with the current code base, where we have to take old
fbdev drivers into account.
(And to be pedantic, we don't really do a handover of the device. We
hot-unplug the generic platform device, so that the driver for the
native device can operate the HW without interference.)
Simpledrm only acquires an aperture, but never removes a driver. If
there is a driver already, simpledrm would fail. Only native drivers try
to remove drivers and would trigger the test. So your patch is more
something like 'disable_native_drivers'.
I'd go with 'disable_native_drivers', or maybe 'disable_device_handover'
as a second option. If simpledrm, or any other generic DRM driver, would
ever try to remove an existing driver from a device, we'd have to
distinguish between native and generic drivers. But that's a trivial
problem for later.
>
> +#if IS_REACHABLE(CONFIG_FB)
> a = alloc_apertures(1);
> if (!a)
> return -ENOMEM;
> @@ -322,6 +335,9 @@ EXPORT_SYMBOL(drm_aperture_remove_conflicting_framebuffers);
> * for any of @pdev's memory bars. The function assumes that PCI device with
> * shadowed ROM drives a primary display and so kicks out vga16fb.
> *
> + * The conflicting framebuffers removal can be disabled by setting the drm.remove_fb=0 kernel
> + * command line option. When this is disabled, the function will return an -EINVAL errno code.
> + *
> * Returns:
> * 0 on success, or a negative errno code otherwise
> */
> @@ -331,6 +347,9 @@ int drm_aperture_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
> resource_size_t base, size;
> int bar, ret = 0;
>
> + if (!drm_aperture_remove_fb)
> + return -EINVAL;
-EBUSY again
Best regards
Thomas
> +
> for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
> if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
> continue;
>
--
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/20211022/a8f03b88/attachment.sig>
More information about the dri-devel
mailing list