Hi Am 17.04.23 um 10:58 schrieb Javier Martinez Canillas:
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 becalculated 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)
I'd rather keep the code as-is until we get an actual bug report.For example, DRM framebuffer sizes are often multiples of 64. Creating a framebuffer of 800x600 will create a framebuffer with stride/pitch/linelength of 832. I can imagine that some BIOSes out there do something similar with the system framebuffer. Messing with the stride would break them.
Best regards Thomas
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
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
