Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

["You, Benjamin" <benjamin.you@intel.com>]

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:nico.h@gmx.de]
> Sent: Saturday, January 27, 2018 10:14 PM
> To: You, Benjamin <benjamin.you@intel.com>
> Cc: Arthur Heymans <arthur@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