Thomas Zimmermann <tzimmermann@xxxxxxx> writes:
Hi,
thanks a lot to both of you for this bug fix.
Am 13.04.23 um 03:34 schrieb Pierre Asselin:
(not tested)
Tested. It fixes the regression on my laptop.
diff --git a/drivers/firmware/sysfb_simplefb.c
b/drivers/firmware/sysfb_simplefb.c
index 82c64cb9f531..9f5299d54732 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -56,10 +56,11 @@ __init bool sysfb_parse_mode(const struct screen_info
*si,
* don't specify alpha channels.
*/
if (si->lfb_depth > 8) {
- bits_per_pixel = max(max3(si->red_size + si->red_pos,
+ bits_per_pixel = max3(max3(si->red_size + si->red_pos,
si->green_size + si->green_pos,
si->blue_size + si->blue_pos),
- si->rsvd_size + si->rsvd_pos);
+ si->rsvd_size + si->rsvd_pos,
+ si->lfb_depth);
I'm OK with this change. There's a comment
"The best solution is to compute bits_per_pixel here and ignore
lfb_depth."
I'd change this to
"The best solution is to compute bits_per_pixel here from the color
bits, the reserved bits and the reported color depth; whatever is highest."
That will hopefully clarify the meaning of these max3() statements. They
are not obvious at first.
I'm OK with this as well but then should probably also apply my patch [1]
that computed the stride too. Since if we don't trust the lfb_depth and
calculate the BPP, then we shouldn't trust the reported line length too.
As Pierre reported in the thread [2], when the wrong BPP was calculated (and
wrong pixel format chosen), the line lenght didn't match the BPP * lfb_width.
He mentioned that it was like this:
format=r8g8b8, mode=1024x768x24, linelength=4096
Instead of the expected:
format=r8g8b8, mode=1024x768x24, linelength=3072
My patch in [1], fixed the linelength calculation so it was internally
consistent (but still wrong since the pixel format was really xr8g8b8).
In other words, I think that we should either:
a) Trust the lfb_linelength and lfb_width (we are already doing that since
mode->stride and mode->width are set to those once the format matches).
If we decided to trust those, then the bits-per-pixel could just be
calculated as: bits_per_pixel = si->lfb_linelength * 8 / si->lfb_width
which is what I do on my v2 patch [3].
b) Not trust lfb_linelength, since that would need to be recalculated after
the BPP was calcualted. That's why I mentioned that we need Pierre's fix +
my patch [1] that did:
stride = DIV_ROUND_UP(si->lfb_width * bits_per_pixel, 8)
But calculating a BPP yet blindly using linelength doens't make sense to me.
[1]: https://lists.freedesktop.org/archives/dri-devel/2023-April/399963.html
[2]: https://lore.kernel.org/dri-devel/dfb4f25ca8dfb0d81d778d6315f104ad.squirrel@xxxxxxxxxxxxxx/
[3]: https://lists.freedesktop.org/archives/dri-devel/2023-April/400088.html
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature