2022-05-23 06:59:57

by Huacai Chen

[permalink] [raw]
Subject: Re: [PATCH V11 09/22] LoongArch: Add boot and setup routines

Hi, Javier,

On Fri, May 20, 2022 at 5:41 PM Javier Martinez Canillas
<[email protected]> wrote:
>
> Hello Ard and Huacai,
>
> On 5/20/22 11:17, Ard Biesheuvel wrote:
>
> [snip]
>
> >> +
> >> +static int __init register_gop_device(void)
> >> +{
> >> + void *pd;
> >> +
> >> + if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
> >> + return 0;
> >> + pd = platform_device_register_data(NULL, "efi-framebuffer", 0,
> >> + &screen_info, sizeof(screen_info));
> >> + return PTR_ERR_OR_ZERO(pd);
> >> +}
> >> +subsys_initcall(register_gop_device);
> >
> > Not sure this is now the correct way to do this - cc'ing Javier.
> >
>
> Is not the correct way to do it indeed, that can just be dropped.
>
> We have unified now all the system framebuffer platform device
> registration under drivers/firmware/sysfb.c (and the EFI quirks
> if needed under drivers/firmware/efi/sysfb_efi.c).
>
> So the only thing that a platform should do, is to enable the
> the CONFIG_SYSFB config option. The screen_info should be set
> correctly from the EFI GOP, but it seems that's already working
> since you were already using it in register_gop_device().
>
> But also, the "efi-framebuffer" platform device matches against
> the legacy efifb fbdev driver. And now there's a simpledrm driver
> that is also able to use the firmware-provided framebuffer.
>
> You can enable that driver with CONFIG_DRM_SIMPLEDRM.
>
> That driver though doesn't match against "efi-framebuffer" but with
> a "simple-framebuffer", to make sysfb register that instead of the
> "efi-framebuffer" device, you need to set CONFIG_SYSFB_SIMPLEFB too.
>
> If for some reasons you need to provide a fbdev interface to the
> user-space, you can enable CONFIG_DRM_FBDEV_EMULATION to have that.
>
> In summary, just enable the following to use the firmware framebuffer:
>
> CONFIG_DRM_SIMPLEDRM=y
> CONFIG_DRM_FBDEV_EMULATION=y
> CONFIG_SYSFB=y
> CONFIG_SYSFB_SIMPLEFB=y
Thank you very much, since 5.15 sysfb_init() do all things of
register_gop_device(), so register_gop_device() here can be removed.
But there is another small problem: if simpledrm or efifb driver is
built-in, there is no display. The reason is both sysfb_init() and
display driver are in the same initcall level (device_initcall()).
From the comments I know that sysfb_init() should after PCI
enumeration which is in subsys_initcall(), so, could we make
sysfb_init() to be a subsys_initcall_sync()?

Huacai
>
> --
> Best regards,
>
> Javier Martinez Canillas
> Linux Engineering
> Red Hat
>


2022-05-23 07:03:02

by Javier Martinez Canillas

[permalink] [raw]
Subject: Re: [PATCH V11 09/22] LoongArch: Add boot and setup routines

Hello Huacai,

On 5/20/22 16:09, Huacai Chen wrote:

[snip]

>>
>> In summary, just enable the following to use the firmware framebuffer:
>>
>> CONFIG_DRM_SIMPLEDRM=y
>> CONFIG_DRM_FBDEV_EMULATION=y
>> CONFIG_SYSFB=y
>> CONFIG_SYSFB_SIMPLEFB=y
> Thank you very much, since 5.15 sysfb_init() do all things of
> register_gop_device(), so register_gop_device() here can be removed.

Correct.

> But there is another small problem: if simpledrm or efifb driver is
> built-in, there is no display. The reason is both sysfb_init() and
> display driver are in the same initcall level (device_initcall()).
> From the comments I know that sysfb_init() should after PCI
> enumeration which is in subsys_initcall(), so, could we make
> sysfb_init() to be a subsys_initcall_sync()?
>

I don't understand why we would need that... it shouldn't matter the order
in which the driver's init and sysfb_init() are done. If the driver's init
is executed first, then the driver will be registered and later when the
sysfb_init() registers the device, it will match the driver and probe.

Conversely, if the sysfb_init() is executed first then the platform device
will be registered and latter when the driver's init register the driver
this will match the already registered device.

In other words, it will be handled by the Linux device model and we should
not attempt to hand pick the initcall level for each. That's quite fragile.

Or am I missing something here ?

--
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat