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@xxxxxxxxxx> 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@xxxxxxxxx> >> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> >> --- >> 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
