Re: [Qemu-devel] [PATCH v2 1/3] vfio/display: add edid support.

[Alex Williamson]

On Thu, 21 Feb 2019 08:38:50 +0100
Gerd Hoffmann <address@hidden> wrote:

> On Wed, Feb 20, 2019 at 02:54:35PM -0700, Alex Williamson wrote:
> > On Wed, 20 Feb 2019 09:47:51 +0100
> > Gerd Hoffmann <address@hidden> wrote:
> >   
> > > This patch adds EDID support to the vfio display (aka vgpu) code.
> > > When supported by the mdev driver qemu will generate a EDID blob
> > > and pass it on using the new vfio edid region.  The EDID blob will
> > > be updated on UI changes (i.e. window resize), so the guest can
> > > adapt.  
> > 
> > What are the requirements to enable this resizing feature?  I grabbed
> > the gvt-next-2019-02-01 branch and my ever expanding qemu:commandline
> > now looks like this:
> > 
> >   <qemu:commandline>
> >     <qemu:arg value='-set'/>
> >     <qemu:arg value='device.hostdev0.x-igd-opregion=on'/>
> >     <qemu:arg value='-set'/>
> >     <qemu:arg value='device.hostdev0.ramfb=on'/>
> >     <qemu:arg value='-set'/>
> >     <qemu:arg value='device.hostdev0.driver=vfio-pci-nohotplug'/>
> >     <qemu:arg value='-set'/>
> >     <qemu:arg value='device.hostdev0.xres=1600'/>
> >     <qemu:arg value='-set'/>
> >     <qemu:arg value='device.hostdev0.yres=900'/>
> >   </qemu:commandline>
> > 
> > Other relevant sections:
> > 
> >     <graphics type='spice'>
> >       <listen type='none'/>
> >       <gl enable='yes' 
> > rendernode='/dev/dri/by-path/pci-0000:00:02.0-render'/>
> >     </graphics>  
> 
> When using spice you also need the spicevmc channel and the spice agent
> being installed and active in the guest.

This is the vdagent using installation like this:

https://www.ovirt.org/develop/infra/testing/spice.html

ie. vdservice install, net start vdservice?

I'm not seeing anything magically change when I do that.  I do have the
default serial and redirection devices installed by virt-manager:

     <serial type='pty'>
      <source path='/dev/pts/4'/>
      <target type='isa-serial' port='0'>
        <model name='isa-serial'/>
      </target>
      <alias name='serial0'/>
    </serial>
    <console type='pty' tty='/dev/pts/4'>
      <source path='/dev/pts/4'/>
      <target type='serial' port='0'/>
      <alias name='serial0'/>
    </console>
    <redirdev bus='usb' type='spicevmc'>
      <alias name='redir0'/>
      <address type='usb' bus='0' port='2'/>
    </redirdev>
    <redirdev bus='usb' type='spicevmc'>
      <alias name='redir1'/>
      <address type='usb' bus='0' port='3'/>
    </redirdev>

I believe I have drivers installed, but it's a bit difficult to verify
given the Intel graphics glitches I copied you on.

> > > +    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
> > > +    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
> > > +    trace_vfio_display_edid_link_up();
> > > +    return;
> > > +
> > > +err:
> > > +    trace_vfio_display_edid_write_error();
> > > +    return;  
> > 
> > nit, no unwind and only one call point, could probably do without the
> > goto.  
> 
> Not that easily due to the goto being hidden in the pwrite_field()
> macro.

Ah, I knew those macros were going to be tricky, but I fell for it
anyway.  Sorry.

> > > +    trace_vfio_display_edid_available();
> > > +    dpy->edid_regs = g_new0(struct vfio_region_gfx_edid, 1);
> > > +    pread_field(fd, dpy->edid_info, dpy->edid_regs, edid_offset);
> > > +    pread_field(fd, dpy->edid_info, dpy->edid_regs, edid_max_size);
> > > +    pread_field(fd, dpy->edid_info, dpy->edid_regs, max_xres);
> > > +    pread_field(fd, dpy->edid_info, dpy->edid_regs, max_yres);
> > > +    dpy->edid_blob = g_malloc0(dpy->edid_regs->edid_max_size);
> > > +
> > > +    vfio_display_edid_update(vdev, true, 0, 0);
> > > +    return;
> > > +
> > > +err:
> > > +    fprintf(stderr, "%s: Oops, pread error\n", __func__);
> > > +    g_free(dpy->edid_regs);
> > > +    dpy->edid_regs = NULL;
> > > +    return;  
> > 
> > This code is unreachable.  
> 
> It's not.  Again, the goto is in pread_field.
> 
> But I just noticed I missed one fprintf which should be a
> trace_vfio_display_edid_write_error() ...

Yep.