From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.31; helo=mga06.intel.com; envelope-from=benjamin.you@intel.com; receiver=edk2-devel@lists.01.org Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D8D302222C22F for ; Fri, 26 Jan 2018 00:34:52 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Jan 2018 00:40:22 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,415,1511856000"; d="scan'208";a="12761228" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga007.jf.intel.com with ESMTP; 26 Jan 2018 00:40:22 -0800 Received: from fmsmsx155.amr.corp.intel.com (10.18.116.71) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 26 Jan 2018 00:40:16 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 26 Jan 2018 00:40:16 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.124]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.192]) with mapi id 14.03.0319.002; Fri, 26 Jan 2018 16:40:14 +0800 From: "You, Benjamin" To: Arthur Heymans , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine Thread-Index: AQHTlbtXbO9yM5GE4kKb+ZrPyeBPYKOF0yOQ Date: Fri, 26 Jan 2018 08:40:14 +0000 Message-ID: References: <20180124105736.14877-1-arthur@aheymans.xyz> <87mv12gvjc.fsf@aheymans.xyz> In-Reply-To: <87mv12gvjc.fsf@aheymans.xyz> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOThmNGY1YTUtOTMxNS00N2I0LTk1NDktYWZjZDJkMjVlN2ExIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJXZFdcL3BOZjk0Vm4zMnRyc2YwcFU3SkFmRUJuNW8rZksrVmh6dFYrTmd2bnppamZKSlZXcjJDejRkT2dxNTJrbyJ9 dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 26 Jan 2018 08:34:53 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Arthur, > -----Original Message----- > From: Arthur Heymans [mailto:arthur@aheymans.xyz] > Sent: Thursday, January 25, 2018 5:03 PM > To: You, Benjamin > Cc: edk2-devel@lists.01.org > Subject: Re: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanL= ine >=20 > "You, Benjamin" writes: >=20 > > Hi Arthur, > > > > Could you please give more details about your case that > > HorizontalResolution * (BitsPerPixel / 8) and pFbInfo->BytesPerScanLine > > don't match? > > >=20 > 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. >=20 > > 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 conventio= n > > * 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 =3D 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 framebuffe= r > > bits_per_pixel is still 32, as shown in > > coreboot-4.6\src\drivers\emulation\qemu\bochs.c: > > > > edid.panel_bits_per_pixel =3D 24; > > edid_set_framebuffer_bits_per_pixel(&edid, 32, 0); > > > > It would be good if you could help with more details on how the mismatc= h > > happened in your case as I may have missed something. > > >=20 > 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. >=20 > 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. >=20 > HorizontalResolution * BitsPerPixel / 8 =3D 5464 >=20 > But this value is not 64 byte aligned which the hardware expects so. >=20 > aligned value =3D ((HorizontalResolution * (BitsPerPixel / 8) + 63) & ~63 > =3D 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 =3D ALIGN_UP(edid->mode.ha * div_round_up(fb_bpp, 8), row_byte_alignment); edid->x_resolution =3D edid->bytes_per_line / (fb_bpp / 8); Above calculations derive x_resolution from the roundup value of=20 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=20 bytes_per_line =3D=3D (HorizontalResolution * (BitsPerPixel / 8) It appears this is what Coreboot produces right now. Not sure if there are= =20 other cases leading to Coreboot producing framebuffer parameters NOT=20 satisfying the above equation. BTW, do you think the above calculation of x_resolution hides the=20 information of display and should be fixed? > So tianocore should use the value coreboot provides it instead of trying > to compute/guess it. >=20 > > Thanks, > > > > - ben > > >=20 > I hope this clarifies it. >=20 > Arthur >=20 > >> -----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 > >> Subject: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLi= ne > >> > >> From: Arthur Heymans > >> > >> 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 > >> > >> 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 =3D pFbInfo->BitsPerPixel; > >> HorizontalResolution =3D pFbInfo->HorizontalResolution; > >> VerticalResolution =3D pFbInfo->VerticalResolution; > >> - BytesPerScanLine =3D HorizontalResolution * (BitsPerPixel / 8); > >> + BytesPerScanLine =3D pFbInfo->BytesPerScanLine; > >> > >> ModeBuffer =3D (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=3Dpgpmime mode=3Dsign> >=20 > -- > Arthur Heymans Thanks, - ben