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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Oct 22, 2021 at 11:16 AM Javier Martinez Canillas
<javierm@xxxxxxxxxx> 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@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...
>

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!




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux