[PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[arthur]

From: Arthur Heymans <arthur@aheymans.xyz>

Fetch BytesPerScanLine from coreboot table to reflect how the actual
framebuffer is set up instead of guessing it from the horizontal
resolution.

This fixes a garbled display when HorizontalResolution * (BitsPerPixel
/ 8) and pFbInfo->BytesPerScanLine don't match.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>

diff --git a/CorebootPayloadPkg/FbGop/FbGop.c b/CorebootPayloadPkg/FbGop/FbGop.c
index 37d6def7f7..6790617033 100644
--- a/CorebootPayloadPkg/FbGop/FbGop.c
+++ b/CorebootPayloadPkg/FbGop/FbGop.c
@@ -822,7 +822,7 @@ FbGopCheckForVbe (
   BitsPerPixel         = pFbInfo->BitsPerPixel;
   HorizontalResolution = pFbInfo->HorizontalResolution;
   VerticalResolution   = pFbInfo->VerticalResolution;
-  BytesPerScanLine     = HorizontalResolution * (BitsPerPixel / 8);
+  BytesPerScanLine     = pFbInfo->BytesPerScanLine;
   
   ModeBuffer = (FB_VIDEO_MODE_DATA *) AllocatePool (
 																						ModeNumber * sizeof (FB_VIDEO_MODE_DATA)
-- 
2.16.1

Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[You, Benjamin]

Hi Arthur,

Could you please give more details about your case that 
HorizontalResolution * (BitsPerPixel / 8) and pFbInfo->BytesPerScanLine 
don't match?

I did a brief search in Coreboot source and found following comments in 
coreboot-4.6\src\lib\edid.c:

  /* In the case of (e.g.) 24 framebuffer bits per pixel, the convention
   * nowadays seems to be to round it up to the nearest reasonable
   * boundary, because otherwise the byte-packing is hideous.
   
So it appears framebuffer BitsPerPixel will likely be 16 or 32, and the 
following statement in the same file calculates:

  edid->x_resolution = edid->bytes_per_line / (fb_bpp / 8);

which results in bytes_per_line (already rounded up to be 32 or 64 byte 
aligned) matching x_resolution * (fb_bpp / 8).

There are cases that even if panel bits_per_pixel is 24, the framebuffer 
bits_per_pixel is still 32, as shown in 
coreboot-4.6\src\drivers\emulation\qemu\bochs.c:

  edid.panel_bits_per_pixel = 24;
  edid_set_framebuffer_bits_per_pixel(&edid, 32, 0);

It would be good if you could help with more details on how the mismatch 
happened in your case as I may have missed something.

Thanks,

- ben

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> arthur@aheymans.xyz
> Sent: Wednesday, January 24, 2018 6:58 PM
> To: edk2-devel@lists.01.org
> Cc: Arthur Heymans <arthur@aheymans.xyz>
> Subject: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine
> 
> From: Arthur Heymans <arthur@aheymans.xyz>
> 
> Fetch BytesPerScanLine from coreboot table to reflect how the actual
> framebuffer is set up instead of guessing it from the horizontal
> resolution.
> 
> This fixes a garbled display when HorizontalResolution * (BitsPerPixel
> / 8) and pFbInfo->BytesPerScanLine don't match.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
> 
> diff --git a/CorebootPayloadPkg/FbGop/FbGop.c
> b/CorebootPayloadPkg/FbGop/FbGop.c
> index 37d6def7f7..6790617033 100644
> --- a/CorebootPayloadPkg/FbGop/FbGop.c
> +++ b/CorebootPayloadPkg/FbGop/FbGop.c
> @@ -822,7 +822,7 @@ FbGopCheckForVbe (
>    BitsPerPixel         = pFbInfo->BitsPerPixel;
>    HorizontalResolution = pFbInfo->HorizontalResolution;
>    VerticalResolution   = pFbInfo->VerticalResolution;
> -  BytesPerScanLine     = HorizontalResolution * (BitsPerPixel / 8);
> +  BytesPerScanLine     = pFbInfo->BytesPerScanLine;
> 
>    ModeBuffer = (FB_VIDEO_MODE_DATA *) AllocatePool (
> 
> 
> 				ModeNumber * sizeof
> (FB_VIDEO_MODE_DATA)
> --
> 2.16.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[Arthur Heymans]

"You, Benjamin" <benjamin.you@intel.com> writes:

> Hi Arthur,
>
> Could you please give more details about your case that 
> HorizontalResolution * (BitsPerPixel / 8) and pFbInfo->BytesPerScanLine 
> don't match?
>

On many devices, notably Intel hardware, the STRIDE needs to be 64 byte
aligned when used in linear memory mode, which coreboot does. STRIDE is
value that used to determine the line to line increment for the
display. So what coreboot does when initializing the hardware to align
(HorizontalResolution * (BitsPerPixel / 8)), 64 bytes up.

> I did a brief search in Coreboot source and found following comments in 
> coreboot-4.6\src\lib\edid.c:
>
>   /* In the case of (e.g.) 24 framebuffer bits per pixel, the convention
>    * nowadays seems to be to round it up to the nearest reasonable
>    * boundary, because otherwise the byte-packing is hideous.
>    
> So it appears framebuffer BitsPerPixel will likely be 16 or 32, and the 
> following statement in the same file calculates:
>
>   edid->x_resolution = edid->bytes_per_line / (fb_bpp / 8);
>
> which results in bytes_per_line (already rounded up to be 32 or 64 byte 
> aligned) matching x_resolution * (fb_bpp / 8).
>
> There are cases that even if panel bits_per_pixel is 24, the framebuffer 
> bits_per_pixel is still 32, as shown in 
> coreboot-4.6\src\drivers\emulation\qemu\bochs.c:
>
>   edid.panel_bits_per_pixel = 24;
>   edid_set_framebuffer_bits_per_pixel(&edid, 32, 0);
>
> It would be good if you could help with more details on how the mismatch 
> happened in your case as I may have missed something.
>

So long story short 'bytes_per_line' reflects how the actual hardware is
set up, while using '(HorizontalResolution * (BitsPerPixel / 8)' is a
guess which is only sometimes correct.

To give an example (on which this was actually a problem):
let's say we have a display 1366 pixel horizontal resolution with 32
bits per pixel.

HorizontalResolution * BitsPerPixel / 8 = 5464

But this value is not 64 byte aligned which the hardware expects so.

aligned value = ((HorizontalResolution * (BitsPerPixel / 8) + 63) & ~63
= 5504

So tianocore should use the value coreboot provides it instead of trying
to compute/guess it.

> Thanks,
>
> - ben
>

I hope this clarifies it.

Arthur

>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> arthur@aheymans.xyz
>> Sent: Wednesday, January 24, 2018 6:58 PM
>> To: edk2-devel@lists.01.org
>> Cc: Arthur Heymans <arthur@aheymans.xyz>
>> Subject: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine
>> 
>> From: Arthur Heymans <arthur@aheymans.xyz>
>> 
>> Fetch BytesPerScanLine from coreboot table to reflect how the actual
>> framebuffer is set up instead of guessing it from the horizontal
>> resolution.
>> 
>> This fixes a garbled display when HorizontalResolution * (BitsPerPixel
>> / 8) and pFbInfo->BytesPerScanLine don't match.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
>> 
>> diff --git a/CorebootPayloadPkg/FbGop/FbGop.c
>> b/CorebootPayloadPkg/FbGop/FbGop.c
>> index 37d6def7f7..6790617033 100644
>> --- a/CorebootPayloadPkg/FbGop/FbGop.c
>> +++ b/CorebootPayloadPkg/FbGop/FbGop.c
>> @@ -822,7 +822,7 @@ FbGopCheckForVbe (
>>    BitsPerPixel         = pFbInfo->BitsPerPixel;
>>    HorizontalResolution = pFbInfo->HorizontalResolution;
>>    VerticalResolution   = pFbInfo->VerticalResolution;
>> -  BytesPerScanLine     = HorizontalResolution * (BitsPerPixel / 8);
>> +  BytesPerScanLine     = pFbInfo->BytesPerScanLine;
>> 
>>    ModeBuffer = (FB_VIDEO_MODE_DATA *) AllocatePool (
>> 
>> 
>> 				ModeNumber * sizeof
>> (FB_VIDEO_MODE_DATA)
>> --
>> 2.16.1
>> 
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
<#secure method=pgpmime mode=sign>

-- 
Arthur Heymans

Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[You, Benjamin]

Hi Arthur,

> -----Original Message-----
> From: Arthur Heymans [mailto:arthur@aheymans.xyz]
> Sent: Thursday, January 25, 2018 5:03 PM
> To: You, Benjamin <benjamin.you@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine
> 
> "You, Benjamin" <benjamin.you@intel.com> writes:
> 
> > Hi Arthur,
> >
> > Could you please give more details about your case that
> > HorizontalResolution * (BitsPerPixel / 8) and pFbInfo->BytesPerScanLine
> > don't match?
> >
> 
> On many devices, notably Intel hardware, the STRIDE needs to be 64 byte
> aligned when used in linear memory mode, which coreboot does. STRIDE is
> value that used to determine the line to line increment for the
> display. So what coreboot does when initializing the hardware to align
> (HorizontalResolution * (BitsPerPixel / 8)), 64 bytes up.
> 
> > I did a brief search in Coreboot source and found following comments in
> > coreboot-4.6\src\lib\edid.c:
> >
> >   /* In the case of (e.g.) 24 framebuffer bits per pixel, the convention
> >    * nowadays seems to be to round it up to the nearest reasonable
> >    * boundary, because otherwise the byte-packing is hideous.
> >
> > So it appears framebuffer BitsPerPixel will likely be 16 or 32, and the
> > following statement in the same file calculates:
> >
> >   edid->x_resolution = edid->bytes_per_line / (fb_bpp / 8);
> >
> > which results in bytes_per_line (already rounded up to be 32 or 64 byte
> > aligned) matching x_resolution * (fb_bpp / 8).
> >
> > There are cases that even if panel bits_per_pixel is 24, the framebuffer
> > bits_per_pixel is still 32, as shown in
> > coreboot-4.6\src\drivers\emulation\qemu\bochs.c:
> >
> >   edid.panel_bits_per_pixel = 24;
> >   edid_set_framebuffer_bits_per_pixel(&edid, 32, 0);
> >
> > It would be good if you could help with more details on how the mismatch
> > happened in your case as I may have missed something.
> >
> 
> So long story short 'bytes_per_line' reflects how the actual hardware is
> set up, while using '(HorizontalResolution * (BitsPerPixel / 8)' is a
> guess which is only sometimes correct.
> 
> To give an example (on which this was actually a problem):
> let's say we have a display 1366 pixel horizontal resolution with 32
> bits per pixel.
> 
> HorizontalResolution * BitsPerPixel / 8 = 5464
> 
> But this value is not 64 byte aligned which the hardware expects so.
> 
> aligned value = ((HorizontalResolution * (BitsPerPixel / 8) + 63) & ~63
> = 5504
>

I noticed in coreboot-4.7\src\include\edid.h, there are following comments:

/* 3 variables needed for coreboot framebuffer.
	 * In most cases, they are the same as the ha
	 * and va variables, but not always, as in the
	 * case of a 1366 wide display.
	 */
	u32 x_resolution;
	u32 y_resolution;
	u32 bytes_per_line;

And in coreboot-4.7\src\lib\edid.c:

	edid->bytes_per_line = ALIGN_UP(edid->mode.ha *
		div_round_up(fb_bpp, 8), row_byte_alignment);
	edid->x_resolution = edid->bytes_per_line / (fb_bpp / 8);

Above calculations derive x_resolution from the roundup value of 
bytes_per_line. In case of 1366 display, it would produce a x_resolution of
1376, which is larger than 1366 but satisfies the equation of 
bytes_per_line == (HorizontalResolution * (BitsPerPixel / 8)

It appears this is what Coreboot produces right now. Not sure if there are 
other cases leading to Coreboot producing framebuffer parameters NOT 
satisfying the above equation.

BTW, do you think the above calculation of x_resolution hides the 
information of display and should be fixed?

> So tianocore should use the value coreboot provides it instead of trying
> to compute/guess it.
> 
> > Thanks,
> >
> > - ben
> >
> 
> I hope this clarifies it.
> 
> Arthur
> 
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> >> arthur@aheymans.xyz
> >> Sent: Wednesday, January 24, 2018 6:58 PM
> >> To: edk2-devel@lists.01.org
> >> Cc: Arthur Heymans <arthur@aheymans.xyz>
> >> Subject: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine
> >>
> >> From: Arthur Heymans <arthur@aheymans.xyz>
> >>
> >> Fetch BytesPerScanLine from coreboot table to reflect how the actual
> >> framebuffer is set up instead of guessing it from the horizontal
> >> resolution.
> >>
> >> This fixes a garbled display when HorizontalResolution * (BitsPerPixel
> >> / 8) and pFbInfo->BytesPerScanLine don't match.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
> >>
> >> diff --git a/CorebootPayloadPkg/FbGop/FbGop.c
> >> b/CorebootPayloadPkg/FbGop/FbGop.c
> >> index 37d6def7f7..6790617033 100644
> >> --- a/CorebootPayloadPkg/FbGop/FbGop.c
> >> +++ b/CorebootPayloadPkg/FbGop/FbGop.c
> >> @@ -822,7 +822,7 @@ FbGopCheckForVbe (
> >>    BitsPerPixel         = pFbInfo->BitsPerPixel;
> >>    HorizontalResolution = pFbInfo->HorizontalResolution;
> >>    VerticalResolution   = pFbInfo->VerticalResolution;
> >> -  BytesPerScanLine     = HorizontalResolution * (BitsPerPixel / 8);
> >> +  BytesPerScanLine     = pFbInfo->BytesPerScanLine;
> >>
> >>    ModeBuffer = (FB_VIDEO_MODE_DATA *) AllocatePool (
> >>
> >>
> >> 				ModeNumber * sizeof
> >> (FB_VIDEO_MODE_DATA)
> >> --
> >> 2.16.1
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
> <#secure method=pgpmime mode=sign>
> 
> --
> Arthur Heymans

Thanks,

- ben

Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[Arthur Heymans]

"You, Benjamin" <benjamin.you@intel.com> writes:

>
> I noticed in coreboot-4.7\src\include\edid.h, there are following comments:
>
> /* 3 variables needed for coreboot framebuffer.
> 	 * In most cases, they are the same as the ha
> 	 * and va variables, but not always, as in the
> 	 * case of a 1366 wide display.
> 	 */
> 	u32 x_resolution;
> 	u32 y_resolution;
> 	u32 bytes_per_line;
>
> And in coreboot-4.7\src\lib\edid.c:
>
> 	edid->bytes_per_line = ALIGN_UP(edid->mode.ha *
> 		div_round_up(fb_bpp, 8), row_byte_alignment);
> 	edid->x_resolution = edid->bytes_per_line / (fb_bpp / 8);
>

This is how x_resolution initially gets set after the EDID is read, but
it is further modified to satisfy the display controllers needs,
e.g. src/northbridge/intel/gm45/gma.c:

edid->bytes_per_line = (edid->bytes_per_line + 63) & ~63;

before it gets send to code that sets up the coreboot tables from which
payloads extract this information:

set_vbe_mode_info_valid(edid, lfb);

There are also other code paths that don't use src/lib/edid.c to set up
the framebuffer.

In src/drivers/intel/gma/hires_fb/gma.adb we have:
            x_resolution         => word32 (fb.Width),
            y_resolution         => word32 (fb.Height),
            bytes_per_line       => 4 * word32 (fb.Stride),


> Above calculations derive x_resolution from the roundup value of 
> bytes_per_line. In case of 1366 display, it would produce a x_resolution of
> 1376, which is larger than 1366 but satisfies the equation of 
> bytes_per_line == (HorizontalResolution * (BitsPerPixel / 8)
>

It is only initialized to that when the EDID is read. The code that
actually sets up the hardware further modifies it or passes on the value
it needs to bytes_per_line.

> It appears this is what Coreboot produces right now. Not sure if there are 
> other cases leading to Coreboot producing framebuffer parameters NOT 
> satisfying the above equation.
>

well given that other code touches edid_bytes_per_lines, there are many
examples where this is not satisfied.

> BTW, do you think the above calculation of x_resolution hides the 
> information of display and should be fixed?
>
>> So tianocore should use the value coreboot provides it instead of trying
>> to compute/guess it.
>> 
>> > Thanks,
>> >
>> > - ben
>> >
>> 
>> I hope this clarifies it.
>> 
>> Arthur
>> 
>> >> -----Original Message-----
>> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> >> arthur@aheymans.xyz
>> >> Sent: Wednesday, January 24, 2018 6:58 PM
>> >> To: edk2-devel@lists.01.org
>> >> Cc: Arthur Heymans <arthur@aheymans.xyz>
>> >> Subject: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine
>> >>
>> >> From: Arthur Heymans <arthur@aheymans.xyz>
>> >>
>> >> Fetch BytesPerScanLine from coreboot table to reflect how the actual
>> >> framebuffer is set up instead of guessing it from the horizontal
>> >> resolution.
>> >>
>> >> This fixes a garbled display when HorizontalResolution * (BitsPerPixel
>> >> / 8) and pFbInfo->BytesPerScanLine don't match.
>> >>
>> >> Contributed-under: TianoCore Contribution Agreement 1.1
>> >> Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
>> >>
>> >> diff --git a/CorebootPayloadPkg/FbGop/FbGop.c
>> >> b/CorebootPayloadPkg/FbGop/FbGop.c
>> >> index 37d6def7f7..6790617033 100644
>> >> --- a/CorebootPayloadPkg/FbGop/FbGop.c
>> >> +++ b/CorebootPayloadPkg/FbGop/FbGop.c
>> >> @@ -822,7 +822,7 @@ FbGopCheckForVbe (
>> >>    BitsPerPixel         = pFbInfo->BitsPerPixel;
>> >>    HorizontalResolution = pFbInfo->HorizontalResolution;
>> >>    VerticalResolution   = pFbInfo->VerticalResolution;
>> >> -  BytesPerScanLine     = HorizontalResolution * (BitsPerPixel / 8);
>> >> +  BytesPerScanLine     = pFbInfo->BytesPerScanLine;
>> >>
>> >>    ModeBuffer = (FB_VIDEO_MODE_DATA *) AllocatePool (
>> >>
>> >>
>> >> 				ModeNumber * sizeof
>> >> (FB_VIDEO_MODE_DATA)
>> >> --
>> >> 2.16.1
>> >>
>> >> _______________________________________________
>> >> edk2-devel mailing list
>> >> edk2-devel@lists.01.org
>> >> https://lists.01.org/mailman/listinfo/edk2-devel
>> <#secure method=pgpmime mode=sign>
>> 
>> --
>> Arthur Heymans
>
> Thanks,
>
> - ben

-- 
Arthur Heymans

Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[You, Benjamin]

Hi Arthur,

I agree with your suggestion that Payload interpret BytesPerScanLine and 
Horizontal Resolution properly such that a 1366 display can be handled well.

The functioning will depend on Coreboot interpreting properly too. However
fixing the Payload will not cause any regression anyway.

I am still not very clear about some cases in Coreboot as below:

> -----Original Message-----
> From: Arthur Heymans [mailto:arthur@aheymans.xyz]
> Sent: Friday, January 26, 2018 5:09 PM
> To: You, Benjamin <benjamin.you@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine
> 
> "You, Benjamin" <benjamin.you@intel.com> writes:
> 
> >
> > I noticed in coreboot-4.7\src\include\edid.h, there are following comments:
> >
> > /* 3 variables needed for coreboot framebuffer.
> > 	 * In most cases, they are the same as the ha
> > 	 * and va variables, but not always, as in the
> > 	 * case of a 1366 wide display.
> > 	 */
> > 	u32 x_resolution;
> > 	u32 y_resolution;
> > 	u32 bytes_per_line;
> >
> > And in coreboot-4.7\src\lib\edid.c:
> >
> > 	edid->bytes_per_line = ALIGN_UP(edid->mode.ha *
> > 		div_round_up(fb_bpp, 8), row_byte_alignment);
> > 	edid->x_resolution = edid->bytes_per_line / (fb_bpp / 8);
> >
> 
> This is how x_resolution initially gets set after the EDID is read, but
> it is further modified to satisfy the display controllers needs,
> e.g. src/northbridge/intel/gm45/gma.c:
> 
> edid->bytes_per_line = (edid->bytes_per_line + 63) & ~63;

This line does not change the value of edid->bytes_per_line since it is 
already rounded up to 64 by previous calculation in edid.c:

  edid->bytes_per_line = ALIGN_UP(edid->mode.ha *
 		div_round_up(fb_bpp, 8), row_byte_alignment);

> 
> before it gets send to code that sets up the coreboot tables from which
> payloads extract this information:
> 
> set_vbe_mode_info_valid(edid, lfb);
> 
> There are also other code paths that don't use src/lib/edid.c to set up
> the framebuffer.
> 
> In src/drivers/intel/gma/hires_fb/gma.adb we have:
>             x_resolution         => word32 (fb.Width),
>             y_resolution         => word32 (fb.Height),
>             bytes_per_line       => 4 * word32 (fb.Stride),
>
>From the same file, I found:
    Stride  => ((Width_Type (min_h) + 63) / 64) * 64
    
This line seems to expand Stride to 64 alignment in the unit of Pixel, not
Byte. I thought line padding is on 64 byte alignment, not on 64 pixel 
alignment.
 
> 
> > Above calculations derive x_resolution from the roundup value of
> > bytes_per_line. In case of 1366 display, it would produce a x_resolution of
> > 1376, which is larger than 1366 but satisfies the equation of
> > bytes_per_line == (HorizontalResolution * (BitsPerPixel / 8)
> >
> 
> It is only initialized to that when the EDID is read. The code that
> actually sets up the hardware further modifies it or passes on the value
> it needs to bytes_per_line.
> 
> > It appears this is what Coreboot produces right now. Not sure if there are
> > other cases leading to Coreboot producing framebuffer parameters NOT
> > satisfying the above equation.
> >
> 
> well given that other code touches edid_bytes_per_lines, there are many
> examples where this is not satisfied.
> 
> > BTW, do you think the above calculation of x_resolution hides the
> > information of display and should be fixed?
> >
> >> So tianocore should use the value coreboot provides it instead of trying
> >> to compute/guess it.
> >>
> >> > Thanks,
> >> >
> >> > - ben
> >> >
> >>
> >> I hope this clarifies it.
> >>
> >> Arthur
> >>
> >> >> -----Original Message-----
> >> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> >> >> arthur@aheymans.xyz
> >> >> Sent: Wednesday, January 24, 2018 6:58 PM
> >> >> To: edk2-devel@lists.01.org
> >> >> Cc: Arthur Heymans <arthur@aheymans.xyz>
> >> >> Subject: [edk2] [PATCH] CorebootPayloadPkg: Use correct
> BytesPerScanLine
> >> >>
> >> >> From: Arthur Heymans <arthur@aheymans.xyz>
> >> >>
> >> >> Fetch BytesPerScanLine from coreboot table to reflect how the actual
> >> >> framebuffer is set up instead of guessing it from the horizontal
> >> >> resolution.
> >> >>
> >> >> This fixes a garbled display when HorizontalResolution * (BitsPerPixel
> >> >> / 8) and pFbInfo->BytesPerScanLine don't match.
> >> >>
> >> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> >> Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
> >> >>
> >> >> diff --git a/CorebootPayloadPkg/FbGop/FbGop.c
> >> >> b/CorebootPayloadPkg/FbGop/FbGop.c
> >> >> index 37d6def7f7..6790617033 100644
> >> >> --- a/CorebootPayloadPkg/FbGop/FbGop.c
> >> >> +++ b/CorebootPayloadPkg/FbGop/FbGop.c
> >> >> @@ -822,7 +822,7 @@ FbGopCheckForVbe (
> >> >>    BitsPerPixel         = pFbInfo->BitsPerPixel;
> >> >>    HorizontalResolution = pFbInfo->HorizontalResolution;
> >> >>    VerticalResolution   = pFbInfo->VerticalResolution;
> >> >> -  BytesPerScanLine     = HorizontalResolution * (BitsPerPixel / 8);
> >> >> +  BytesPerScanLine     = pFbInfo->BytesPerScanLine;
> >> >>
> >> >>    ModeBuffer = (FB_VIDEO_MODE_DATA *) AllocatePool (
> >> >>
> >> >>
> >> >> 				ModeNumber * sizeof
> >> >> (FB_VIDEO_MODE_DATA)
> >> >> --
> >> >> 2.16.1
> >> >>
> >> >> _______________________________________________
> >> >> edk2-devel mailing list
> >> >> edk2-devel@lists.01.org
> >> >> https://lists.01.org/mailman/listinfo/edk2-devel
> >> <#secure method=pgpmime mode=sign>
> >>
> >> --
> >> Arthur Heymans
> >
> > Thanks,
> >
> > - ben
> 
> --
> Arthur Heymans


Thanks,

- ben

Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[Arthur Heymans]

"You, Benjamin" <benjamin.you@intel.com> writes:

> Hi Arthur,
>
> I agree with your suggestion that Payload interpret BytesPerScanLine and 
> Horizontal Resolution properly such that a 1366 display can be handled well.
>
> The functioning will depend on Coreboot interpreting properly too. However
> fixing the Payload will not cause any regression anyway.
>

This is coreboot's interface of telling payloads how the framebuffer was
set up, for the payload to reuse it. The internal implementations in
coreboot really shouldn't matter for payloads. The payload should assume
it is sane.

> I am still not very clear about some cases in Coreboot as below:
>
>> -----Original Message-----
>> From: Arthur Heymans [mailto:arthur@aheymans.xyz]
>> Sent: Friday, January 26, 2018 5:09 PM
>> To: You, Benjamin <benjamin.you@intel.com>
>> Cc: edk2-devel@lists.01.org
>> Subject: Re: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine
>> 
>> "You, Benjamin" <benjamin.you@intel.com> writes:
>> 
>> >
>> > I noticed in coreboot-4.7\src\include\edid.h, there are following comments:
>> >
>> > /* 3 variables needed for coreboot framebuffer.
>> > 	 * In most cases, they are the same as the ha
>> > 	 * and va variables, but not always, as in the
>> > 	 * case of a 1366 wide display.
>> > 	 */
>> > 	u32 x_resolution;
>> > 	u32 y_resolution;
>> > 	u32 bytes_per_line;
>> >
>> > And in coreboot-4.7\src\lib\edid.c:
>> >
>> > 	edid->bytes_per_line = ALIGN_UP(edid->mode.ha *
>> > 		div_round_up(fb_bpp, 8), row_byte_alignment);
>> > 	edid->x_resolution = edid->bytes_per_line / (fb_bpp / 8);
>> >
>> 
>> This is how x_resolution initially gets set after the EDID is read, but
>> it is further modified to satisfy the display controllers needs,
>> e.g. src/northbridge/intel/gm45/gma.c:
>> 
>> edid->bytes_per_line = (edid->bytes_per_line + 63) & ~63;
>
> This line does not change the value of edid->bytes_per_line since it is 
> already rounded up to 64 by previous calculation in edid.c:
>
>   edid->bytes_per_line = ALIGN_UP(edid->mode.ha *
>  		div_round_up(fb_bpp, 8), row_byte_alignment);
>

Could be, but that doesn't really matter for this patch. What is
important is that coreboot has an interface for bytes_per_line for the
framebuffer and that it is the payloads job to use that interface.

>> 
>> before it gets send to code that sets up the coreboot tables from which
>> payloads extract this information:
>> 
>> set_vbe_mode_info_valid(edid, lfb);
>> 
>> There are also other code paths that don't use src/lib/edid.c to set up
>> the framebuffer.
>> 
>> In src/drivers/intel/gma/hires_fb/gma.adb we have:
>>             x_resolution         => word32 (fb.Width),
>>             y_resolution         => word32 (fb.Height),
>>             bytes_per_line       => 4 * word32 (fb.Stride),
>>
> From the same file, I found:
>     Stride  => ((Width_Type (min_h) + 63) / 64) * 64
>     
> This line seems to expand Stride to 64 alignment in the unit of Pixel, not
> Byte. I thought line padding is on 64 byte alignment, not on 64 pixel 
> alignment.
>

"bytes_per_line       => 4 * word32 (fb.Stride)", changes it from pixel to
bytes (32bits per pixel). That Stride is the maximum stride btw. What it
ends up being is something else that depends on the internal of that
library.

>> >> >> -----Original Message-----
>> >> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> >> >> arthur@aheymans.xyz
>> >> >> Sent: Wednesday, January 24, 2018 6:58 PM
>> >> >> To: edk2-devel@lists.01.org
>> >> >> Cc: Arthur Heymans <arthur@aheymans.xyz>
>> >> >> Subject: [edk2] [PATCH] CorebootPayloadPkg: Use correct
>> BytesPerScanLine
>> >> >>
>> >> >> From: Arthur Heymans <arthur@aheymans.xyz>
>> >> >>
>> >> >> Fetch BytesPerScanLine from coreboot table to reflect how the actual
>> >> >> framebuffer is set up instead of guessing it from the horizontal
>> >> >> resolution.
>> >> >>
>> >> >> This fixes a garbled display when HorizontalResolution * (BitsPerPixel
>> >> >> / 8) and pFbInfo->BytesPerScanLine don't match.
>> >> >>
>> >> >> Contributed-under: TianoCore Contribution Agreement 1.1
>> >> >> Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
>> >> >>
>> >> >> diff --git a/CorebootPayloadPkg/FbGop/FbGop.c
>> >> >> b/CorebootPayloadPkg/FbGop/FbGop.c
>> >> >> index 37d6def7f7..6790617033 100644
>> >> >> --- a/CorebootPayloadPkg/FbGop/FbGop.c
>> >> >> +++ b/CorebootPayloadPkg/FbGop/FbGop.c
>> >> >> @@ -822,7 +822,7 @@ FbGopCheckForVbe (
>> >> >>    BitsPerPixel         = pFbInfo->BitsPerPixel;
>> >> >>    HorizontalResolution = pFbInfo->HorizontalResolution;
>> >> >>    VerticalResolution   = pFbInfo->VerticalResolution;
>> >> >> -  BytesPerScanLine     = HorizontalResolution * (BitsPerPixel / 8);
>> >> >> +  BytesPerScanLine     = pFbInfo->BytesPerScanLine;
>> >> >>
>> >> >>    ModeBuffer = (FB_VIDEO_MODE_DATA *) AllocatePool (
>> >> >>
>> >> >>
>> >> >> 				ModeNumber * sizeof
>> >> >> (FB_VIDEO_MODE_DATA)
>> >> >> --
>> >> >> 2.16.1
>> >> >>
>> >> >> _______________________________________________
>> >> >> edk2-devel mailing list
>> >> >> edk2-devel@lists.01.org
>> >> >> https://lists.01.org/mailman/listinfo/edk2-devel
>> >> <#secure method=pgpmime mode=sign>
>> >>
>> >> --
>> >> Arthur Heymans
>> >
>> > Thanks,
>> >
>> > - ben
>> 
>> --
>> Arthur Heymans
>
>
> Thanks,
>
> - ben

==============
Arthur Heymans

Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[Nico Huber]

Hello Ben,

On 27.01.2018 05:11, benjamin.you at intel.com (You, Benjamin) wrote:
> The functioning will depend on Coreboot interpreting properly too. However
> fixing the Payload will not cause any regression anyway.

I'm not sure what you mean by "interpreting properly". The
`bytes_per_line` field has to match the hardware configuration ofc.
But it is never interpreted by coreboot, coreboot is the producer
of that value, never the consumer.

Please don't make this more complicated than it is. The consumer of the
framebuffer configuration (in this case TianoCore) simply has to honor
the `bytes_per_line` field and shouldn't make any assumptions about it
(there is no generic rule how to get from an `x_resolution` to a
`bytes_per_line` value, there are only constraints. otherwise, there
would be no reason to pass `bytes_per_line` at all).

If you'd like, I can add some documentation to `struct lb_framebuffer`
(that's how it's called in coreboot, don't know the name in TianoCore).
The consumer should be only written with that struct (and it's documen-
tation) in mind (i.e. not by looking at coreboot internal code that may
change at any time).

> I am still not very clear about some cases in Coreboot as below:
> 
>> This is how x_resolution initially gets set after the EDID is read, but
>> it is further modified to satisfy the display controllers needs,
>> e.g. src/northbridge/intel/gm45/gma.c:
>>
>> edid->bytes_per_line = (edid->bytes_per_line + 63) & ~63;
> 
> This line does not change the value of edid->bytes_per_line since it is 
> already rounded up to 64 by previous calculation in edid.c:
> 
>   edid->bytes_per_line = ALIGN_UP(edid->mode.ha *
>  		div_round_up(fb_bpp, 8), row_byte_alignment);

The piece of code in gm45/gma.c uses `struct edid` as an interface.
There is no guarantee that this struct run through the code in edid.c,
it could also have been generated somewhere else. But the hardware
covered by gm45/gma.c has this requirement, so it has to be done there.

Moreover the code in edid.c is broken anyway: it aligns x_resolution
up as well, which is wrong (generally. it might be correct for some
hardware but that should be handled in the respective driver).

Please ignore any code surrounding `struct edid` in coreboot, it's
very badly designed and implemented worse.

>> There are also other code paths that don't use src/lib/edid.c to set up
>> the framebuffer.
>>
>> In src/drivers/intel/gma/hires_fb/gma.adb we have:
>>             x_resolution         => word32 (fb.Width),
>>             y_resolution         => word32 (fb.Height),
>>             bytes_per_line       => 4 * word32 (fb.Stride),
>>
>>From the same file, I found:
>     Stride  => ((Width_Type (min_h) + 63) / 64) * 64
>     
> This line seems to expand Stride to 64 alignment in the unit of Pixel, not
> Byte. I thought line padding is on 64 byte alignment, not on 64 pixel 
> alignment.

Yes, you are right. This "over-alignment" was introduced by accident, by
me. I'd like to keep it, though. It's not wrong by any means, and only
wastes a little memory. OTOH, as it's using unusual values while still
being compliant with hardware and the coreboot framebuffer interface,
it's a great measure to discover bugs in consumers of our framebuffer
interface.

Btw. there is much more gfx init code in coreboot for non-Intel hard-
ware. Which may have it's own constraints regarding `bytes_per_pixel`.
The only guarantee for `bytes_per_pixel` and `x_resolution` you get as
a consumer, is that the former is big enough to hold `x_resolution`
pixels.

Nico

Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[You, Benjamin]

Hi Nico,

Thanks for the detailed information. It makes sense. I do like the idea of
documenting the lb_framebuffer.

> The only guarantee for `bytes_per_pixel` (typo? 'bytes_per_scanline') 
> and `x_resolution` you get as a consumer, is that the former is big 
> enough to hold `x_resolution` pixels.

I think it would be good to also document that the consumer is assured that
in framebuffer, all the 'x_resolution' pixels are aligned at the beginning
of each scanline, and the extra bytes are always padded after the 
'x_resolution' pixels in the scanline. Would this be true with existing
graphics devices? (I am not expert in this area so I'd like to confirm.)

> -----Original Message-----
> From: Nico Huber [mailto:nico.h@gmx.de]
> Sent: Saturday, January 27, 2018 10:14 PM
> To: You, Benjamin <benjamin.you@intel.com>
> Cc: Arthur Heymans <arthur@aheymans.xyz>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine
> 
> Hello Ben,
> 
> On 27.01.2018 05:11, benjamin.you at intel.com (You, Benjamin) wrote:
> > The functioning will depend on Coreboot interpreting properly too. However
> > fixing the Payload will not cause any regression anyway.
> 
> I'm not sure what you mean by "interpreting properly". The
> `bytes_per_line` field has to match the hardware configuration ofc.
> But it is never interpreted by coreboot, coreboot is the producer
> of that value, never the consumer.
> 
> Please don't make this more complicated than it is. The consumer of the
> framebuffer configuration (in this case TianoCore) simply has to honor
> the `bytes_per_line` field and shouldn't make any assumptions about it
> (there is no generic rule how to get from an `x_resolution` to a
> `bytes_per_line` value, there are only constraints. otherwise, there
> would be no reason to pass `bytes_per_line` at all).
> 
> If you'd like, I can add some documentation to `struct lb_framebuffer`
> (that's how it's called in coreboot, don't know the name in TianoCore).
> The consumer should be only written with that struct (and it's documen-
> tation) in mind (i.e. not by looking at coreboot internal code that may
> change at any time).
> 
> > I am still not very clear about some cases in Coreboot as below:
> >
> >> This is how x_resolution initially gets set after the EDID is read, but
> >> it is further modified to satisfy the display controllers needs,
> >> e.g. src/northbridge/intel/gm45/gma.c:
> >>
> >> edid->bytes_per_line = (edid->bytes_per_line + 63) & ~63;
> >
> > This line does not change the value of edid->bytes_per_line since it is
> > already rounded up to 64 by previous calculation in edid.c:
> >
> >   edid->bytes_per_line = ALIGN_UP(edid->mode.ha *
> >  		div_round_up(fb_bpp, 8), row_byte_alignment);
> 
> The piece of code in gm45/gma.c uses `struct edid` as an interface.
> There is no guarantee that this struct run through the code in edid.c,
> it could also have been generated somewhere else. But the hardware
> covered by gm45/gma.c has this requirement, so it has to be done there.
> 
> Moreover the code in edid.c is broken anyway: it aligns x_resolution
> up as well, which is wrong (generally. it might be correct for some
> hardware but that should be handled in the respective driver).
> 
> Please ignore any code surrounding `struct edid` in coreboot, it's
> very badly designed and implemented worse.
> 
> >> There are also other code paths that don't use src/lib/edid.c to set up
> >> the framebuffer.
> >>
> >> In src/drivers/intel/gma/hires_fb/gma.adb we have:
> >>             x_resolution         => word32 (fb.Width),
> >>             y_resolution         => word32 (fb.Height),
> >>             bytes_per_line       => 4 * word32 (fb.Stride),
> >>
> >>From the same file, I found:
> >     Stride  => ((Width_Type (min_h) + 63) / 64) * 64
> >
> > This line seems to expand Stride to 64 alignment in the unit of Pixel, not
> > Byte. I thought line padding is on 64 byte alignment, not on 64 pixel
> > alignment.
> 
> Yes, you are right. This "over-alignment" was introduced by accident, by
> me. I'd like to keep it, though. It's not wrong by any means, and only
> wastes a little memory. OTOH, as it's using unusual values while still
> being compliant with hardware and the coreboot framebuffer interface,
> it's a great measure to discover bugs in consumers of our framebuffer
> interface.
> 
> Btw. there is much more gfx init code in coreboot for non-Intel hard-
> ware. Which may have it's own constraints regarding `bytes_per_pixel`.
> The only guarantee for `bytes_per_pixel` and `x_resolution` you get as
> a consumer, is that the former is big enough to hold `x_resolution`
> pixels.
> 
> Nico

Thanks,

- ben

Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[Nico Huber]

Hi Ben,

On 28.01.2018 09:49, You, Benjamin wrote:
> Hi Nico,
> 
> Thanks for the detailed information. It makes sense. I do like the idea of
> documenting the lb_framebuffer.
> 
>> The only guarantee for `bytes_per_pixel` (typo? 'bytes_per_scanline') 

yes, typo.

>> and `x_resolution` you get as a consumer, is that the former is big 
>> enough to hold `x_resolution` pixels.
> 
> I think it would be good to also document that the consumer is assured that
> in framebuffer, all the 'x_resolution' pixels are aligned at the beginning
> of each scanline, and the extra bytes are always padded after the 
> 'x_resolution' pixels in the scanline. Would this be true with existing
> graphics devices? (I am not expert in this area so I'd like to confirm.)

Yes, that's true. I've started to document the whole thing now:

https://review.coreboot.org/#/c/coreboot/+/23466/4/src/commonlib/include/commonlib/coreboot_tables.h@214

The same applies in principle to EFI_GRAPHICS_OUTPUT_MODE_INFORMATION.
This framebuffer layout is very common.

Nico

Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[You, Benjamin]

Hi Nico,

Thanks for the documentation that is very clear.

Thanks,

- ben

> -----Original Message-----
> From: Nico Huber [mailto:nico.h@gmx.de]
> Sent: Sunday, January 28, 2018 10:34 PM
> To: You, Benjamin <benjamin.you@intel.com>; edk2-devel@lists.01.org
> Cc: Arthur Heymans <arthur@aheymans.xyz>
> Subject: Re: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine
> 
> Hi Ben,
> 
> On 28.01.2018 09:49, You, Benjamin wrote:
> > Hi Nico,
> >
> > Thanks for the detailed information. It makes sense. I do like the idea of
> > documenting the lb_framebuffer.
> >
> >> The only guarantee for `bytes_per_pixel` (typo? 'bytes_per_scanline')
> 
> yes, typo.
> 
> >> and `x_resolution` you get as a consumer, is that the former is big
> >> enough to hold `x_resolution` pixels.
> >
> > I think it would be good to also document that the consumer is assured that
> > in framebuffer, all the 'x_resolution' pixels are aligned at the beginning
> > of each scanline, and the extra bytes are always padded after the
> > 'x_resolution' pixels in the scanline. Would this be true with existing
> > graphics devices? (I am not expert in this area so I'd like to confirm.)
> 
> Yes, that's true. I've started to document the whole thing now:
> 
> https://review.coreboot.org/#/c/coreboot/+/23466/4/src/commonlib/include/
> commonlib/coreboot_tables.h@214
> 
> The same applies in principle to EFI_GRAPHICS_OUTPUT_MODE_INFORMATION.
> This framebuffer layout is very common.
> 
> Nico

Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[You, Benjamin]

Hi Arthur,

There is a related fix in the same file (FbGop.c):

Line 896: 

FbGopPrivate->GraphicsOutput.Mode->Info->PixelsPerScanLine 
  = HorizontalResolution; 

must be changed to:

FbGopPrivate->GraphicsOutput.Mode->Info->PixelsPerScanLine 
= BytesPerScanLine / (BitsPerPixel / 8);

I will submit a separate patch for this.

Thanks,

- ben

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> arthur@aheymans.xyz
> Sent: Wednesday, January 24, 2018 6:58 PM
> To: edk2-devel@lists.01.org
> Cc: Arthur Heymans <arthur@aheymans.xyz>
> Subject: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine
> 
> From: Arthur Heymans <arthur@aheymans.xyz>
> 
> Fetch BytesPerScanLine from coreboot table to reflect how the actual
> framebuffer is set up instead of guessing it from the horizontal
> resolution.
> 
> This fixes a garbled display when HorizontalResolution * (BitsPerPixel
> / 8) and pFbInfo->BytesPerScanLine don't match.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
> 
> diff --git a/CorebootPayloadPkg/FbGop/FbGop.c
> b/CorebootPayloadPkg/FbGop/FbGop.c
> index 37d6def7f7..6790617033 100644
> --- a/CorebootPayloadPkg/FbGop/FbGop.c
> +++ b/CorebootPayloadPkg/FbGop/FbGop.c
> @@ -822,7 +822,7 @@ FbGopCheckForVbe (
>    BitsPerPixel         = pFbInfo->BitsPerPixel;
>    HorizontalResolution = pFbInfo->HorizontalResolution;
>    VerticalResolution   = pFbInfo->VerticalResolution;
> -  BytesPerScanLine     = HorizontalResolution * (BitsPerPixel / 8);
> +  BytesPerScanLine     = pFbInfo->BytesPerScanLine;
> 
>    ModeBuffer = (FB_VIDEO_MODE_DATA *) AllocatePool (
> 
> 
> 				ModeNumber * sizeof
> (FB_VIDEO_MODE_DATA)
> --
> 2.16.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[You, Benjamin]

Reviewed-by: Benjamin You <benjamin.you@intel.com>

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> arthur@aheymans.xyz
> Sent: Wednesday, January 24, 2018 6:58 PM
> To: edk2-devel@lists.01.org
> Cc: Arthur Heymans <arthur@aheymans.xyz>
> Subject: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine
> 
> From: Arthur Heymans <arthur@aheymans.xyz>
> 
> Fetch BytesPerScanLine from coreboot table to reflect how the actual
> framebuffer is set up instead of guessing it from the horizontal
> resolution.
> 
> This fixes a garbled display when HorizontalResolution * (BitsPerPixel
> / 8) and pFbInfo->BytesPerScanLine don't match.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Arthur Heymans <arthur@aheymans.xyz>
> 
> diff --git a/CorebootPayloadPkg/FbGop/FbGop.c
> b/CorebootPayloadPkg/FbGop/FbGop.c
> index 37d6def7f7..6790617033 100644
> --- a/CorebootPayloadPkg/FbGop/FbGop.c
> +++ b/CorebootPayloadPkg/FbGop/FbGop.c
> @@ -822,7 +822,7 @@ FbGopCheckForVbe (
>    BitsPerPixel         = pFbInfo->BitsPerPixel;
>    HorizontalResolution = pFbInfo->HorizontalResolution;
>    VerticalResolution   = pFbInfo->VerticalResolution;
> -  BytesPerScanLine     = HorizontalResolution * (BitsPerPixel / 8);
> +  BytesPerScanLine     = pFbInfo->BytesPerScanLine;
> 
>    ModeBuffer = (FB_VIDEO_MODE_DATA *) AllocatePool (
> 
> 
> 				ModeNumber * sizeof
> (FB_VIDEO_MODE_DATA)
> --
> 2.16.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel