Patchwork [2/2] qxl: change rom so that 4096 < size < 8192

login
register
mail settings
Submitter Alon Levy
Date Dec. 13, 2012, 11:36 a.m.
Message ID <1355398603-22186-2-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/205809/
State New
Headers show

Comments

Alon Levy - Dec. 13, 2012, 11:36 a.m.
This is a simpler solution to 869981, where migration breaks since qxl's
rom bar size has changed. Instead of ignoring fields in QXLRom, which is what has
actually changed, we remove some of the modes, a mechanism already
accounted for by the guest. To reach exactly two pages and not one, we
remove two out of four orientations, the remaining are normal and right
turn (chosen arbitrarily). Leaving only normal would result in a single
page which would also break migration.

Added assertions to ensure changes in spice-protocol in the future
causing increase or decrease of page size will result in failure at
startup (could do this compile time also but not sure how).

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
Gerd Hoffmann - Dec. 13, 2012, 12:05 p.m.
On 12/13/12 12:36, Alon Levy wrote:
> This is a simpler solution to 869981, where migration breaks since qxl's
> rom bar size has changed. Instead of ignoring fields in QXLRom, which is what has
> actually changed, we remove some of the modes, a mechanism already
> accounted for by the guest. To reach exactly two pages and not one, we
> remove two out of four orientations, the remaining are normal and right
> turn (chosen arbitrarily). Leaving only normal would result in a single
> page which would also break migration.
> 
> Added assertions to ensure changes in spice-protocol in the future
> causing increase or decrease of page size will result in failure at
> startup (could do this compile time also but not sure how).

The assertions are not in the patch.

>  #define QXL_MODE_EX(x_res, y_res)                 \
>      QXL_MODE_16_32(x_res, y_res, 0),              \
> -    QXL_MODE_16_32(y_res, x_res, 1),              \
> -    QXL_MODE_16_32(x_res, y_res, 2),              \
> -    QXL_MODE_16_32(y_res, x_res, 3)
> +    QXL_MODE_16_32(x_res, y_res, 1)

Why do you leave orientation = 1 in?  Just to keep the size above 4K?
Shouldn't we just hardcode the rom size to 8k instead?  Then assert that
everything fits into 8k?  Or even better add a compile time check?

While being at it it might be a good idea to move the mode table to a
fixed, large enougth offset (say 0x4096), so it doesn't move around
again in case we extend QXLRom one more time.

cheers,
  Gerd
Alon Levy - Dec. 23, 2012, 8:31 p.m.
On Thu, Dec 13, 2012 at 01:05:48PM +0100, Gerd Hoffmann wrote:
> On 12/13/12 12:36, Alon Levy wrote:
> > This is a simpler solution to 869981, where migration breaks since qxl's
> > rom bar size has changed. Instead of ignoring fields in QXLRom, which is what has
> > actually changed, we remove some of the modes, a mechanism already
> > accounted for by the guest. To reach exactly two pages and not one, we
> > remove two out of four orientations, the remaining are normal and right
> > turn (chosen arbitrarily). Leaving only normal would result in a single
> > page which would also break migration.
> > 
> > Added assertions to ensure changes in spice-protocol in the future
> > causing increase or decrease of page size will result in failure at
> > startup (could do this compile time also but not sure how).
> 
> The assertions are not in the patch.
> 
> >  #define QXL_MODE_EX(x_res, y_res)                 \
> >      QXL_MODE_16_32(x_res, y_res, 0),              \
> > -    QXL_MODE_16_32(y_res, x_res, 1),              \
> > -    QXL_MODE_16_32(x_res, y_res, 2),              \
> > -    QXL_MODE_16_32(y_res, x_res, 3)
> > +    QXL_MODE_16_32(x_res, y_res, 1)
> 
> Why do you leave orientation = 1 in?  Just to keep the size above 4K?
> Shouldn't we just hardcode the rom size to 8k instead?  Then assert that
> everything fits into 8k?  Or even better add a compile time check?
> 
> While being at it it might be a good idea to move the mode table to a
> fixed, large enougth offset (say 0x4096), so it doesn't move around
> again in case we extend QXLRom one more time.

This solution is breaking backward compatibility like Yonit pointed
out. The fact that I can't produce a user that would break because of
this doesn't prove there is no such user. I suggest we go back to the
original patch I posted, breaking it into two like you requested. What
do you say?

> 
> cheers,
>   Gerd
>
Alon Levy - Jan. 15, 2013, 1:34 p.m.
On Sun, Dec 23, 2012 at 10:31:38PM +0200, Alon Levy wrote:
> On Thu, Dec 13, 2012 at 01:05:48PM +0100, Gerd Hoffmann wrote:
> > On 12/13/12 12:36, Alon Levy wrote:
> > > This is a simpler solution to 869981, where migration breaks since qxl's
> > > rom bar size has changed. Instead of ignoring fields in QXLRom, which is what has
> > > actually changed, we remove some of the modes, a mechanism already
> > > accounted for by the guest. To reach exactly two pages and not one, we
> > > remove two out of four orientations, the remaining are normal and right
> > > turn (chosen arbitrarily). Leaving only normal would result in a single
> > > page which would also break migration.
> > > 
> > > Added assertions to ensure changes in spice-protocol in the future
> > > causing increase or decrease of page size will result in failure at
> > > startup (could do this compile time also but not sure how).
> > 
> > The assertions are not in the patch.
> > 
> > >  #define QXL_MODE_EX(x_res, y_res)                 \
> > >      QXL_MODE_16_32(x_res, y_res, 0),              \
> > > -    QXL_MODE_16_32(y_res, x_res, 1),              \
> > > -    QXL_MODE_16_32(x_res, y_res, 2),              \
> > > -    QXL_MODE_16_32(y_res, x_res, 3)
> > > +    QXL_MODE_16_32(x_res, y_res, 1)
> > 
> > Why do you leave orientation = 1 in?  Just to keep the size above 4K?
> > Shouldn't we just hardcode the rom size to 8k instead?  Then assert that
> > everything fits into 8k?  Or even better add a compile time check?
> > 
> > While being at it it might be a good idea to move the mode table to a
> > fixed, large enougth offset (say 0x4096), so it doesn't move around
> > again in case we extend QXLRom one more time.
> 
> This solution is breaking backward compatibility like Yonit pointed
> out. The fact that I can't produce a user that would break because of
> this doesn't prove there is no such user. I suggest we go back to the
> original patch I posted, breaking it into two like you requested. What
> do you say?

Ping?

> 
> > 
> > cheers,
> >   Gerd
> > 
>
Gerd Hoffmann - Jan. 15, 2013, 3:19 p.m.
Hi,

>>>>  #define QXL_MODE_EX(x_res, y_res)                 \
>>>>      QXL_MODE_16_32(x_res, y_res, 0),              \
>>>> -    QXL_MODE_16_32(y_res, x_res, 1),              \
>>>> -    QXL_MODE_16_32(x_res, y_res, 2),              \
>>>> -    QXL_MODE_16_32(y_res, x_res, 3)
>>>> +    QXL_MODE_16_32(x_res, y_res, 1)
>>>
>>> Why do you leave orientation = 1 in?  Just to keep the size above 4K?
>>> Shouldn't we just hardcode the rom size to 8k instead?  Then assert that
>>> everything fits into 8k?  Or even better add a compile time check?
>>>
>>> While being at it it might be a good idea to move the mode table to a
>>> fixed, large enougth offset (say 0x4096), so it doesn't move around
>>> again in case we extend QXLRom one more time.
>>
>> This solution is breaking backward compatibility like Yonit pointed
>> out. The fact that I can't produce a user that would break because of
>> this doesn't prove there is no such user. I suggest we go back to the
>> original patch I posted, breaking it into two like you requested. What
>> do you say?
> 
> Ping?

I think I've answered this one already.

It is still not clear to me how this orientation thing is supposed to
work and what exactly we loose when we drop those modes from the list.

I havn't found a way to activate a portrait mode (orientation == 1,3) in
the windows  guest.

Orientation isn't used anywhere in qxl (other than rom initialization),
so in case the guest activates a mode with orientation != 0 spice server
(and thus spice client) isn't notified about that.  So spice client
wouldn't rotate the display according to the guests orientation.  Hmm?

And finally only prehistorical 0.4.x spice guests (which use SET_MODE)
can pick modes with orientation != 0.  The QXLSurfaceCreate struct has
no orientation field ...

cheers,
  Gerd
Alon Levy - Jan. 16, 2013, 5:59 p.m.
Regarding orientation setting in windows 7 64 guest:
Desktop, right click->Screen resolution
 - You can choose Orientation: Landscape, Portrait, Landscape (flipped), Portrait (flipped)
 - You can choose Resolution
 - You can click "Advanced Settings", then "List All Modes" at the bottom, you get all the modes (i.e. four of each resolution, one for each orientation)

There are two changes after applying the "change rom size to 8192" patch:
 - there is no longer an Orientation option
 - the modes listed under "List All Modes" reduce as expected

Changes to the second patch:
 - no orientations except the normal
 - hard code 8192 bytes rom size
 - assert if the required size is larger

Alon Levy (2):
  qxl: stop using non revision 4 rom fields for revision < 4
  qxl: change rom size to 8192

 hw/qxl.c     | 25 ++++++++++++++++++-------
 trace-events |  2 ++
 2 files changed, 20 insertions(+), 7 deletions(-)
Gerd Hoffmann - Jan. 17, 2013, 1:02 p.m.
On 01/16/13 18:59, Alon Levy wrote:
> Regarding orientation setting in windows 7 64 guest:
> Desktop, right click->Screen resolution
>  - You can choose Orientation: Landscape, Portrait, Landscape (flipped), Portrait (flipped)
>  - You can choose Resolution
>  - You can click "Advanced Settings", then "List All Modes" at the bottom, you get all the modes (i.e. four of each resolution, one for each orientation)

Ah, ok.  The driver seems to handle portrait and swap x+y when creating
a displaysurface.  At least I get a 600x800 display upright.

I can't see a difference between Landscape + Landscape (flipped).
Likewise Portrait + Portrait (flipped).  Is there any?

> There are two changes after applying the "change rom size to 8192" patch:
>  - there is no longer an Orientation option
>  - the modes listed under "List All Modes" reduce as expected

Ok, so we loose the Portrait mode.

> Changes to the second patch:
>  - no orientations except the normal

Keeping orientation 0+1 (and dropping the flipped 2+3 versions) should
make the mode list small enougth that it fits while maintaining support
for the portrait mode.

I think it would also be good to fix the driver to ignore everything with or

How about that?

>  - hard code 8192 bytes rom size
>  - assert if the required size is larger

Good.

cheers,
  Gerd
Alon Levy - Jan. 17, 2013, 1:28 p.m.
----- Original Message -----
> On 01/16/13 18:59, Alon Levy wrote:
> > Regarding orientation setting in windows 7 64 guest:
> > Desktop, right click->Screen resolution
> >  - You can choose Orientation: Landscape, Portrait, Landscape
> >  (flipped), Portrait (flipped)
> >  - You can choose Resolution
> >  - You can click "Advanced Settings", then "List All Modes" at the
> >  bottom, you get all the modes (i.e. four of each resolution, one
> >  for each orientation)
> 
> Ah, ok.  The driver seems to handle portrait and swap x+y when
> creating
> a displaysurface.  At least I get a 600x800 display upright.
> 
> I can't see a difference between Landscape + Landscape (flipped).
> Likewise Portrait + Portrait (flipped).  Is there any?
> 
> > There are two changes after applying the "change rom size to 8192"
> > patch:
> >  - there is no longer an Orientation option
> >  - the modes listed under "List All Modes" reduce as expected
> 
> Ok, so we loose the Portrait mode.
> 
> > Changes to the second patch:
> >  - no orientations except the normal
> 
> Keeping orientation 0+1 (and dropping the flipped 2+3 versions)
> should
> make the mode list small enougth that it fits while maintaining
> support
> for the portrait mode.

I'll test if this changes anything for a windows guest & linux guest.

> 
> I think it would also be good to fix the driver to ignore everything
> with or

... what was the end of that sentence?

> 
> How about that?
> 
> >  - hard code 8192 bytes rom size
> >  - assert if the required size is larger
> 
> Good.
> 
> cheers,
>   Gerd
> 
> 
>
Gerd Hoffmann - Jan. 17, 2013, 1:44 p.m.
Hi,

>> I think it would also be good to fix the driver to ignore everything
>> with or

> ... what was the end of that sentence?

.. orientation != 0, then registers every mode with the orientations it
wants, so orientation becomes unused with newer drivers (and we keep
orientation=0,1 for old driver compatibility).

But maybe this isn't worth the trouble.

cheers,
  Gerd
Alon Levy - Jan. 20, 2013, 4:30 p.m.
On Thu, Jan 17, 2013 at 02:02:26PM +0100, Gerd Hoffmann wrote:
> On 01/16/13 18:59, Alon Levy wrote:
> > Regarding orientation setting in windows 7 64 guest:
> > Desktop, right click->Screen resolution
> >  - You can choose Orientation: Landscape, Portrait, Landscape (flipped), Portrait (flipped)
> >  - You can choose Resolution
> >  - You can click "Advanced Settings", then "List All Modes" at the bottom, you get all the modes (i.e. four of each resolution, one for each orientation)
> 
> Ah, ok.  The driver seems to handle portrait and swap x+y when creating
> a displaysurface.  At least I get a 600x800 display upright.
> 
> I can't see a difference between Landscape + Landscape (flipped).
> Likewise Portrait + Portrait (flipped).  Is there any?

I can't actually get the "(flipped)" modes (both portrait and landscape)
to work, I get an error message "Unable to save display settings". How
did you manage to get them to work? which driver, qemu command line,
qemu version did you use?

> 
> > There are two changes after applying the "change rom size to 8192" patch:
> >  - there is no longer an Orientation option
> >  - the modes listed under "List All Modes" reduce as expected
> 
> Ok, so we loose the Portrait mode.
> 
> > Changes to the second patch:
> >  - no orientations except the normal
> 
> Keeping orientation 0+1 (and dropping the flipped 2+3 versions) should
> make the mode list small enougth that it fits while maintaining support
> for the portrait mode.

That's what I'm going to send.

> 
> I think it would also be good to fix the driver to ignore everything with or
> 
> How about that?
> 
> >  - hard code 8192 bytes rom size
> >  - assert if the required size is larger
> 
> Good.
> 
> cheers,
>   Gerd
> 
>
Gerd Hoffmann - Jan. 21, 2013, 6:26 a.m.
Hi,

>> I can't see a difference between Landscape + Landscape (flipped).
>> Likewise Portrait + Portrait (flipped).  Is there any?
> 
> I can't actually get the "(flipped)" modes (both portrait and landscape)
> to work, I get an error message "Unable to save display settings". How
> did you manage to get them to work? which driver, qemu command line,
> qemu version did you use?

upstream qemu, qxl.rev set to 3, qxl driver 4.5.something (i.e. not the
latest 5.x).

cheers,
  Gerd
Alon Levy - Jan. 21, 2013, 12:47 p.m.
> On 01/16/13 18:59, Alon Levy wrote:
> > Regarding orientation setting in windows 7 64 guest:
> > Desktop, right click->Screen resolution
> >  - You can choose Orientation: Landscape, Portrait, Landscape
> >  (flipped), Portrait (flipped)
> >  - You can choose Resolution
> >  - You can click "Advanced Settings", then "List All Modes" at the
> >  bottom, you get all the modes (i.e. four of each resolution, one
> >  for each orientation)
> 
> Ah, ok.  The driver seems to handle portrait and swap x+y when
> creating
> a displaysurface.  At least I get a 600x800 display upright.
> 
> I can't see a difference between Landscape + Landscape (flipped).
> Likewise Portrait + Portrait (flipped).  Is there any?

I couldn't see any visible change when using 6.1.0.1015, so I still have no idea. I'm sure it was supposed to flip upside down. Perhaps the driver doesn't support it and the gui doesn't acknowledge it.

> 
> > There are two changes after applying the "change rom size to 8192"
> > patch:
> >  - there is no longer an Orientation option
> >  - the modes listed under "List All Modes" reduce as expected
> 
> Ok, so we loose the Portrait mode.
> 
> > Changes to the second patch:
> >  - no orientations except the normal
> 
> Keeping orientation 0+1 (and dropping the flipped 2+3 versions)
> should
> make the mode list small enougth that it fits while maintaining
> support
> for the portrait mode.

Sending a patch with this change.

> 
> I think it would also be good to fix the driver to ignore everything
> with or
> 
> How about that?
> 
> >  - hard code 8192 bytes rom size
> >  - assert if the required size is larger
> 
> Good.
> 
> cheers,
>   Gerd
> 
> 
>

Patch

diff --git a/hw/qxl.c b/hw/qxl.c
index 8611ee9..99b354a 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -88,9 +88,7 @@ 
 
 #define QXL_MODE_EX(x_res, y_res)                 \
     QXL_MODE_16_32(x_res, y_res, 0),              \
-    QXL_MODE_16_32(y_res, x_res, 1),              \
-    QXL_MODE_16_32(x_res, y_res, 2),              \
-    QXL_MODE_16_32(y_res, x_res, 3)
+    QXL_MODE_16_32(x_res, y_res, 1)
 
 static QXLMode qxl_modes[] = {
     QXL_MODE_EX(640, 480),