Thomas Zimmermann <tzimmermann@xxxxxxx> writes:
Hello Thomas,
> Hi Javier,
>
> thanks for the patch.
>
Thanks for your feedback.
> Am 12.09.24 um 18:33 schrieb Javier Martinez Canillas:
>> Julius Werner <jwerner@xxxxxxxxxxxx> writes:
[...]
>> ---
>> drivers/firmware/google/framebuffer-coreboot.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
>> index daadd71d8ddd..4e50da17cd7e 100644
>> --- a/drivers/firmware/google/framebuffer-coreboot.c
>> +++ b/drivers/firmware/google/framebuffer-coreboot.c
>> @@ -15,6 +15,7 @@
>> #include <linux/module.h>
>> #include <linux/platform_data/simplefb.h>
>> #include <linux/platform_device.h>
>> +#include <linux/screen_info.h>
>>
>> #include "coreboot_table.h"
>>
>> @@ -27,6 +28,7 @@ static int framebuffer_probe(struct coreboot_device *dev)
>> int i;
>> u32 length;
>> struct lb_framebuffer *fb = &dev->framebuffer;
>> + struct screen_info *si = &screen_info;
>
> Probably 'const'.
>
Ok.
>> struct platform_device *pdev;
>> struct resource res;
>> struct simplefb_platform_data pdata = {
>> @@ -36,6 +38,20 @@ static int framebuffer_probe(struct coreboot_device *dev)
>> .format = NULL,
>> };
>>
>> + /*
>> + * If the global screen_info data has been filled, the Generic
>> + * System Framebuffers (sysfb) will already register a platform
>> + * and pass the screen_info as platform_data to a driver that
>> + * could scan-out using the system provided framebuffer.
>> + *
>> + * On Coreboot systems, the advertise LB_TAG_FRAMEBUFFER entry
>> + * in the Coreboot table should only be used if the payload did
>> + * not set video mode info and passed it to the Linux kernel.
>> + */
>> + if (si->orig_video_isVGA == VIDEO_TYPE_VLFB ||
>> + si->orig_video_isVGA == VIDEO_TYPE_EFI)
>
> Rather call screen_info_video_type(si) [1] to get the type. If it
Indeed. I missed that helper, I'll change it.
> returns 0, the screen_info is unset and the corebios code can handle the
> framebuffer. In any other case, the framebuffer went through a
> bootloader, which might have modified it. This also handles awkward
> cases, such as if the bootloader programs a VGA text mode.
>
> [1]
> https://elixir.bootlin.com/linux/v6.10.10/source/include/linux/screen_info.h#L92
>
> With these changes:
>
> Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
>
Thanks. I'll wait for others in this thread to comment and if all agree
with the solution, I'll post a proper patch (addressing your comments).
> Best regards
> Thomas
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
