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 BE9722222C247 for ; Fri, 26 Jan 2018 20:06:24 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Jan 2018 20:11:54 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,420,1511856000"; d="scan'208";a="22665442" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga003.jf.intel.com with ESMTP; 26 Jan 2018 20:11:54 -0800 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 26 Jan 2018 20:11:54 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 26 Jan 2018 20:11:53 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.124]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.130]) with mapi id 14.03.0319.002; Sat, 27 Jan 2018 12:11:52 +0800 From: "You, Benjamin" To: Arthur Heymans , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine Thread-Index: AQHTloVEbO9yM5GE4kKb+ZrPyeBPYKOHEbGA Date: Sat, 27 Jan 2018 04:11:51 +0000 Message-ID: References: <20180124105736.14877-1-arthur@aheymans.xyz> <87mv12gvjc.fsf@aheymans.xyz> <87efmdyokj.fsf@aheymans.xyz> In-Reply-To: <87efmdyokj.fsf@aheymans.xyz> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTNlMDk4YmYtNmIzZS00OTUyLWJhNGYtMjk0ZjRmZGNiOWMzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJnUEt5dDB4N3JmOXB5M1RZTWpPak9SdXZib2xNV2NKem95MlRMTDFoUnNiemxMa29RNGxEU2R3UnVSQkZJN3lSIn0= 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: Sat, 27 Jan 2018 04:06:25 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Arthur, I agree with your suggestion that Payload interpret BytesPerScanLine and=20 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 > Cc: edk2-devel@lists.01.org > Subject: Re: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanL= ine >=20 > "You, Benjamin" writes: >=20 > > > > I noticed in coreboot-4.7\src\include\edid.h, there are following comme= nts: > > > > /* 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); > > >=20 > 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: >=20 > edid->bytes_per_line =3D (edid->bytes_per_line + 63) & ~63; This line does not change the value of edid->bytes_per_line since it is=20 already rounded up to 64 by previous calculation in edid.c: edid->bytes_per_line =3D ALIGN_UP(edid->mode.ha * div_round_up(fb_bpp, 8), row_byte_alignment); >=20 > before it gets send to code that sets up the coreboot tables from which > payloads extract this information: >=20 > set_vbe_mode_info_valid(edid, lfb); >=20 > There are also other code paths that don't use src/lib/edid.c to set up > the framebuffer. >=20 > In src/drivers/intel/gma/hires_fb/gma.adb we have: > x_resolution =3D> word32 (fb.Width), > y_resolution =3D> word32 (fb.Height), > bytes_per_line =3D> 4 * word32 (fb.Stride), > >>From the same file, I found: Stride =3D> ((Width_Type (min_h) + 63) / 64) * 64 =20 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=20 alignment. =20 >=20 > > Above calculations derive x_resolution from the roundup value of > > bytes_per_line. In case of 1366 display, it would produce a x_resolutio= n of > > 1376, which is larger than 1366 but satisfies the equation of > > bytes_per_line =3D=3D (HorizontalResolution * (BitsPerPixel / 8) > > >=20 > 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. >=20 > > 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. > > >=20 > well given that other code touches edid_bytes_per_lines, there are many > examples where this is not satisfied. >=20 > > 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 tryi= ng > >> 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 > >> >> Subject: [edk2] [PATCH] CorebootPayloadPkg: Use correct > BytesPerScanLine > >> >> > >> >> From: Arthur Heymans > >> >> > >> >> Fetch BytesPerScanLine from coreboot table to reflect how the actua= l > >> >> framebuffer is set up instead of guessing it from the horizontal > >> >> resolution. > >> >> > >> >> This fixes a garbled display when HorizontalResolution * (BitsPerPi= xel > >> >> / 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> > >> > >> -- > >> Arthur Heymans > > > > Thanks, > > > > - ben >=20 > -- > Arthur Heymans Thanks, - ben