[v1] add a refresh_rate option to virtio-gpu

[PATCH] add a refresh_rate option to virtio-gpu

Posted by slonkazoid via qemu development 2 weeks, 1 day ago

Signed-off-by: slonkazoid <slonkazoid@slonk.ing>
---
currently, the virtio-gpu module does not set a refresh rate, making it
default to 75Hz in the qemu edid generation logic. i wrote a sloppy patch
that adds a `refresh_rate` option to virtio-gpu, which propagates to all
the devices based on it (i was interested in virtio-vga-gl). it's not of
much use by itself but when combined with something like Sunshine or
looking-glass, it can provide *far* smoother guest graphics than the
inbuilt display devices. i have done a bare minimum amount of manual
testing (virtio-vga-gl with venus XOR amdgpu drm native context streaming
video games) and it functions as intended, and should not break the code
path to fetch the refresh rate from the active display device (?), however,
i have never contributed to QEMU before and i don't know the customs
concerning testing, documentation, etc. if there's more i should do, please
tell me! if this is something that would benefit QEMU, feel free to merge it.
it's probably not a copyrightable change anyways.
---
 hw/display/virtio-gpu-base.c   | 1 +
 include/hw/virtio/virtio-gpu.h | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index cb76302e2d..d87f6604d0 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -232,6 +232,7 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
 
     g->req_state[0].width = g->conf.xres;
     g->req_state[0].height = g->conf.yres;
+    g->req_state[0].refresh_rate = g->conf.refresh_rate;
 
     for (output_idx = 0, node = g->conf.outputs;
          node && output_idx < g->conf.max_outputs;
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index f69fc19462..66cf921045 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -131,6 +131,7 @@ struct virtio_gpu_base_conf {
     uint32_t flags;
     uint32_t xres;
     uint32_t yres;
+    uint32_t refresh_rate;
     uint64_t hostmem;
     VirtIOGPUOutputList *outputs;
 };
@@ -176,7 +177,8 @@ struct VirtIOGPUBaseClass {
     DEFINE_PROP_BIT("edid", _state, _conf.flags, \
                     VIRTIO_GPU_FLAG_EDID_ENABLED, true), \
     DEFINE_PROP_UINT32("xres", _state, _conf.xres, 1280), \
-    DEFINE_PROP_UINT32("yres", _state, _conf.yres, 800)
+    DEFINE_PROP_UINT32("yres", _state, _conf.yres, 800), \
+    DEFINE_PROP_UINT32("refresh_rate", _state, _conf.refresh_rate, 60000)
 
 typedef struct VGPUDMABuf {
     QemuDmaBuf *buf;

---
base-commit: 985062e2319e6bf4a4be9c216ede6d34379e5777
change-id: 20260414-virtio-gpu-refresh-rate-37b388e7dd72

Best regards,
-- 
slonkazoid <slonkazoid@slonk.ing>

Re: [PATCH] add a refresh_rate option to virtio-gpu

Posted by Akihiko Odaki 2 weeks ago

On 2026/04/15 3:39, slonkazoid wrote:
> Signed-off-by: slonkazoid <slonkazoid@slonk.ing>
> ---
> currently, the virtio-gpu module does not set a refresh rate, making it
> default to 75Hz in the qemu edid generation logic. i wrote a sloppy patch
> that adds a `refresh_rate` option to virtio-gpu, which propagates to all
> the devices based on it (i was interested in virtio-vga-gl). it's not of
> much use by itself but when combined with something like Sunshine or
> looking-glass, it can provide *far* smoother guest graphics than the
> inbuilt display devices. i have done a bare minimum amount of manual
> testing (virtio-vga-gl with venus XOR amdgpu drm native context streaming
> video games) and it functions as intended, and should not break the code
> path to fetch the refresh rate from the active display device (?), however,
> i have never contributed to QEMU before and i don't know the customs
> concerning testing, documentation, etc. if there's more i should do, please
> tell me! if this is something that would benefit QEMU, feel free to merge it.


Hi,

I think it's fine as long as you confirmed the change actually works.

> it's probably not a copyrightable change anyways.
> ---
>   hw/display/virtio-gpu-base.c   | 1 +
>   include/hw/virtio/virtio-gpu.h | 4 +++-
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> index cb76302e2d..d87f6604d0 100644
> --- a/hw/display/virtio-gpu-base.c
> +++ b/hw/display/virtio-gpu-base.c
> @@ -232,6 +232,7 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
>   
>       g->req_state[0].width = g->conf.xres;
>       g->req_state[0].height = g->conf.yres;
> +    g->req_state[0].refresh_rate = g->conf.refresh_rate;
>   
>       for (output_idx = 0, node = g->conf.outputs;
>            node && output_idx < g->conf.max_outputs;
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index f69fc19462..66cf921045 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -131,6 +131,7 @@ struct virtio_gpu_base_conf {
>       uint32_t flags;
>       uint32_t xres;
>       uint32_t yres;
> +    uint32_t refresh_rate;
>       uint64_t hostmem;
>       VirtIOGPUOutputList *outputs;
>   };
> @@ -176,7 +177,8 @@ struct VirtIOGPUBaseClass {
>       DEFINE_PROP_BIT("edid", _state, _conf.flags, \
>                       VIRTIO_GPU_FLAG_EDID_ENABLED, true), \
>       DEFINE_PROP_UINT32("xres", _state, _conf.xres, 1280), \
> -    DEFINE_PROP_UINT32("yres", _state, _conf.yres, 800)
> +    DEFINE_PROP_UINT32("yres", _state, _conf.yres, 800), \
> +    DEFINE_PROP_UINT32("refresh_rate", _state, _conf.refresh_rate, 60000)

Please keep the old default value. Not only it is a good practice for 
usability, it is a requirement for migration compatibility.
(c.f., docs/devel/migration/compatibility.rst)

I prefer having a macro to define the default value and share it here 
and at edid-generate.c. (ideally xres and yres should have such macros, 
but it is out of scope of this change so you are not required to add them.)

Regards,
Akihiko Odaki

>   
>   typedef struct VGPUDMABuf {
>       QemuDmaBuf *buf;
> 
> ---
> base-commit: 985062e2319e6bf4a4be9c216ede6d34379e5777
> change-id: 20260414-virtio-gpu-refresh-rate-37b388e7dd72
> 
> Best regards,

Re: [PATCH] add a refresh_rate option to virtio-gpu

Posted by BALATON Zoltan 2 weeks ago

On Wed, 15 Apr 2026, Akihiko Odaki wrote:
> On 2026/04/15 3:39, slonkazoid wrote:
>> Signed-off-by: slonkazoid <slonkazoid@slonk.ing>
>> ---
>> currently, the virtio-gpu module does not set a refresh rate, making it
>> default to 75Hz in the qemu edid generation logic. i wrote a sloppy patch
>> that adds a `refresh_rate` option to virtio-gpu, which propagates to all
>> the devices based on it (i was interested in virtio-vga-gl). it's not of
>> much use by itself but when combined with something like Sunshine or
>> looking-glass, it can provide *far* smoother guest graphics than the
>> inbuilt display devices. i have done a bare minimum amount of manual
>> testing (virtio-vga-gl with venus XOR amdgpu drm native context streaming
>> video games) and it functions as intended, and should not break the code
>> path to fetch the refresh rate from the active display device (?), however,
>> i have never contributed to QEMU before and i don't know the customs
>> concerning testing, documentation, etc. if there's more i should do, please
>> tell me! if this is something that would benefit QEMU, feel free to merge 
>> it.
>
>
> Hi,
>
> I think it's fine as long as you confirmed the change actually works.
>
>> it's probably not a copyrightable change anyways.
>> ---
>>   hw/display/virtio-gpu-base.c   | 1 +
>>   include/hw/virtio/virtio-gpu.h | 4 +++-
>>   2 files changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
>> index cb76302e2d..d87f6604d0 100644
>> --- a/hw/display/virtio-gpu-base.c
>> +++ b/hw/display/virtio-gpu-base.c
>> @@ -232,6 +232,7 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
>>         g->req_state[0].width = g->conf.xres;
>>       g->req_state[0].height = g->conf.yres;
>> +    g->req_state[0].refresh_rate = g->conf.refresh_rate;
>>         for (output_idx = 0, node = g->conf.outputs;
>>            node && output_idx < g->conf.max_outputs;
>> diff --git a/include/hw/virtio/virtio-gpu.h 
>> b/include/hw/virtio/virtio-gpu.h
>> index f69fc19462..66cf921045 100644
>> --- a/include/hw/virtio/virtio-gpu.h
>> +++ b/include/hw/virtio/virtio-gpu.h
>> @@ -131,6 +131,7 @@ struct virtio_gpu_base_conf {
>>       uint32_t flags;
>>       uint32_t xres;
>>       uint32_t yres;
>> +    uint32_t refresh_rate;
>>       uint64_t hostmem;
>>       VirtIOGPUOutputList *outputs;
>>   };
>> @@ -176,7 +177,8 @@ struct VirtIOGPUBaseClass {
>>       DEFINE_PROP_BIT("edid", _state, _conf.flags, \
>>                       VIRTIO_GPU_FLAG_EDID_ENABLED, true), \
>>       DEFINE_PROP_UINT32("xres", _state, _conf.xres, 1280), \
>> -    DEFINE_PROP_UINT32("yres", _state, _conf.yres, 800)
>> +    DEFINE_PROP_UINT32("yres", _state, _conf.yres, 800), \
>> +    DEFINE_PROP_UINT32("refresh_rate", _state, _conf.refresh_rate, 60000)
>
> Please keep the old default value. Not only it is a good practice for 
> usability, it is a requirement for migration compatibility.
> (c.f., docs/devel/migration/compatibility.rst)
>
> I prefer having a macro to define the default value and share it here and at 
> edid-generate.c. (ideally xres and yres should have such macros, but it is 
> out of scope of this change so you are not required to add them.)

There is a DEFINE_EDID_PROPERTIES macro in include/hw/display/edid.h which 
may be used but I'm not sure that sets the same defaults.

Regards,
BALATON Zoltan

> Regards,
> Akihiko Odaki
>
>>     typedef struct VGPUDMABuf {
>>       QemuDmaBuf *buf;
>> 
>> ---
>> base-commit: 985062e2319e6bf4a4be9c216ede6d34379e5777
>> change-id: 20260414-virtio-gpu-refresh-rate-37b388e7dd72
>> 
>> Best regards,
>
>
>

Re: [PATCH] add a refresh_rate option to virtio-gpu

Posted by Akihiko Odaki 2 weeks ago

On 2026/04/15 21:48, BALATON Zoltan wrote:
> On Wed, 15 Apr 2026, Akihiko Odaki wrote:
>> On 2026/04/15 3:39, slonkazoid wrote:
>>> Signed-off-by: slonkazoid <slonkazoid@slonk.ing>
>>> ---
>>> currently, the virtio-gpu module does not set a refresh rate, making it
>>> default to 75Hz in the qemu edid generation logic. i wrote a sloppy 
>>> patch
>>> that adds a `refresh_rate` option to virtio-gpu, which propagates to all
>>> the devices based on it (i was interested in virtio-vga-gl). it's not of
>>> much use by itself but when combined with something like Sunshine or
>>> looking-glass, it can provide *far* smoother guest graphics than the
>>> inbuilt display devices. i have done a bare minimum amount of manual
>>> testing (virtio-vga-gl with venus XOR amdgpu drm native context 
>>> streaming
>>> video games) and it functions as intended, and should not break the code
>>> path to fetch the refresh rate from the active display device (?), 
>>> however,
>>> i have never contributed to QEMU before and i don't know the customs
>>> concerning testing, documentation, etc. if there's more i should do, 
>>> please
>>> tell me! if this is something that would benefit QEMU, feel free to 
>>> merge it.
>>
>>
>> Hi,
>>
>> I think it's fine as long as you confirmed the change actually works.
>>
>>> it's probably not a copyrightable change anyways.
>>> ---
>>>   hw/display/virtio-gpu-base.c   | 1 +
>>>   include/hw/virtio/virtio-gpu.h | 4 +++-
>>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
>>> index cb76302e2d..d87f6604d0 100644
>>> --- a/hw/display/virtio-gpu-base.c
>>> +++ b/hw/display/virtio-gpu-base.c
>>> @@ -232,6 +232,7 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
>>>         g->req_state[0].width = g->conf.xres;
>>>       g->req_state[0].height = g->conf.yres;
>>> +    g->req_state[0].refresh_rate = g->conf.refresh_rate;
>>>         for (output_idx = 0, node = g->conf.outputs;
>>>            node && output_idx < g->conf.max_outputs;
>>> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/ 
>>> virtio-gpu.h
>>> index f69fc19462..66cf921045 100644
>>> --- a/include/hw/virtio/virtio-gpu.h
>>> +++ b/include/hw/virtio/virtio-gpu.h
>>> @@ -131,6 +131,7 @@ struct virtio_gpu_base_conf {
>>>       uint32_t flags;
>>>       uint32_t xres;
>>>       uint32_t yres;
>>> +    uint32_t refresh_rate;
>>>       uint64_t hostmem;
>>>       VirtIOGPUOutputList *outputs;
>>>   };
>>> @@ -176,7 +177,8 @@ struct VirtIOGPUBaseClass {
>>>       DEFINE_PROP_BIT("edid", _state, _conf.flags, \
>>>                       VIRTIO_GPU_FLAG_EDID_ENABLED, true), \
>>>       DEFINE_PROP_UINT32("xres", _state, _conf.xres, 1280), \
>>> -    DEFINE_PROP_UINT32("yres", _state, _conf.yres, 800)
>>> +    DEFINE_PROP_UINT32("yres", _state, _conf.yres, 800), \
>>> +    DEFINE_PROP_UINT32("refresh_rate", _state, _conf.refresh_rate, 
>>> 60000)
>>
>> Please keep the old default value. Not only it is a good practice for 
>> usability, it is a requirement for migration compatibility.
>> (c.f., docs/devel/migration/compatibility.rst)
>>
>> I prefer having a macro to define the default value and share it here 
>> and at edid-generate.c. (ideally xres and yres should have such 
>> macros, but it is out of scope of this change so you are not required 
>> to add them.)
> 
> There is a DEFINE_EDID_PROPERTIES macro in include/hw/display/edid.h 
> which may be used but I'm not sure that sets the same defaults.

The definition sets all properties zero. The zeros will be properly 
replaced with the real default values in qemu_edid_generate(), but it 
still has a downside: the default value shown in -device virtio-gpu,help 
will stop showing the real default values.

So, it is nice (but not required) to use DEFINE_EDID_PROPERTIES(), but 
its default values should be changed to avoid regressing the help message.

Regards,
Akihiko Odaki

> 
> Regards,
> BALATON Zoltan
> 
>> Regards,
>> Akihiko Odaki
>>
>>>     typedef struct VGPUDMABuf {
>>>       QemuDmaBuf *buf;
>>>
>>> ---
>>> base-commit: 985062e2319e6bf4a4be9c216ede6d34379e5777
>>> change-id: 20260414-virtio-gpu-refresh-rate-37b388e7dd72
>>>
>>> Best regards,
>>
>>
>>