public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
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: Sat, 27 Jan 2018 11:17:18 +0100	[thread overview]
Message-ID: <87607ny5ap.fsf@aheymans.xyz> (raw)
In-Reply-To: <E748835C6D8DB54B8E8AF33091ECC57C62145E71@shsmsx102.ccr.corp.intel.com> (Benjamin You's message of "Sat, 27 Jan 2018 04:11:51 +0000")

"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


  reply	other threads:[~2018-02-08 18:52 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
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 [this message]
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=87607ny5ap.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