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

From: Neal Gompa <ngompa13@gmail.com>
To: Javier Martinez Canillas <javierm@redhat.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Peter Robinson <pbrobinson@gmail.com>,
	Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	dri-devel@lists.freedesktop.org
Subject: Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal
Date: Fri, 22 Oct 2021 10:56:16 -0400	[thread overview]
Message-ID: <CAEg-Je_v0zvOs1dOZ3P0qsPDO7LC8xk0zxQBLH6gr65V82dnPA@mail.gmail.com> (raw)
In-Reply-To: <20211022144040.3418284-1-javierm@redhat.com>

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!