Created attachment 132010 [details] [review]
compositor-fbdev: Added parameter --pixman-type

This parameter allows to override the weston heuristic in
calculate_pixman_format() and specify a PIXMAN_TYPE_ value.
This is needed at least in LG Nexus 5 because the framebuffer
drivers seem to be reporting wrong information and causes
the screen to be redish when weston is run.

Example of red screen on Nexus 5:
http://imgur.com/a/izehE

Hi,

please send patches to the mailing list as per "Constribution instructions" link on https://wayland.freedesktop.org/ .

Are you sure this is a bug in the kernel driver and not in weston's calculate_pixman_format()?

Is it not possible to fix the kernel drivers to improve things for everyone rather than just for Weston users? Are the drivers proprietary, binary-only? Are there no FOSS alternatives?

I would like to have the reason why this cannot be fixed in the kernel mentioned in the commit message, or if it can be fixed, some rationale why the workaround is still necessary in Weston.

As we have this bug report already, please also add the bugzilla reference in the commit message, and a signed-off-by.

Hi Pekka,

I am also involved with postmarketOS.

> Are you sure this is a bug in the kernel driver and not in weston's calculate_pixman_format()?

We are *not* sure about that. Could you tell us how to find that out?

> Is it not possible to fix the kernel drivers to improve things for everyone rather than just for Weston users? Are the drivers proprietary, binary-only? Are there no FOSS alternatives?

The drivers are open, but right now we're trying to get Weston running on a lot of smartphones, which sadly all run their own fork of some outdated Android kernel.
In the long run, we would really like to have them all run on mainline and have proper drivers (that's the *real* fix for everyone!), but that surely won't happen over night if ever, so we propose this patch as a solution for in-between.

I have already proposed a patch for another (possibly phone-only) issue, which appeared on the Nexus 4, that got accepted: https://patchwork.freedesktop.org/patch/150943/

Last but not least, the patch is just a few lines of code and *adds* a command-line parameter. That might even be useful for other people to quickly check if their driver *would* work if the pixman type was swapped (without recompiling Weston or their kernel), before they could do a proper fix in the kernel driver.

So... do you think, this patch has a chance to get accepted, when we fix the description and add the signed-off-by line and send it in via mail, or not? (Which would then lead to a bit more fragmentation, as we'd need to carry that patch around.)

Thank you and best regards,
Oliver Smith

(In reply to Oliver Smith from comment #4)
> > Are you sure this is a bug in the kernel driver and not in weston's calculate_pixman_format()?
> 
> We are *not* sure about that. Could you tell us how to find that out?

You would look at the kernel's documentation about the parameters (I hope there is some), you check what values the driver is giving out, and compare that to what calculate_pixman_format() does. Does the function make assumptions or take shortcuts that are not correct in general and particularly in your case. Pixman unfortunately doesn't have documentation, but the formats are defined in "bits of a uint32_t" in native endianess.

It would be good to have a Weston log snippet about the format detection in the commit message to show the problematic fbdev pixel format configuration, so reviewers can then check it against Pixman formats.

This is a little-endian platform you have, is it not?

> The drivers are open, but right now we're trying to get Weston running on a
> lot of smartphones, which sadly all run their own fork of some outdated
> Android kernel.
> In the long run, we would really like to have them all run on mainline and
> have proper drivers (that's the *real* fix for everyone!), but that surely
> won't happen over night if ever, so we propose this patch as a solution for
> in-between.
> 

Understood.

> I have already proposed a patch for another (possibly phone-only) issue,
> which appeared on the Nexus 4, that got accepted:
> https://patchwork.freedesktop.org/patch/150943/
> 
> Last but not least, the patch is just a few lines of code and *adds* a
> command-line parameter. That might even be useful for other people to
> quickly check if their driver *would* work if the pixman type was swapped
> (without recompiling Weston or their kernel), before they could do a proper
> fix in the kernel driver.
> 

Everything has a maintenance cost, and I would not want to encourage broken drivers left unfixed because enough of userspace has workarounds or even starts depending on the broken behaviour (*if* the driver actually is broken).

Not endorsing broken designs was the reason we removed EGL support from the fbdev-backend, but the removal also points out we accepted EGL support in the fbdev-backend in the past for practical reasons. So we definitely have precedence both ways.

> So... do you think, this patch has a chance to get accepted, when we fix the
> description and add the signed-off-by line and send it in via mail, or not?

I suppose the idea is ok from my behalf but of course I'm not the only person to judge. When adding a command line arg, please update the '--help' summary too. We don't seem to have man-pages for the fbdev-backend, so no need to update those.

Finally, please ensure that passing in a wrong format in --pixman-type cannot cause buffer overflows or other memory access problem, i.e. make sure the size of a pixel matches what is used with the allocations, particularly when wrapping memory not allocated by pixman.

I have not done a proper technical review, so these are not all the review comments. That will happen on the mailing list by someone. Changing compositor.h for this looks strange.

Submitting via mail would be best, but personally I would like to avoid this patch in favour of having the kernel drivers fixed. Sorry.