Re: [PATCH] CorebootPayloadPkg: Use correct BytesPerScanLine

[Nico Huber <nico.h@gmx.de>]

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