Hi Nico, Thanks for the detailed information. It makes sense. I do like the idea of documenting the lb_framebuffer.
> The only guarantee for `bytes_per_pixel` (typo? 'bytes_per_scanline') > and `x_resolution` you get as a consumer, is that the former is big > enough to hold `x_resolution` pixels. I think it would be good to also document that the consumer is assured that in framebuffer, all the 'x_resolution' pixels are aligned at the beginning of each scanline, and the extra bytes are always padded after the 'x_resolution' pixels in the scanline. Would this be true with existing graphics devices? (I am not expert in this area so I'd like to confirm.) > -----Original Message----- > From: Nico Huber [mailto:nic...@gmx.de] > Sent: Saturday, January 27, 2018 10:14 PM > To: You, Benjamin <benjamin....@intel.com> > Cc: Arthur Heymans <art...@aheymans.xyz>; edk2-devel@lists.01.org > Subject: Re: [edk2] [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine > > 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 Thanks, - ben _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
