Re: [PATCH] virtio-gpu: Respect graphics update interval for EDID
On Fri, Feb 26, 2021 at 01:47:38PM +0900, Akihiko Odaki wrote: > 2021年2月25日(木) 20:46 Gerd Hoffmann : > > > > Hi, > > > > > > Because of the wasted frames I'd like this to be an option you can > > > > enable when needed. For the majority of use cases this seems to be > > > > no problem ... > > > > > > I see blinks with GNOME on Wayland on Ubuntu 20.04 and virtio-gpu with > > > the EDID change included in this patch. > > > > /me looks closely at the patch again. > > > > So you update the edid dynamically, each time the refresh rate changes. > > Problem with that approach is software doesn't expect edid to change > > dynamically because on physical hardware it is static information about > > the connected monitor. > > > > So what the virtio-gpu guest driver does is emulate a monitor hotplug > > event to notify userspace. If you resize the qemu window on the host > > it'll look like the monitor with the old window size was unplugged and > > a new monitor with the new window size got plugged instead, so gnome > > shell goes adapt the display resolution to the new virtual monitor size. > > > > The blink you are seeing probably comes from gnome-shell processing the > > monitor hotplug event. > > > > We could try to skip generating a monitor hotplug event in case only the > > refresh rate did change. That would fix the blink, but it would also > > have the effect that nobody will notice the update. > > > > Bottom line: I think making the edid refresh rate configurable might be > > useful, but changing it dynamically most likely isn't. > > > > take care, > > Gerd > > The "hotplug" implementation is probably what other combinations of > devices and guests will do if they want to respond to the changes of > the refresh rate, or display mode in general. That makes telling the > dynamic refresh rate to guests infeasible. > > As you wrote, making the refresh rate configurable should be still > useful, and I think matching it to the backend physical display is > even better. GTK, the sole implementer of gfx_update_interval in my > patch reports the refresh rate of the physical display the window > resides in. It means the value may change when the physical display > changes its refresh rate, which should be rare if it does, or the > window moves to another physical display. Yes. Added debug printf, can see that it changes exactly twice for me, once to the default value and then to the real refresh rate. That rules out the hotplug event as source for the blinks. I'm wondering where they are coming from, and I can't reproduce them on my machine ... > consistent. The only inconsistency I see in my patch is that the > refresh rate change has no throttling while the window size change > has. I don't think it is problematic because it should be rare to move > the window across physical displays, but I can implement one if you > don't agree or know other cases the refresh rate frequently changes. I think it would be best to just add a new field for the refresh rate to QemuUIInfo. That avoids a new callback you also get the throttling for free ;) take care, Gerd
Re: [PATCH] virtio-gpu: Respect graphics update interval for EDID
2021年2月25日(木) 20:46 Gerd Hoffmann : > > Hi, > > > > Because of the wasted frames I'd like this to be an option you can > > > enable when needed. For the majority of use cases this seems to be > > > no problem ... > > > > I see blinks with GNOME on Wayland on Ubuntu 20.04 and virtio-gpu with > > the EDID change included in this patch. > > /me looks closely at the patch again. > > So you update the edid dynamically, each time the refresh rate changes. > Problem with that approach is software doesn't expect edid to change > dynamically because on physical hardware it is static information about > the connected monitor. > > So what the virtio-gpu guest driver does is emulate a monitor hotplug > event to notify userspace. If you resize the qemu window on the host > it'll look like the monitor with the old window size was unplugged and > a new monitor with the new window size got plugged instead, so gnome > shell goes adapt the display resolution to the new virtual monitor size. > > The blink you are seeing probably comes from gnome-shell processing the > monitor hotplug event. > > We could try to skip generating a monitor hotplug event in case only the > refresh rate did change. That would fix the blink, but it would also > have the effect that nobody will notice the update. > > Bottom line: I think making the edid refresh rate configurable might be > useful, but changing it dynamically most likely isn't. > > take care, > Gerd > The "hotplug" implementation is probably what other combinations of devices and guests will do if they want to respond to the changes of the refresh rate, or display mode in general. That makes telling the dynamic refresh rate to guests infeasible. As you wrote, making the refresh rate configurable should be still useful, and I think matching it to the backend physical display is even better. GTK, the sole implementer of gfx_update_interval in my patch reports the refresh rate of the physical display the window resides in. It means the value may change when the physical display changes its refresh rate, which should be rare if it does, or the window moves to another physical display. In the former case, there is nothing different from implementing a physical display driver for guests so there should be no problem. The latter case is similar to how the changes of the window size, which is also part of display mode, is delivered to guests and they should be consistent. The only inconsistency I see in my patch is that the refresh rate change has no throttling while the window size change has. I don't think it is problematic because it should be rare to move the window across physical displays, but I can implement one if you don't agree or know other cases the refresh rate frequently changes. Regards, Akihiko Odaki
Re: [PATCH] virtio-gpu: Respect graphics update interval for EDID
Hi, > > Because of the wasted frames I'd like this to be an option you can > > enable when needed. For the majority of use cases this seems to be > > no problem ... > > I see blinks with GNOME on Wayland on Ubuntu 20.04 and virtio-gpu with > the EDID change included in this patch. /me looks closely at the patch again. So you update the edid dynamically, each time the refresh rate changes. Problem with that approach is software doesn't expect edid to change dynamically because on physical hardware it is static information about the connected monitor. So what the virtio-gpu guest driver does is emulate a monitor hotplug event to notify userspace. If you resize the qemu window on the host it'll look like the monitor with the old window size was unplugged and a new monitor with the new window size got plugged instead, so gnome shell goes adapt the display resolution to the new virtual monitor size. The blink you are seeing probably comes from gnome-shell processing the monitor hotplug event. We could try to skip generating a monitor hotplug event in case only the refresh rate did change. That would fix the blink, but it would also have the effect that nobody will notice the update. Bottom line: I think making the edid refresh rate configurable might be useful, but changing it dynamically most likely isn't. take care, Gerd
Re: [PATCH] virtio-gpu: Respect graphics update interval for EDID
2021年2月24日(水) 20:17 Gerd Hoffmann : > > On Tue, Feb 23, 2021 at 01:50:51PM +0900, Akihiko Odaki wrote: > > 2021年2月22日(月) 19:57 Gerd Hoffmann : > > > > > > On Sun, Feb 21, 2021 at 10:34:14PM +0900, Akihiko Odaki wrote: > > > > This change introduces an additional member, refresh_rate to > > > > qemu_edid_info in include/hw/display/edid.h. > > > > > > > > This change also isolates the graphics update interval from the > > > > display update interval. The guest will update the frame buffer > > > > in the graphics update interval, but displays can be updated in a > > > > dynamic interval, for example to save update costs aggresively > > > > (vnc) or to respond to user-generated events (sdl). > > > > It stabilizes the graphics update interval and prevents the guest > > > > from being confused. > > > > > > Hmm. What problem you are trying to solve here? > > > > > > The update throttle being visible by the guest was done intentionally, > > > so the guest can throttle the display updates too in case nobody is > > > watching those display updated anyway. > > > > Indeed, we are throttling the update for vnc to avoid some worthless > > work. But typically a guest cannot respond to update interval changes > > so often because real display devices the guest is designed for does > > not change the update interval in that way. > > What is the problem you are seeing? > > Some guest software raising timeout errors when they see only > one vblank irq every 3 seconds? If so: which software is this? > Any chance we can fix this on the guest side? > > > That is why we have to > > tell the guest a stable update interval even if it results in wasted > > frames. > > Because of the wasted frames I'd like this to be an option you can > enable when needed. For the majority of use cases this seems to be > no problem ... I see blinks with GNOME on Wayland on Ubuntu 20.04 and virtio-gpu with the EDID change included in this patch. The only devices inspecting the variable, xenfb and modified virtio-gpu, do not yield vblank irq, but they report the refresh rate to the guest, and the guest proactively requests them to switch the surface. I suspect Linux's kernel mode setting causes blinks and other guests have similar problems. > > Also: the EDID changes should go to a separate patch. That makes sense. I'll isolate it to a seperate patch in a series. Regards, Akihiko Odaki > > take care, > Gerd >
Re: [PATCH] virtio-gpu: Respect graphics update interval for EDID
On Tue, Feb 23, 2021 at 01:50:51PM +0900, Akihiko Odaki wrote: > 2021年2月22日(月) 19:57 Gerd Hoffmann : > > > > On Sun, Feb 21, 2021 at 10:34:14PM +0900, Akihiko Odaki wrote: > > > This change introduces an additional member, refresh_rate to > > > qemu_edid_info in include/hw/display/edid.h. > > > > > > This change also isolates the graphics update interval from the > > > display update interval. The guest will update the frame buffer > > > in the graphics update interval, but displays can be updated in a > > > dynamic interval, for example to save update costs aggresively > > > (vnc) or to respond to user-generated events (sdl). > > > It stabilizes the graphics update interval and prevents the guest > > > from being confused. > > > > Hmm. What problem you are trying to solve here? > > > > The update throttle being visible by the guest was done intentionally, > > so the guest can throttle the display updates too in case nobody is > > watching those display updated anyway. > > Indeed, we are throttling the update for vnc to avoid some worthless > work. But typically a guest cannot respond to update interval changes > so often because real display devices the guest is designed for does > not change the update interval in that way. What is the problem you are seeing? Some guest software raising timeout errors when they see only one vblank irq every 3 seconds? If so: which software is this? Any chance we can fix this on the guest side? > That is why we have to > tell the guest a stable update interval even if it results in wasted > frames. Because of the wasted frames I'd like this to be an option you can enable when needed. For the majority of use cases this seems to be no problem ... Also: the EDID changes should go to a separate patch. take care, Gerd
Re: [PATCH] virtio-gpu: Respect graphics update interval for EDID
2021年2月22日(月) 19:57 Gerd Hoffmann : > > On Sun, Feb 21, 2021 at 10:34:14PM +0900, Akihiko Odaki wrote: > > This change introduces an additional member, refresh_rate to > > qemu_edid_info in include/hw/display/edid.h. > > > > This change also isolates the graphics update interval from the > > display update interval. The guest will update the frame buffer > > in the graphics update interval, but displays can be updated in a > > dynamic interval, for example to save update costs aggresively > > (vnc) or to respond to user-generated events (sdl). > > It stabilizes the graphics update interval and prevents the guest > > from being confused. > > Hmm. What problem you are trying to solve here? > > The update throttle being visible by the guest was done intentionally, > so the guest can throttle the display updates too in case nobody is > watching those display updated anyway. Indeed, we are throttling the update for vnc to avoid some worthless work. But typically a guest cannot respond to update interval changes so often because real display devices the guest is designed for does not change the update interval in that way. That is why we have to tell the guest a stable update interval even if it results in wasted frames. Regards, Akihiko Odaki > > take care, > Gerd >
Re: [PATCH] virtio-gpu: Respect graphics update interval for EDID
On Sun, Feb 21, 2021 at 10:34:14PM +0900, Akihiko Odaki wrote: > This change introduces an additional member, refresh_rate to > qemu_edid_info in include/hw/display/edid.h. > > This change also isolates the graphics update interval from the > display update interval. The guest will update the frame buffer > in the graphics update interval, but displays can be updated in a > dynamic interval, for example to save update costs aggresively > (vnc) or to respond to user-generated events (sdl). > It stabilizes the graphics update interval and prevents the guest > from being confused. Hmm. What problem you are trying to solve here? The update throttle being visible by the guest was done intentionally, so the guest can throttle the display updates too in case nobody is watching those display updated anyway. take care, Gerd
