[RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[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@gmail.com>
Signed-off-by: Javier Martinez Canillas <javierm@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;
+module_param_named(remove_fb, drm_aperture_remove_fb, bool, 0600);
+MODULE_PARM_DESC(remove_fb,
+		 "Allow conflicting framebuffers removal [default=true]");
+
 /**
  * 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.
+ *
  * 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)
 	struct apertures_struct *a;
 	int ret;
+#endif
+
+	if (!drm_aperture_remove_fb)
+		return -EINVAL;
 
+#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;
+
 	for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
 		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
 			continue;
-- 
2.31.1

Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[Neal Gompa]

On Fri, Oct 22, 2021 at 10:40 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> 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@gmail.com>
> Signed-off-by: Javier Martinez Canillas <javierm@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').
>

In general, I think the patch is fine, but it might make sense to name
the parameter after the *effect* rather than the *action*. That is,
the effect of this option is that we don't probe and hand over to a
more appropriate hardware DRM driver.

Since the effect (in DRM terms) is that we don't use platform DRM
modules, perhaps we could name the option one of these:

* drm.noplatformdrv
* drm.simpledrv
* drm.force_simple

I'm inclined to say we should use the drm.* namespace for the cmdline
option because that makes it clear what subsystem it affects. The
legacy "nomodeset" option kind of sucked because it didn't really tell
you what that meant, and I'd rather not repeat that mistake.




--
真実はいつも一つ！/ Always, there's only one truth!

Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[Javier Martinez Canillas]

Hello Neal,

Thanks for your feedback.

On 10/22/21 16:56, Neal Gompa wrote:
> On Fri, Oct 22, 2021 at 10:40 AM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>>
>> 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@gmail.com>
>> Signed-off-by: Javier Martinez Canillas <javierm@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').
>>
> 
> In general, I think the patch is fine, but it might make sense to name
> the parameter after the *effect* rather than the *action*. That is,
> the effect of this option is that we don't probe and hand over to a
> more appropriate hardware DRM driver.
> 
> Since the effect (in DRM terms) is that we don't use platform DRM
> modules, perhaps we could name the option one of these:
> 
> * drm.noplatformdrv
> * drm.simpledrv
> * drm.force_simple
>

or maybe drm.disable_handover ? Naming is hard...
 
> I'm inclined to say we should use the drm.* namespace for the cmdline
> option because that makes it clear what subsystem it affects. The
> legacy "nomodeset" option kind of sucked because it didn't really tell
> you what that meant, and I'd rather not repeat that mistake.
>

Yes, agreed. In fact, that is the case for this patch since the param is
in the drm module. I just forgot to mention the namespace in the last
paragraph of the comment.

Best regards, -- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[Neal Gompa]

On Fri, Oct 22, 2021 at 11:16 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Hello Neal,
>
> Thanks for your feedback.
>
> On 10/22/21 16:56, Neal Gompa wrote:
> > On Fri, Oct 22, 2021 at 10:40 AM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> >>
> >> 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@gmail.com>
> >> Signed-off-by: Javier Martinez Canillas <javierm@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').
> >>
> >
> > In general, I think the patch is fine, but it might make sense to name
> > the parameter after the *effect* rather than the *action*. That is,
> > the effect of this option is that we don't probe and hand over to a
> > more appropriate hardware DRM driver.
> >
> > Since the effect (in DRM terms) is that we don't use platform DRM
> > modules, perhaps we could name the option one of these:
> >
> > * drm.noplatformdrv
> > * drm.simpledrv
> > * drm.force_simple
> >
>
> or maybe drm.disable_handover ? Naming is hard...
>

That would make sense for a parameter named by the action. If we want
to go that route, then I'd be fine with that. My goal is to have
something people understand.

> > I'm inclined to say we should use the drm.* namespace for the cmdline
> > option because that makes it clear what subsystem it affects. The
> > legacy "nomodeset" option kind of sucked because it didn't really tell
> > you what that meant, and I'd rather not repeat that mistake.
> >
>
> Yes, agreed. In fact, that is the case for this patch since the param is
> in the drm module. I just forgot to mention the namespace in the last
> paragraph of the comment.
>

Good to know. :)



--
真実はいつも一つ！/ Always, there's only one truth!

Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 6120 bytes --]

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@gmail.com>
> Signed-off-by: Javier Martinez Canillas <javierm@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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[Ville Syrjälä]

On Fri, Oct 22, 2021 at 04:40:40PM +0200, Javier Martinez Canillas wrote:
> 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.

Why is that better than just modprobe.blacklisting those drivers?

-- 
Ville Syrjälä
Intel

Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[Javier Martinez Canillas]

Hello Ville,

On 10/22/21 21:12, Ville Syrjälä wrote:
> On Fri, Oct 22, 2021 at 04:40:40PM +0200, Javier Martinez Canillas wrote:
>> 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.
> 
> Why is that better than just modprobe.blacklisting those drivers?
> 

Because would allow to deny list all native (as Thomas called it) DRM drivers
and only allow the simpledrm driver to be probed. This is useful for distros,
since could add a "Basic graphics mode" to the boot menu entries, that could
boot the kernel passing a "drm.disable_native_drivers=1" cmdline option.

That way, if there's any problem with a given DRM driver, the distro may be
installed and booted using the simpledrm driver and troubleshoot why a native
DRM driver is not working. Or try updating the kernel package, etc.

Currently what most distros do is to pass "nomodeset" in this mode. But now
that we have the simpledrm driver, would be nice to just use the frame buffer
set by the system firmware in those cases.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[Javier Martinez Canillas]

Hello Thomas,

Thanks a lot for your feedback.

On 10/22/21 21:05, Thomas Zimmermann wrote:

[snip]

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

Agreed. I'll change that.
 
>>   /**
>>    * 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.
>

Sure, makes sense. I was pondering between -EINVAL, -EBUSY and -EPERM.

>> + *
>>    * 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;
> }
>

Ok.

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

That works for me and "drm.disable_native_drivers" is also what Neal meant
with his "drm.noplatformdrv", but yours is much easier to remember / type.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[Neal Gompa]

On Sun, Oct 24, 2021 at 4:40 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Hello Thomas,
>
> Thanks a lot for your feedback.
>
> On 10/22/21 21:05, Thomas Zimmermann wrote:
>
> > 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'
>
> That works for me and "drm.disable_native_drivers" is also what Neal meant
> with his "drm.noplatformdrv", but yours is much easier to remember / type.
>

I'm good with that too. :)

-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[Michel Dänzer]

On 2021-10-24 22:32, Javier Martinez Canillas wrote:
> Hello Ville,
> 
> On 10/22/21 21:12, Ville Syrjälä wrote:
>> On Fri, Oct 22, 2021 at 04:40:40PM +0200, Javier Martinez Canillas wrote:
>>> 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.
>>
>> Why is that better than just modprobe.blacklisting those drivers?
> 
> Because would allow to deny list all native (as Thomas called it) DRM drivers
> and only allow the simpledrm driver to be probed. This is useful for distros,
> since could add a "Basic graphics mode" to the boot menu entries, that could
> boot the kernel passing a "drm.disable_native_drivers=1" cmdline option.
> 
> That way, if there's any problem with a given DRM driver, the distro may be
> installed and booted using the simpledrm driver and troubleshoot why a native
> DRM driver is not working. Or try updating the kernel package, etc.

For troubleshooting, it'll be helpful if this new parameter can be enabled for the boot via the kernel command line, then disabled again after boot-up. One simple possibility for this would be allowing the parameter to be changed via /sys/module/drm/parameters/<name>, which I suspect doesn't work with the patch as is (due to the 0600 permissions).


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[Javier Martinez Canillas]

Hello Michel,

On 10/25/21 12:45, Michel Dänzer wrote:
> On 2021-10-24 22:32, Javier Martinez Canillas wrote:
>> Hello Ville,
>>
>> On 10/22/21 21:12, Ville Syrjälä wrote:
>>> On Fri, Oct 22, 2021 at 04:40:40PM +0200, Javier Martinez Canillas wrote:
>>>> 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.
>>>
>>> Why is that better than just modprobe.blacklisting those drivers?
>>
>> Because would allow to deny list all native (as Thomas called it) DRM drivers
>> and only allow the simpledrm driver to be probed. This is useful for distros,
>> since could add a "Basic graphics mode" to the boot menu entries, that could
>> boot the kernel passing a "drm.disable_native_drivers=1" cmdline option.
>>
>> That way, if there's any problem with a given DRM driver, the distro may be
>> installed and booted using the simpledrm driver and troubleshoot why a native
>> DRM driver is not working. Or try updating the kernel package, etc.
> 
> For troubleshooting, it'll be helpful if this new parameter can be enabled for the boot via the kernel command line, then disabled again after boot-up. One simple possibility for this would be allowing the parameter to be changed via /sys/module

That's already the case with the current patch, i.e:

$ grep -o drm.* /proc/cmdline 
drm.disable_native_drivers=1

$ cat /proc/fb 
0 simpledrm

$ modprobe virtio_gpu

$ dmesg
[  125.731549] [drm] pci: virtio-vga detected at 0000:00:01.0
[  125.732410] virtio_gpu: probe of virtio0 failed with error -16

$ echo 0 > /sys/module/drm/parameters/disable_native_drivers

$ modprobe virtio_gpu

$ dmesg 
[  187.889136] [drm] pci: virtio-vga detected at 0000:00:01.0
[  187.894578] Console: switching to colour dummy device 80x25
[  187.897090] virtio-pci 0000:00:01.0: vgaarb: deactivate vga console
[  187.899983] [drm] features: -virgl +edid -resource_blob -host_visible
[  187.907176] [drm] number of scanouts: 1
[  187.907714] [drm] number of cap sets: 0
[  187.914108] [drm] Initialized virtio_gpu 0.1.0 0 for virtio0 on minor 1
[  187.930807] Console: switching to colour frame buffer device 128x48
[  187.938737] virtio_gpu virtio0: [drm] fb0: virtio_gpu frame buffer device

$ cat /proc/fb 
0 virtio_gpu

/drm/parameters/<name>, which I suspect doesn't work with the patch as is (due to the 0600 permissions).
> 
> 

I followed the convention used by other drm module parameters, hence the
0600. Do you mean that for this parameter we should be less restrictive ?

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[Neal Gompa]

On Mon, Oct 25, 2021 at 8:28 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Hello Michel,
>
> On 10/25/21 12:45, Michel Dänzer wrote:
> > On 2021-10-24 22:32, Javier Martinez Canillas wrote:
> >> Hello Ville,
> >>
> >> On 10/22/21 21:12, Ville Syrjälä wrote:
> >>> On Fri, Oct 22, 2021 at 04:40:40PM +0200, Javier Martinez Canillas wrote:
> >>>> 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.
> >>>
> >>> Why is that better than just modprobe.blacklisting those drivers?
> >>
> >> Because would allow to deny list all native (as Thomas called it) DRM drivers
> >> and only allow the simpledrm driver to be probed. This is useful for distros,
> >> since could add a "Basic graphics mode" to the boot menu entries, that could
> >> boot the kernel passing a "drm.disable_native_drivers=1" cmdline option.
> >>
> >> That way, if there's any problem with a given DRM driver, the distro may be
> >> installed and booted using the simpledrm driver and troubleshoot why a native
> >> DRM driver is not working. Or try updating the kernel package, etc.
> >
> > For troubleshooting, it'll be helpful if this new parameter can be enabled for the boot via the kernel command line, then disabled again after boot-up. One simple possibility for this would be allowing the parameter to be changed via /sys/module
>
> That's already the case with the current patch, i.e:
>
> $ grep -o drm.* /proc/cmdline
> drm.disable_native_drivers=1
>
> $ cat /proc/fb
> 0 simpledrm
>
> $ modprobe virtio_gpu
>
> $ dmesg
> [  125.731549] [drm] pci: virtio-vga detected at 0000:00:01.0
> [  125.732410] virtio_gpu: probe of virtio0 failed with error -16
>
> $ echo 0 > /sys/module/drm/parameters/disable_native_drivers
>
> $ modprobe virtio_gpu
>
> $ dmesg
> [  187.889136] [drm] pci: virtio-vga detected at 0000:00:01.0
> [  187.894578] Console: switching to colour dummy device 80x25
> [  187.897090] virtio-pci 0000:00:01.0: vgaarb: deactivate vga console
> [  187.899983] [drm] features: -virgl +edid -resource_blob -host_visible
> [  187.907176] [drm] number of scanouts: 1
> [  187.907714] [drm] number of cap sets: 0
> [  187.914108] [drm] Initialized virtio_gpu 0.1.0 0 for virtio0 on minor 1
> [  187.930807] Console: switching to colour frame buffer device 128x48
> [  187.938737] virtio_gpu virtio0: [drm] fb0: virtio_gpu frame buffer device
>
> $ cat /proc/fb
> 0 virtio_gpu
>
> /drm/parameters/<name>, which I suspect doesn't work with the patch as is (due to the 0600 permissions).
> >
> >
>
> I followed the convention used by other drm module parameters, hence the
> 0600. Do you mean that for this parameter we should be less restrictive ?
>

I would think that the 600 permissions would still permit it, since
the root user can still access and manipulate it.


-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[Michel Dänzer]

On 2021-10-25 14:28, Javier Martinez Canillas wrote:
> Hello Michel,
> 
> On 10/25/21 12:45, Michel Dänzer wrote:
>> On 2021-10-24 22:32, Javier Martinez Canillas wrote:
>>> Hello Ville,
>>>
>>> On 10/22/21 21:12, Ville Syrjälä wrote:
>>>> On Fri, Oct 22, 2021 at 04:40:40PM +0200, Javier Martinez Canillas wrote:
>>>>> 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.
>>>>
>>>> Why is that better than just modprobe.blacklisting those drivers?
>>>
>>> Because would allow to deny list all native (as Thomas called it) DRM drivers
>>> and only allow the simpledrm driver to be probed. This is useful for distros,
>>> since could add a "Basic graphics mode" to the boot menu entries, that could
>>> boot the kernel passing a "drm.disable_native_drivers=1" cmdline option.
>>>
>>> That way, if there's any problem with a given DRM driver, the distro may be
>>> installed and booted using the simpledrm driver and troubleshoot why a native
>>> DRM driver is not working. Or try updating the kernel package, etc.
>>
>> For troubleshooting, it'll be helpful if this new parameter can be enabled for the boot via the kernel command line, then disabled again after boot-up. One simple possibility for this would be allowing the parameter to be changed via /sys/module
> 
> That's already the case with the current patch, i.e:
> 
> $ grep -o drm.* /proc/cmdline 
> drm.disable_native_drivers=1
> 
> $ cat /proc/fb 
> 0 simpledrm
> 
> $ modprobe virtio_gpu
> 
> $ dmesg
> [  125.731549] [drm] pci: virtio-vga detected at 0000:00:01.0
> [  125.732410] virtio_gpu: probe of virtio0 failed with error -16
> 
> $ echo 0 > /sys/module/drm/parameters/disable_native_drivers
> 
> $ modprobe virtio_gpu
> 
> $ dmesg 
> [  187.889136] [drm] pci: virtio-vga detected at 0000:00:01.0
> [  187.894578] Console: switching to colour dummy device 80x25
> [  187.897090] virtio-pci 0000:00:01.0: vgaarb: deactivate vga console
> [  187.899983] [drm] features: -virgl +edid -resource_blob -host_visible
> [  187.907176] [drm] number of scanouts: 1
> [  187.907714] [drm] number of cap sets: 0
> [  187.914108] [drm] Initialized virtio_gpu 0.1.0 0 for virtio0 on minor 1
> [  187.930807] Console: switching to colour frame buffer device 128x48
> [  187.938737] virtio_gpu virtio0: [drm] fb0: virtio_gpu frame buffer device
> 
> $ cat /proc/fb 
> 0 virtio_gpu
> 
> /drm/parameters/<name>, which I suspect doesn't work with the patch as is (due to the 0600 permissions).
>>
>>
> 
> I followed the convention used by other drm module parameters, hence the
> 0600. Do you mean that for this parameter we should be less restrictive ?

No, it was simply a brain fart on my part. :}

Thanks for confirming this works!


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer