Re: [PATCH] firmware/sysfb: Fix wrong stride when bits-per-pixel is calculated

[Javier Martinez Canillas]

pa@xxxxxxxxx (Pierre Asselin) writes:

> After careful reading of the comments in f35cd3fa7729, would this
> be an appropriate fix ?  Does it still address all the issues raised
> by Thomas ?
>
> (not tested)
>
> diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
> index 82c64cb9f531..9f5299d54732 100644
> --- a/drivers/firmware/sysfb_simplefb.c
> +++ b/drivers/firmware/sysfb_simplefb.c
> @@ -56,10 +56,11 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
>  	 * don't specify alpha channels.
>  	 */
>  	if (si->lfb_depth > 8) {
> -		bits_per_pixel = max(max3(si->red_size + si->red_pos,
> +		bits_per_pixel = max3(max3(si->red_size + si->red_pos,
>  					  si->green_size + si->green_pos,
>  					  si->blue_size + si->blue_pos),
> -				     si->rsvd_size + si->rsvd_pos);
> +				     si->rsvd_size + si->rsvd_pos,
> +				     si->lfb_depth);

I would defer to Thomas but personally I don't like it. Seems to me that
this is getting too complicated just to workaround buggy BIOS that are not
reporting consistent information about their firmware-provided framebuffer.

I would either trust the pixel channel information (what Thomas patch did)
+ my patch to calculate the stride (since we can't trust the line lenght
which is based on the reported depth) + a DMI table for broken machines.

But that will only work if your systems are the exception and not a common
issue, otherwise then Thomas' approach won't work if there are too many
buggy BIOS out there.

Another option is to do the opposite, not calculate a BPP using the pixel
and then use that value to calculate a new stride, but instead assume that
the lfb_linelength is correct and use that to calculate the BPP.

Something like the following patch, which should also fix your regression
and may be enough to address Thomas' concerns of inconsistent depths too?