On 05/08/2014 07:26 PM, David Airlie wrote:
----- Original Message -----
I have an explanation for this, but not a fix. The fix needs to be made
by the owner of this code (Alon or Dave according to the header!)
The bug lies in qxl_display.c:qxl_crtc_mode_set. In this method, there
is a conditional termed "recreate_primary". The logic for this is based
on qcrtc->index. In other words, the "recreate_primary" branch is taken
only on the first head. In this case, the surface_id is FORCED to 0,
and for all other heads, the actual surface_id is used.
However, no _actual_ surface_id's are 0. See above. All surfaces
(valid for this context) have surface_id >0. So it is IMPOSSIBLE for
the monitor_config for the second monitor to have surface_id = 0.
Ok
So you may be asking yourself (or me) - so how is it working (in
gnome3)? Well, that is funny. It's just "working by coincidence" for
gnome3, but broken by design.
Here's how that works. When the bo is created, it _initially_ gets
surface_id = 0. The actual surface id (assigned by qxl_bo_check_id)
isn't assigned until the qxl_update_area_ioctl is called (by userspace).
(there seems to be other paths to get surface checked/allocated,
with the qxl_release stuff. But I have no idea how this works)
In gnome3, this ioctl is called AFTER setting the mode on the 2nd
monitor, so the second branch above which returns the "actual"
surface_id still returns 0 because the surface hasn't been "checked"
(whatever that means). In MATE and other xrandr environments, the ioctl
is called right BEFORE setting the mode, so the actual surface_id is
assigned and we get the trap in spice-widget.c (which I have worked
around).
Thoughts?
How come gnome3 (and mate) draw correctly all the monitors on the surface 0
(that's the only things spice-gtk shows, even with your patch), since the
crtc seems to be associated with a different surface?..
Hopefully Alon or Dave can shed some light here.
Okay I barely remember how this code works, and my memory is saying I wrote it
that way to avoid hitting asserts in the host spice server code, which were close
to impossible to avoid, but I'd have to go setup a test box for this to look into it.
I think the idea was that for the first crtc we had to use the primary surface, but
for non-first crtc's we need a surface id, however if you fixed things so the secondary
head surface id was correct, then qxl host spews up then I'm not sure its really possible
to implement proper KMS support on qxl, though we should be able to avoid this problem,
by always pointing at surface 0, and hoping its big enough, but wayland multi-head would be
badly broken even then.
How about the following patch. It simply looks at the "is_primary"
boolean in the qxl_bo object, and if set, it forces a surface_id of 0 to
be used for the purposes of monitor configuration. I have briefly tested
this and it works at a basic level, and avoids the "FIXME" which
triggers the unwanted behavior in the client.
As I said before, I don't believe that any of the surfaces (once
allocated) actually have an id of 0, and from the other branch of the
conditional (and from the assumptions built into the client) it's plain
that the primary surface must have id of 0, so this seems correct(-ish).
Thanks,
David
Only in qxl.orig/: .#qxl_display.c
Only in qxl.orig/: #qxl_display.c#
diff -ur qxl.orig/qxl_display.c qxl/qxl_display.c
--- qxl.orig/qxl_display.c 2014-03-30 23:40:15.000000000 -0400
+++ qxl/qxl_display.c 2014-05-08 21:41:05.244611653 -0400
@@ -576,7 +576,12 @@
bo->is_primary = true;
surf_id = 0;
} else {
- surf_id = bo->surface_id;
+ if (bo->is_primary) {
+ DRM_DEBUG_KMS("forcing surface_id 0 for primary surface used for non-primary crtc");
+ surf_id = 0;
+ } else {
+ surf_id = bo->surface_id;
+ }
}
if (old_bo && old_bo != bo) {
Only in qxl: qxl_display.c~
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel