[Spice-devel] [video-qxl 2/2] Default to only one head, not 4.

[Hans de Goede]

Hi,

On 01/23/2013 11:39 PM, Jeremy White wrote:
>> BTW reading patch 1 again, I wonder what exactly it tries to solve, since
>> currently when using spice with qemu (no experience with x-spice) you can
>> already set modes > 1024x768 without problems.
>
> Alright, I've spent my day relearning this code, and feel I have a
> better grasp.
>
> The issue is primarily this code here:
>   http://cgit.freedesktop.org/xorg/driver/xf86-video-qxl/tree/src/qxl_driver.c#n2325
>
> That sets the current virtual x/y to 1024x768, and calls Xorg's code to
> process the initial configuration.  One of the things that code does is
> read the configured modelines in the supplied xorg.conf file, and finds
> a mode that fits within that virtual size.  However, having had the
> virtual display set to 1024, that code will apply that as a maximum, and
> will pick a modeline from the spiceqxl.xorg.conf file that is within
> 1024x768.
>
> You *can* use xrandr to change to a larger resolution later on, but you
> cannot specify a default starting resolution in the spiceqxl.xorg.conf
> file.  As far as I can tell, virt-viewer usage never uses an xorg.conf
> file, so you guys never see this problem.  You've always got
> gnome-settings-daemon or someone else resizing displays for you (which
> does, in fact work).
>
> In my humble opinion, the current code attempts to set a default mode of
> 1024x768 in a crude and incorrect fashion.
>
> This led to Marc-André's patch of last July (c1b537fc,
>   Return a preferred mode matching the current mode), which
> I think in turn triggers bug 894421.

Agreed.

> The good news is that I ever so humbly <grin> think my patch 1 fixes all
> of these issues.

Agreed too, I actually send a patch yesterday which borrows part of
your patch to fix bug 894421.

> Essentially, it sets the M_T_PREFERRED flag on a 1024x768 mode in a
> non invasive way.  In my testing, it works correctly for my case, and
> for the cases I tried with virt viewer and fc18. I was able to reproduce
> the behavior Marc-André reported last summer,
> and confirm that my patch seems to also correct for it.

Good!

> I do have some mysteries.  fc18 doesn't seem to restore screen
> resolution after reboot, which surprises me.  (This is true both with
> and without my patch).

This is normal behavior. The screen resolution "restoring" is done
by gnome-settings-daemon (or something similar if running another desktop
environment) and gnome-settings-manager will only restore the last resolution
set through gnome-display-settings. So if you've changed the size by simply
resizing the window this won't get restored since it never got stored in
the gnome-settings. So you end up with the default 1024x768, unless you've
actually used gnome-display-settings before, and then you end up with whatever
you picked the last time you ran gnome-display-settings.

This is all not very pretty, but basically the agent and the gnome-settings
stuff are running independent of each other, and integrating them is tricky
because we don't want to dependent on a certain desktop environment.

<snip>

> I also could not reproduce bug 894421,

I can and I'm going to try your patch now instead of my more minimal fix.

 > although without my patch I see a variation of it,
> where default window sizes are often perversely small.

Yes I just hit that too, but only once. I'm happy to hear that your patch
fixes this too :)

> I am resubmitting my patch 1 as a new patch, which partially reverts
> c1b537fc; I would appreciate it if others could review that code.

I've reviewed it and it looks good, but I'm a bit unfamiliar with some
of the parts of xorg involved. I'm going to do some testing with your
patch now on both RHEL-6.4 and F-18 guests and then I'll reply to
the mail where you send the new version of the patch.

Regards,

Hans