[Spice-devel] [PATCH] how can i trace monitor change (etc) events

[David Mansfield]

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



-------------- next part --------------
A non-text attachment was scrubbed...
Name: qxl_force_primary_surface.patch
Type: text/x-patch
Size: 588 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20140508/5f474eeb/attachment.bin>