Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

["You, Benjamin" <benjamin.you@intel.com>]

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