Re: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine
"You, Benjamin" <benjamin@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:art...@aheymans.xyz] >> Sent: Friday, January 26, 2018 5:09 PM >> To: You, Benjamin <benjamin@intel.com> >> Cc: edk2-devel@lists.01.org >> Subject: Re: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine >> >> "You, Benjamin" <benjamin@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-boun...@lists.01.org] On Behalf Of >> >> >> art...@aheymans.xyz >> >> >> Sent: Wednesday, January 24, 2018 6:58 PM >> >> >> To: edk2-devel@lists.01.org >> >> >> Cc: Arthur Heymans <art...@aheymans.xyz> >> >> >> Subject: [edk2] [PATCH] CorebootPayloadPkg: Use correct >> BytesPerScanLine >> >> >> >> >> >> From: Arthur Heymans <art...@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 Ag
Re: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine
"You, Benjamin" <benjamin@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(, 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-boun...@lists.01.org] On Behalf Of >> art...@aheymans.xyz >> Sent: Wednesday, January 24, 2018 6:58 PM >> To: edk2-devel@lists.01.org >> Cc: Arthur Heymans <art...@aheymans.xyz> >> Subject: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine >> >> From: Arthur Heymans <art...@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 <art...@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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
