Re: [PATCH v2 1/5] virtio-gpu: Support the configurable EDID fields

[Markus Armbruster <armbru@redhat.com>]

Roland Clobus <rclobus@rclobus.nl> writes:

> Adds support for the fields that are currently configurable
> in the function qemu_edid_generate.
>
> * The serial number does not need to be numerical.
> See https://en.wikipedia.org/wiki/Extended_Display_Identification_Data
>
> * The name can be optional, there are now many more fields to set
>
> Signed-off-by: Roland Clobus <rclobus@rclobus.nl>

[...]

> diff --git a/qapi/virtio.json b/qapi/virtio.json
> index 09dd0e6d05..791d2f30f1 100644
> --- a/qapi/virtio.json
> +++ b/qapi/virtio.json
> @@ -970,24 +970,62 @@
>  ##
>  # @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 values are set, they take precedence over the root virtio-gpu
> +# configuration.
> +# If none of @xres and @yres are set, the root virtio-gpu resolution
> +# configuration is used and only the first output is enabled.
> +#
> +# @vendor: a 3 letter code for the vendor (since xx.x TODO)
> +#          Default: "RHT" (Red Hat, Inc.)

You need to replace the TODO placeholders before we can merge the patch.

Posting patches that are not ready for merge is okay, but please tag
them with RFC, and explain why in the cover letter.

>  #
>  # @name: the name of the output
> +#        Default: "QEMU Monitor"
> +#
> +# @serial: the serial number (since xx.x TODO)

What's the default?

> +#
> +# @widthmm: width of the display in mm (since xx.x TODO)

Contradiction: "in mm" vs.

> +#           If @widthmm is set, @heightmm must be set too
> +#           If both @widthmm and @heightmm are unset,
> +#             100 DPI is assumed

"DPI".  What's the unit?

Does the default apply to both widthmm and heightmm?

Also, fails to build:

    [77/78] Generating docs/QEMU manual with a custom command
    FAILED: [code=1] docs/docs.stamp 
    /usr/bin/env CONFDIR=etc/qemu /work/armbru/qemu/bld-docs/pyvenv/bin/sphinx-build -q -j auto -W -Dkerneldoc_werror=1 -Dversion=11.0.90 -Drelease= -Ddepfile=docs/docs.d -Ddepfile_stamp=docs/docs.stamp -b html -d /work/armbru/qemu/bld-docs/docs/manual.p /work/armbru/qemu/docs /work/armbru/qemu/bld-docs/docs/manual
    /work/armbru/qemu/docs/../qapi/virtio.json:973: ERROR: Unexpected indentation. [docutils]

With the indentation fixed, this gets rendered like

    • widthmm (int, optional) – width of the display in mm (since xx.x
      TODO) If widthmm is set, heightmm must be set too If both widthmm
      and heightmm are unset, 100 DPI is assumed

End your sentences with punctuation, and separate your paragraphs with
blank lines, please.

> +#
> +# @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.

>  #
>  # @xres: horizontal resolution of the output in pixels (since 11.0)
> +#        If @xres is set, @yres must be set too
>  #
>  # @yres: vertical resolution of the output in pixels (since 11.0)
> +#        If @yres is set, @xres must be set too

Defaults?

> +#
> +# @xmax: maximum horizontal resolution of the output in pixels
> +#        If @xmax is set, @ymax must be set too
> +#        (since xx.x TODO)
> +#
> +# @ymax: maximum vertical resolution of the output in pixels
> +#        If @ymax is set, @xmax must be set too
> +#        (since xx.x TODO)

Defaults?

> +#
> +# @refreshrate: refreshrate in mHz (since xx.x TODO)
> +#               Default: 75000
> +#
> +# .. note:: keep the defaults in sync with hw/display/edid-generate.c
>  #
>  # 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',
> +            '*refreshrate': 'uint32' } }
>  
>  ##
>  # @DummyVirtioForceArrays: