Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <352163b2-2946-aec8-16b4-fd6f01373ff2@redhat.com>
Date:   Fri, 22 Oct 2021 17:16:16 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.1.0
Subject: Re: [RFC PATCH] drm/aperture: Add param to disable conflicting
 framebuffers removal
Content-Language: en-US
To:     Neal Gompa <ngompa13@gmail.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
References: <20211022144040.3418284-1-javierm@redhat.com>
 <CAEg-Je_v0zvOs1dOZ3P0qsPDO7LC8xk0zxQBLH6gr65V82dnPA@mail.gmail.com>
From:   Javier Martinez Canillas <javierm@redhat.com>
In-Reply-To: <CAEg-Je_v0zvOs1dOZ3P0qsPDO7LC8xk0zxQBLH6gr65V82dnPA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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