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

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx>
> ---
>  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.

Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat