From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=212.227.15.15; helo=mout.gmx.net; envelope-from=nico.h@gmx.de; receiver=edk2-devel@lists.01.org Received: from mout.gmx.net (mout.gmx.net [212.227.15.15]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2F9952222C227 for ; Sat, 27 Jan 2018 06:08:47 -0800 (PST) Received: from [192.168.4.129] ([130.180.109.134]) by mail.gmx.com (mrgmx003 [212.227.17.190]) with ESMTPSA (Nemesis) id 0MdKkd-1eOjbj25wp-00IWH8; Sat, 27 Jan 2018 15:14:12 +0100 To: "You, Benjamin" References: <20180124105736.14877-1-arthur@aheymans.xyz> <87mv12gvjc.fsf@aheymans.xyz> <87efmdyokj.fsf@aheymans.xyz> Cc: Arthur Heymans , edk2-devel@lists.01.org From: Nico Huber Message-ID: Date: Sat, 27 Jan 2018 15:14:10 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: X-Provags-ID: V03:K0:Fpi8OBGh/cqtrzF9Si6d/zCNfCWVXuBB9VxJKiXWh5ib6E3NMH4 Wps9T66T0JaRskoHo5//xKAp6nFN4ebLq9+1G11Rd2Prx33o3HX2Qz13TfHqBdLjeOWRif1 1SHFSnh0ERTUNBl9eGi1M7lamFIn/ak1szLKAnODEQwewHHP9lG/FKEE5suY2Y4O9x4qJO4 DTmnwhMEDeaUVW0x6dS+w== X-UI-Out-Filterresults: notjunk:1;V01:K0:iirRy8yPkQM=:Yh7Xb88lIU2rGgUeEqrEaP V1FqKF2Awx1nmy9NTfr9dxn9xpwuNxearBM9tysXzXViajyKWiDiUT3IHDwZ4FHkK7P/iYe28 71bdfD2wXptRYfnYM8K/19dPZ7Lm4jdN2lFGdr9dirImyQspmhhT5+AmP8PZKQ6ygMxdcchSS 9b2F5rYWv2J4CLQZIKo+Uw/Q7kxKFeoQQXPQvGxI/izjz+MgWUd1ylqmO1uv4OjFW0C5b/623 puU8x3Z7g77ze7hTvZKr6mbMHSjB6JTbjt2kq82oKcKxsK0H3Kjb7QQVSUr0ZHt1EexZtFTo8 qtdd2+oRJMZOE5KC+ghGMKqMbFxbDWbbI9tJukI5eFS7hui5TOvHL8XmqzLtT0rtz3eqzBr0J zy1t7PX03LCaFT4X6ptBa1L+JtGpgWYfpsHNjCnQstoZ0JZ44HTkuiXWlNtvbqjRuHTayYpkO OzP5z9jtg+0X1eXNS4dW6/PJX6ED/YFKQmaCGv04dSr9NYxpbFrulfwJmtlXoPpgfgENjhSxg WWr7ILprrgWSffsFF76v6mdQ7SjbtqmbGI6onABJE8Yx9Bf2z8c4HAaPQD5k5xusSVJDFr0hl IEBVtWEqMhGOKJCLLkQ4EYkJIKLRV+Q0XiWnaoq/vuYnccmkwY94IP3KapD3D+SLMXM9ozHhI IUj1zT6JgDYtlvS/JmqbbvbzxajvTKV0AHaPwiPz5cAH4BliaHgFyq38kKYq3LT29Q3ZFdxuY zyBDqvzYTHQPLn/Kywad4SvQgMXqQoA62tF/J9PGQ0/f7MiMDHhkzwsypjdGwVCLALv/DR1pL 5Lgz9yVA93DzIjVfRtpeh1LpQhra7UxyPB82cW3a0p1ANZ8yTw= X-Mailman-Approved-At: Sat, 27 Jan 2018 08:59:22 -0800 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 14:08:49 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hello Ben, On 27.01.2018 05:11, benjamin.you at intel.com (You, Benjamin) wrote: > The functioning will depend on Coreboot interpreting properly too. However > fixing the Payload will not cause any regression anyway. I'm not sure what you mean by "interpreting properly". The `bytes_per_line` field has to match the hardware configuration ofc. But it is never interpreted by coreboot, coreboot is the producer of that value, never the consumer. Please don't make this more complicated than it is. The consumer of the framebuffer configuration (in this case TianoCore) simply has to honor the `bytes_per_line` field and shouldn't make any assumptions about it (there is no generic rule how to get from an `x_resolution` to a `bytes_per_line` value, there are only constraints. otherwise, there would be no reason to pass `bytes_per_line` at all). If you'd like, I can add some documentation to `struct lb_framebuffer` (that's how it's called in coreboot, don't know the name in TianoCore). The consumer should be only written with that struct (and it's documen- tation) in mind (i.e. not by looking at coreboot internal code that may change at any time). > I am still not very clear about some cases in Coreboot as below: > >> 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); The piece of code in gm45/gma.c uses `struct edid` as an interface. There is no guarantee that this struct run through the code in edid.c, it could also have been generated somewhere else. But the hardware covered by gm45/gma.c has this requirement, so it has to be done there. Moreover the code in edid.c is broken anyway: it aligns x_resolution up as well, which is wrong (generally. it might be correct for some hardware but that should be handled in the respective driver). Please ignore any code surrounding `struct edid` in coreboot, it's very badly designed and implemented worse. >> 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. Yes, you are right. This "over-alignment" was introduced by accident, by me. I'd like to keep it, though. It's not wrong by any means, and only wastes a little memory. OTOH, as it's using unusual values while still being compliant with hardware and the coreboot framebuffer interface, it's a great measure to discover bugs in consumers of our framebuffer interface. Btw. there is much more gfx init code in coreboot for non-Intel hard- ware. Which may have it's own constraints regarding `bytes_per_pixel`. The only guarantee for `bytes_per_pixel` and `x_resolution` you get as a consumer, is that the former is big enough to hold `x_resolution` pixels. Nico