Re: [PATCH] firmware/sysfb: Fix wrong stride when bits-per-pixel is calculated

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 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)

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


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux