Re: [PATCH] virtio-gpu: Support the configurable EDID fields

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Mon, Jun 29, 2026 at 7:20 PM Roland Clobus <rclobus@rclobus.nl> wrote:
>>
>> Hello Marc-André,
>>
>> Thank you for the review. I will prepare changes based on the suggestions in your next answer.
>>
>> On 29/06/2026 13:14, Marc-André Lureau wrote:
>> > On Tue, Jun 23, 2026 at 8:38 PM Roland Clobus <rclobus@rclobus.nl> wrote:
>> >>
>> >> This patch adds support for the fields that are currently configurable
>> >> in the function qemu_edid_generate.
>> >>
>> >> Signed-off-by: Roland Clobus <rclobus@rclobus.nl>

[...]

>> >> diff --git a/qapi/virtio.json b/qapi/virtio.json
>> >> index 1fc4e38a44..e1cd5864fe 100644
>> >> --- a/qapi/virtio.json
>> >> +++ b/qapi/virtio.json
>> >> @@ -952,24 +952,54 @@
>> >>   ##
>> >>   # @VirtIOGPUOutput:
>> >>   #
>> >> -# Describes configuration of a VirtIO GPU output.  If both @xres and
>> >> -# @yres are set, they take precedence over root virtio-gpu resolution
>> >> -# configuration and enable the corresponding output.  If none of @xres
>> >> -# and @yres are set, root virtio-gpu resolution configuration takes
>> >> -# precedence and only the first output is enabled.  Only setting one
>> >> -# of @xres or @yres is an error.
>> >> +# Describes configuration of a VirtIO GPU output.  Matches the fields
>> >> +# of struct @qemu_edid_info.

Blank lines between paragraphs, so they get actually rendered as
separate paragraphs.

If VirtIOGPUOutput matches qemu_edid_info, could it *replace*
qemu_edid_info?

>> >> +# If both @xres and @yres are set, they take precedence over root
>> >> +# virtio-gpu resolution configuration and enable the corresponding
>> >> +# output.  If none of @xres and @yres are set, root
>> >> +# virtio-gpu resolution configuration takes precedence and only the
>> >> +# first output is enabled.  Only setting one of @xres or @yres is
>> >> +# an error.
>> >> +#
>> >> +# @vendor: a 3 letter code for the vendor (since xx.x TODO)
>> >>   #
>> >
>> > It should be uppercase. Validation is missing in qemu_edid_generate() too
>>
>> Agreed. The code does (ch - '@') & 0x1f), so even lowercase will work.
>>
>> I've focussed on adding the missing fields that are consumed by qemu_edid_generate, irrespective whether they were validated or not.
>> I can widen the scope of this change request, to add the missing verification of these fields.
>
> thanks
>
>>
>> > We should also document the default values
>>
>> I can duplicate the default values from qemu_edid_generate. Should there be a comment in both files which recommends to keep both in sync?
>
> a comment in the source code could help, but in general any documented
> default value as a counterpart in the code

QAPI schema doc comments become the "QEMU QMP Reference Manual",
documenting the external QMP interface.  Defaults should be documented,
since they are part of the interface.

Some types there aren't used only for QOM properties, which the QMP
schema doesn't cover.  They end up in the manual anyway, just not
properly connected to commands.  Defaults should be documented
regardless.

Some could be used only for other purposes, but that doesn't concern us
here.

>> >>   # @name: the name of the output
>>
>> Historically name was the first field in the struct VirtIOGPUOutput. Since the scope of this change request is widening, can it now become an optional field, given that every other field can be changed independently?
>>
>
> I think this is backward compatible, so yes
>
>> >>   #
>> >> +# @serial: the serial number (since xx.x TODO)
>> >> +#          must be numerical
>> >
>> > Then why do we accept a string? We should also fix qemu_edid_info then
>>
>> I was also wondering about that (but I tried not to touch the existing code too much). By using uint32, it will never change its data type (and there will be no need for atoi in the generator code).
>>
>
> ok

Using strings for numbers is heavily frowned upon in the QAPI schema.

>> >> +#
>> >> +# @widthmm: width of the display in mm (since xx.x TODO)
>> >> +#           If @widthmm is set, @heightmm must be set too
>> >> +#
>> >> +# @heightmm: height of the display in mm (since xx.x TODO)
>> >> +#           If @heightmm is set, @widthmm must be set too

Please separate words with dashes in member names: width-mm, height-mm.

We don't normally put the unit in the member name.  E.g.

    ##
    # @ChardevVC:
    #
    # Configuration info for virtual console chardevs.
    #
    # @width: console width, in pixels
    #
    # @height: console height, in pixels
    #
    # @cols: console width, in chars
    #
    # @rows: console height, in chars

Perhaps an argument could be made that it's helpful here, because
millimeters is unusual.

>> > Setting one without the other would be weird. Maybe we should have a
>> > struct with both fields mandatory?

This way, the constraint "set both or none" is expressed syntactically,
which is desirable.  However, we have quite a few coordinate pairs
elsewhere, so this introduces inconsistency, which is undesirable.  Hmm.

>> That would make the JSON to set the values slightly more complex, but doable. (I'm also working on a patch that does so, but that needs more work, and would be independent of this change request)
>> But xres/yres will be an (the only?) exception to having a mandatory pair.
>
> we can keep xres/yres for compatibility, but mark them as deprecated?
>
>> >> +#
>> >>   # @xres: horizontal resolution of the output in pixels (since 11.0)
>> >>   #
>> >>   # @yres: vertical resolution of the output in pixels (since 11.0)
>> >
>> > Same, but pre-existing.
>>
>> But here the situation is slightly more complex. If at least one of the pair xres,yres is set to zero, the display is considered inactive. So it would be legal to only set one of them to zero or non-zero to turn the display off or on (enabled_output_bitmask)
>
> we could still mandate both xres/yres to be set in the new struct
>
>>
>> I have a local patch in preparation to allow modifications, but that needs some more work. It is not a blocker for this change request.
>> If that patch is ready, one can modify all fields of this struct on-the-fly, simulating the (dis)connect of a new display to the VM.
>>
>> FYI that would eventually allow something similar to:
>> ```
>> qom-set -j /machine/peripheral/vga001 outputs [ { "yres": 768, "name": "AAA", "widthmm": 40, "serial": "42", "heightmm": 30, "vendor": "IVM", "xmax": 2048, "ymax": 1536, "refreshrate": 50000, "xres": 1024 }, { "yres": 768, "name": "DDD", "widthmm": 1000, "serial": "43", "heightmm": 750, "vendor": "RHT", "xmax": 1024, "ymax": 768, "refreshrate": 60000, "xres": 0 } ]
>> ```
>>
>> But that's not in scope for this change request, where setting the widthmm/heightmm values will be able to trigger HiDPI scenarios.
>>
>
> I see, well you could make this part of the same series imho
>
>> >>   #
>> >> +# @xmax: maximum horizontal resolution of the output in pixels
>> >> +#        (since xx.x TODO)
>> >> +#
>> >> +# @ymax: maximum vertical resolution of the output in pixels
>> >> +#        (since xx.x TODO)
>> >
>> > Same
>>
>> If you prefer a struct of two elements, the names for the value-pair-struct could become: `resolution`, `maxresolution`, `sizemm`. Please say your preference.
>
> wdyt Markus?

Pros and cons, hard to decide.

What are the existing coordinate pairs?

>> >> +#
>> >> +# @refreshrate: refreshrate in mHz (since xx.x TODO)
>> >> +#
>> >>   # Since: 10.1
>> >>   ##
>> >>
>> >>   { 'struct': 'VirtIOGPUOutput',
>> >> -  'data': { 'name': 'str', '*xres': 'uint16', '*yres': 'uint16' } }
>> >> +  'data': { '*vendor': 'str',
>> >> +            'name': 'str',
>> >> +            '*serial': 'str',
>> >> +            '*widthmm': 'uint16',
>> >> +            '*heightmm': 'uint16',
>> >> +            '*xres': 'uint32',
>> >> +            '*yres': 'uint32',
>> >> +            '*xmax': 'uint32',
>> >> +            '*ymax': 'uint32'
>> >
>> > It changes the existing xres/yres fields type.
>> > Is it necessary to accept uint32?
>>
>> g->req_state[0].width (and height) are uint32_t in virtio_gpu_requested_state, and passed along in the generator code as uint32_t, so I updated the struct to match that.
>>
>> But in the end only 16 bits are used in qemu_displayid_generate (`stw_le_p(did + 12, 0xffff & (xres - 1));`)
>
> I think we should stay u16, if that's what EDID is limited to

The choice doesn't affect the external  interface.

Changing integer types in the QAPI schema is invisible externally as
long as the set of accepted values remains the same.

>> >> +            '*refreshrate': 'uint32' } }
>> >>
>> >>   ##
>> >>   # @DummyVirtioForceArrays:
>> >> --
>> >> 2.53.0
>>
>
> thanks