From: Arthur Heymans <arthur@aheymans.xyz>
To: "You\, Benjamin" <benjamin.you@intel.com>
Cc: "edk2-devel\@lists.01.org" <edk2-devel@lists.01.org>
Subject: Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine
Date: Thu, 25 Jan 2018 10:03:19 +0100 [thread overview]
Message-ID: <87mv12gvjc.fsf@aheymans.xyz> (raw)
In-Reply-To: <E748835C6D8DB54B8E8AF33091ECC57C6213D4F3@SHSMSX103.ccr.corp.intel.com> (Benjamin You's message of "Thu, 25 Jan 2018 05:34:47 +0000")
"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
next prev parent reply other threads:[~2018-01-25 8:57 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-24 10:57 [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine arthur
2018-01-25 5:34 ` You, Benjamin
2018-01-25 9:03 ` Arthur Heymans [this message]
2018-01-26 8:40 ` You, Benjamin
2018-01-26 9:08 ` Arthur Heymans
2018-01-27 4:11 ` You, Benjamin
2018-01-27 10:17 ` Arthur Heymans
2018-01-27 14:14 ` Nico Huber
2018-01-28 8:49 ` You, Benjamin
2018-01-28 14:33 ` Nico Huber
2018-01-29 1:09 ` You, Benjamin
2018-01-29 5:36 ` You, Benjamin
2018-01-30 6:15 ` You, Benjamin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87mv12gvjc.fsf@aheymans.xyz \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox