Re: [PATCH v2 31/37] drm: rcar-du: Add support for the nomodeset kernel parameter

[Laurent Pinchart]

Hi Thomas,

On Fri, Jan 28, 2022 at 11:46:49AM +0100, Thomas Zimmermann wrote:
> Am 28.01.22 um 11:34 schrieb Laurent Pinchart:
> > On Fri, Jan 28, 2022 at 10:33:21AM +0100, Thomas Zimmermann wrote:
> >> Am 28.01.22 um 10:13 schrieb Kieran Bingham:
> >>> Quoting Javier Martinez Canillas (2021-12-17 00:37:46)
> >>>> According to disable Documentation/admin-guide/kernel-parameters.txt, this
> >>>> parameter can be used to disable kernel modesetting.
> >>>>
> >>>> DRM drivers will not perform display-mode changes or accelerated rendering
> >>>> and only the system framebuffer will be available if it was set-up.
> >>>
> >>> What is the 'system framebuffer' in this instance? Reading
> >>> https://www.kernel.org/doc/Documentation/admin-guide/kernel-parameters.txt
> >>> it sounds like that means anything already set up by the bootloader.
> >>
> >> Exactly this.
> >>
> >>>> But only a few DRM drivers currently check for nomodeset, make this driver
> >>>> to also support the command line parameter.
> >>>>
> >>>> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> >>>> ---
> >>>>
> >>>> (no changes since v1)
> >>>>
> >>>>    drivers/gpu/drm/rcar-du/rcar_du_drv.c | 3 +++
> >>>>    1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >>>> index 5a8131ef81d5..982e450233ed 100644
> >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >>>> @@ -701,6 +701,9 @@ static struct platform_driver rcar_du_platform_driver = {
> >>>>    
> >>>>    static int __init rcar_du_init(void)
> >>>>    {
> >>>> +       if (drm_firmware_drivers_only())
> >>>> +               return -ENODEV;
> >>>> +
> >>>
> >>> This will completely disable all control of the display device when
> >>> nomodeset is enabled.
> >>>
> >>> Is there any requirement for us to support outputting to the display if
> >>> it was previously set up? presumably without setting or changing any
> >>> modes, but simply allowing the existing frame to be updated?
> >>
> >> There's no requirement for your driver. We just want a parameter where
> >> we can conveniently disable most of DRM's drivers and reduce it to a
> >> minimum. Helps distributions to provide a simple fallback mode.  Most
> >> PCI-based drivers already support that. Now we're added it to the other
> >> drivers as well.
> >>
> >>>
> >>> I think the implication is that 'firmware drivers' would mean a display
> >>> could be updated through some firmware interface, which we won't have
> >>> ... so it seems reasonable to accept that this whole driver can be
> >>> disabled in that instance.
> >>
> >> It cannot be 'mode-setted'. We get a pre-configured framebuffer from the
> >> firmware or bootloader. Whatever we draw there shows up on the screen.
> > 
> > I doubt that's going to work as you expect, clocks and regulators will
> > get disabled at boot if not used by any driver.
> 
> Simpledrm and simplefb attach to these firmware framebuffers. Both 
> drivers look at the device tree nodes to acquire the relevant clocks and 
> regulators.

How about clocks and regulators for the ancillary devices, such as
encoders, or in the R-Car case, the external composer handled by the
vsp1 driver (in drivers/media/platform/vsp1) ?

This approach may work fine on x86 desktop systems, but for ARM-based
devices, the situation is usually more complex.

> >>> Reading your mail that brought this thread up in my inbox, I think
> >>> you've already hit merge on this, so don't worry about adding a tag in
> >>> that instance, but I think this is ok.
> >>>
> >>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> >>>
> >>>>           rcar_du_of_init(rcar_du_of_table);
> >>>>    
> >>>>           return platform_driver_register(&rcar_du_platform_driver);

-- 
Regards,

Laurent Pinchart