Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39430 )
Change subject: edid_fill_fb: Add row_alignment argument ......................................................................
Patch Set 13:
What does it fix? What does it provide?
All I see is a positive diffstat (comlexitiy added?), and a working, generic concept (bytes_per_line) replaced by a broken, abstract one that doesn't always apply (bytes_per_line can be arbitrary so far, there is no power-of-2 alignment rule).
I don't really remember but I think I suggested this somewhere hoping that we could then get rid of edid_set_framebuffer_bits_per_pixel() and the redundant x_resolution/y_resolution in struct edid eventually (just because storing the same thing redundantly in multiple forms is never great). Looks like these patches don't quite get there yet.
Fundamentally, the framebuffer layout is defined by the X and Y resolution of the chosen display mode (which is already in edid->mode.ha, so doesn't really need to be slightly adjusted in edid->x_resolution anymore) and the "gap" the display controller expects between the end of one row and the start of the next. In practice I think this gap is always caused by a hardwired alignment (at least all cases I've seen suggest that) so that's why we use row_alignment to model that (which we have been doing with edid_set_framebuffer_bits_per_pixel() for a while already). I think the bytes_per_line is just a hold-over from VBE modes but it's weird because it is always dependent on the display mode (which is decided by the panel, not a fundamental property of the display controller hardware). So when writing native graphics init the thing the SoC display code wants to provide is really just that alignment, and right now the SoC code always has to call this function to translate it into a resolution-dependent value after the panel resolution is known, which is a bit weird and unnecessarily roundabout.
I would kinda like to separate the stuff that comes from the panel (like resolution) which belongs in struct edid from the stuff that comes from display controller hardware (like that alignment) which probably belongs somewhere else. Maybe this patch series isn't the right place to shoehorn that in, I don't know... but in general struct edid has some redundancies and some confusion of purpose (storing info that's really not related to the EDID) that would be nice to get rid of.