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