| Message ID | 1355398603-22186-2-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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 > > >
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
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(-)
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
----- 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 > > >
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
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 > >
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
> 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 > > >
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),
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(-)