Re: [PATCH 3/5] efi/libstub: gop: Initialize screen_info in helper function

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 24.10.25 um 11:53 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
>
>> Move initialization of screen_info into a single helper function.
>> Frees up space in the main setup helper for adding EDID support.
>> No functional changes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/firmware/efi/libstub/gop.c | 76 +++++++++++++-----------------
>>   1 file changed, 33 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
>> index fd32be8dd146..02459ef0f18c 100644
>> --- a/drivers/firmware/efi/libstub/gop.c
>> +++ b/drivers/firmware/efi/libstub/gop.c
>> @@ -367,24 +367,31 @@ static void find_bits(u32 mask, u8 *pos, u8 *size)
>>   	*size = __fls(mask) - *pos + 1;
>>   }
>>   
>> -static void
>> -setup_pixel_info(struct screen_info *si, u32 pixels_per_scan_line,
>> -		 efi_pixel_bitmask_t pixel_info, int pixel_format)
>> +static void setup_screen_info(struct screen_info *si, const efi_graphics_output_protocol_t *gop)
>>   {
>> -	if (pixel_format == PIXEL_BIT_MASK) {
>> -		find_bits(pixel_info.red_mask,
>> -			  &si->red_pos, &si->red_size);
>> -		find_bits(pixel_info.green_mask,
>> -			  &si->green_pos, &si->green_size);
>> -		find_bits(pixel_info.blue_mask,
>> -			  &si->blue_pos, &si->blue_size);
>> -		find_bits(pixel_info.reserved_mask,
>> -			  &si->rsvd_pos, &si->rsvd_size);
>> -		si->lfb_depth = si->red_size + si->green_size +
>> -			si->blue_size + si->rsvd_size;
>> -		si->lfb_linelength = (pixels_per_scan_line * si->lfb_depth) / 8;
>> +	const efi_graphics_output_protocol_mode_t *mode = efi_table_attr(gop, mode);
>> +	const efi_graphics_output_mode_info_t *info = efi_table_attr(mode, info);
>> +
>> +	si->orig_video_isVGA = VIDEO_TYPE_EFI;
>> +
> Not related with your patch but I dislike so much the name of this field,
> since it started as a "is VGA?" bool and ended being an enum afterwards.
>
> But I beleve we discussed this already and decided that it would be too
> much churn to change it at this point.

That's why we need helpers for decoding, such as [1]. The kernel only 
initializes screen_info to VGAC, EFI and VLFB. We could add initializer 
functions for these cases.

[1] 
https://elixir.bootlin.com/linux/v6.17.8/source/include/linux/screen_info.h#L92

Best regards
Thomas

>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)