Re: [PATCH 2/2] hw/display: Allow injection of virtio-gpu EDID name

[Daniel P . Berrangé]

CC Markus to keep me honest in my comments below

On Mon, Dec 02, 2024 at 03:31:53PM -0500, Andrew Keesler wrote:
> Hi again Daniel. I have a follow up question. Can you help me
> understand how I can declare this "outputs" property?
> 
>    -device '{"driver":"virtio-vga",
>              "max_outputs":2,
>              "id":"vga",
>              "outputs":[
>                {
>                   "name":"AAA",
>                },
>                {
>                   "name":"BBB",
>                },
>              ]}'
> 
> I thought DEFINE_PROP_ARRAY would do it, but I can't tell what PropertyInfo
> implementation I should pass. All of the PropertyInfo implementations I can
> find use scalar types, or simple text decoding. I am wondering if I am
> missing
> some sort of "JSON" encoding capabilities that can happen behind the scenes.

I could have sworn we had an example of how to handle this already,
but I'm not finding any Device class with a non-scalar property
that isn't merely an array of scalars.

We definitely have some examples elsewhere for exmaple "Machine" class
has an SmpCacheProperties array property, and the QAuthZList class
has an array of "QAuthZListRule" property.

In both cases the struct is defined in th qapi/<blah>.json, which
auto-generates code eg visit_type_QAuthZListRuleList, which can
then get called from qauthz_list_prop_get_rules and
qauthz_list_prop_set_rules, for the property.

Devices use a slightly higher level wrapper so instead of calling
object_class_property_add directly, then define the PropertyInfo
and object_class_property_add gets called indirectly for them.
I'm thinking it should still be possible to use the QAPI code
generator to help though. You could either just define the struct,
and thn use that to create  PropertyInfo to be used in combination
with DEFINE_PROP_ARRAY, of you could define a list of structs at
the QAPI level and use plain DEFINE_PROP. I guess the former is
probably better aligned with other Device code.

> 
> On Tue, Nov 26, 2024 at 4:07 PM Andrew Keesler <ankeesler@google.com> wrote:
> 
> > Thanks, Daniel. We'll get this patch updated and send it out again.
> >
> > > it makes sense to allow for a data structure
> >
> > Whoops, I misread your original message - data structure SGTM.
> >
> > On Tue, Nov 26, 2024 at 11:04 AM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> >> On Mon, Nov 25, 2024 at 03:54:40PM -0500, Andrew Keesler wrote:
> >> > I follow what you are saying. I misunderstood what a "display" was in
> >> the
> >> > domain of QEMU. Yes, this makes more sense now.
> >> >
> >> > > the user should give names for every output at startup
> >> >
> >> > I see DEFINE_PROP_ARRAY exists. I can use that to define the new
> >> "outputs"
> >> > property. Any reason that each "output" would ever need to be an object
> >> > (rather than just a string)? Nothing comes to mind, I'm just taking a
> >> second
> >> > to think about API forwards compatibility.
> >>
> >> Currently we have 'xres' and 'yres' properties set against the device
> >> for virtio-gpu.
> >>
> >> If we're going to extend  it to allow the name of each "output" head
> >> to be configured, it makes sense to allow for a data structure that
> >> will let us also cnofigure xres & yres per output.
> >>
> >> Hence, I thought it would make more sense to have an array of structs,
> >> rather than the simpler array of strings, which will let us set any
> >> amount of per-output config data we might want in future.
> >>
> >> NB, I'm not asking you to wire up support for xres/yres per output,
> >> just that we anticipate it as a possibility.
> >>
> >> > > upto whatever they said for "max_outputs"
> >> >
> >> > Where is the best place to perform this validation? I would imagine we
> >> would
> >> > want to fast-fail if the user provided more "outputs" than
> >> "max_outputs". I
> >> > can
> >> > perform the validation in virtio_gpu_base_get_features but that seems
> >> late.
> >>
> >> I'd suggest putting it in virtio_gpu_base_device_realize, as we already
> >> have code there to validate 'max_outputs" is within limits.
> >>
> >>
> >> With regards,
> >> Daniel
> >> --
> >> |: https://berrange.com      -o-
> >> https://www.flickr.com/photos/dberrange :|
> >> |: https://libvirt.org         -o-
> >> https://fstop138.berrange.com :|
> >> |: https://entangle-photo.org    -o-
> >> https://www.instagram.com/dberrange :|
> >>
> >>

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|