[Wayland-bugs] [Bug 101472] compositor-fbdev: Added parameter --pixman-type

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=101472

--- Comment #5 from Pekka Paalanen <ppaalanen at gmail.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-bugs/attachments/20170620/33c332e3/attachment.html>