This adds support to the virtio-gpu get_edid command when using the
vhost-user-gpu implementation in contrib/.
So far, qemu has been outputting the following message:
  EDID requested but the backend doesn't support it.
when using that implementation.

This is tested with vhost-user-gpu, the dbus ui backend and the
monitor-edid application, which now shows complete "QEMU Monitor" edid
data.

In this v1, I would appreciate some feedback especially regarding:
- Can we enable it by default or do need to create another config option
  flag for it?
- Can we now also remove the "EDID requested but the backend doesn't
  support it." warning and logic from hw/display or do we still want to
  keep that around for other potential implementations of
  vhost-user-gpu?
- The structs used as payloads of the vhost-user-gpu messages. Looks
  like there was no command so far requiring bidirectional messages with
  different payloads so I just based it on similar available ones.

Thanks


Erico Nunes (2):
  virtio-gpu: refactor generate_edid function to virtio_gpu_base
  vhost-user-gpu: implement get_edid feature

 contrib/vhost-user-gpu/vhost-user-gpu.c | 53 ++++++++++++++++++++++++-
 contrib/vhost-user-gpu/virgl.c          |  3 ++
 contrib/vhost-user-gpu/vugpu.h          |  8 ++++
 docs/interop/vhost-user-gpu.rst         |  9 +++++
 hw/display/vhost-user-gpu.c             | 31 +++++++++++++++
 hw/display/virtio-gpu-base.c            | 17 ++++++++
 hw/display/virtio-gpu.c                 | 20 +---------
 include/hw/virtio/virtio-gpu.h          |  2 +
 8 files changed, 122 insertions(+), 21 deletions(-)

-- 
2.39.2

Hi

On Thu, May 11, 2023 at 5:02 PM Erico Nunes <ernunes@redhat.com> wrote:

> This adds support to the virtio-gpu get_edid command when using the
> vhost-user-gpu implementation in contrib/.
> So far, qemu has been outputting the following message:
>   EDID requested but the backend doesn't support it.
> when using that implementation.
>
> This is tested with vhost-user-gpu, the dbus ui backend and the
> monitor-edid application, which now shows complete "QEMU Monitor" edid
> data.
>
> In this v1, I would appreciate some feedback especially regarding:
> - Can we enable it by default or do need to create another config option
>   flag for it?
>

Enabled as default is ok I think


> - Can we now also remove the "EDID requested but the backend doesn't
>   support it." warning and logic from hw/display or do we still want to
>   keep that around for other potential implementations of
>   vhost-user-gpu?
>

Imho, that should remain. (vhost-user-gpu could have set edid=false by
default to avoid this error, but then it would need to be explicitly turned
on to match the backend implementation)


> - The structs used as payloads of the vhost-user-gpu messages. Looks
>   like there was no command so far requiring bidirectional messages with
>   different payloads so I just based it on similar available ones.
>
>
That looks fine to me


> Thanks
>
>
> Erico Nunes (2):
>   virtio-gpu: refactor generate_edid function to virtio_gpu_base
>   vhost-user-gpu: implement get_edid feature
>
>  contrib/vhost-user-gpu/vhost-user-gpu.c | 53 ++++++++++++++++++++++++-
>  contrib/vhost-user-gpu/virgl.c          |  3 ++
>  contrib/vhost-user-gpu/vugpu.h          |  8 ++++
>  docs/interop/vhost-user-gpu.rst         |  9 +++++
>  hw/display/vhost-user-gpu.c             | 31 +++++++++++++++
>  hw/display/virtio-gpu-base.c            | 17 ++++++++
>  hw/display/virtio-gpu.c                 | 20 +---------
>  include/hw/virtio/virtio-gpu.h          |  2 +
>  8 files changed, 122 insertions(+), 21 deletions(-)
>
> --
> 2.39.2
>
>
>
I wonder if the backend couldn't have avoided the need for calling the
front-end (the VHOST_USER_GPU_GET_EDID call). But after all, it can still
implement it on its own, so it's optional anyway.

However, I worry about using the new backend (calling GET_EDID) with an
older front-end/QEMU. It may just hang, since
vhost_user_gpu_handle_display() won't reply to unknown messages. That's
what PROTOCOL_FEATURES were meant for, iirc. Can you check? thanks


-- 
Marc-André Lureau

On 15/05/2023 13:38, Marc-André Lureau wrote:
> However, I worry about using the new backend (calling GET_EDID) with an
> older front-end/QEMU. It may just hang, since
> vhost_user_gpu_handle_display() won't reply to unknown messages. That's
> what PROTOCOL_FEATURES were meant for, iirc. Can you check? thanks

Indeed as you say, there is a hang with older qemu.

From what I see there are the generic protocol_features and also a
vhost-user-gpu message for them. I assume it is so that we don't have to
add vhost-user-gpu specific features to the generic set?
In any case, the current vhost-user-gpu specific protocol_features
negotiation happens too late to enable or disable virtio-gpu features
(triggered by VHOST_USER_GPU_SET_SOCKET). I suppose we could move it
earlier to the time the generic protocol_features are negotiated,
through the callback hooks that already exist in the vhost-user layer
(not implemented so far by vhost-user-gpu though).
So I guess we could remove the protocol_features negotiation that is
currently triggered by VHOST_USER_GPU_SET_SOCKET and use that to prevent
exposing VIRTIO_GPU_F_EDID at all. Does that make sense?

Otherwise, if we keep exposing VIRTIO_GPU_F_EDID and just not sending
VHOST_USER_GPU_GET_EDID then the get_edid feature is not quite
functional overall, which doesn't sound too great.

Thanks

Erico

Hi Erico

On Wed, May 17, 2023 at 8:09 PM Erico Nunes <ernunes@redhat.com> wrote:

> On 15/05/2023 13:38, Marc-André Lureau wrote:
> > However, I worry about using the new backend (calling GET_EDID) with an
> > older front-end/QEMU. It may just hang, since
> > vhost_user_gpu_handle_display() won't reply to unknown messages. That's
> > what PROTOCOL_FEATURES were meant for, iirc. Can you check? thanks
>
> Indeed as you say, there is a hang with older qemu.
>
> From what I see there are the generic protocol_features and also a
> vhost-user-gpu message for them. I assume it is so that we don't have to
> add vhost-user-gpu specific features to the generic set?
> In any case, the current vhost-user-gpu specific protocol_features
> negotiation happens too late to enable or disable virtio-gpu features
> (triggered by VHOST_USER_GPU_SET_SOCKET). I suppose we could move it
> earlier to the time the generic protocol_features are negotiated,
> through the callback hooks that already exist in the vhost-user layer
> (not implemented so far by vhost-user-gpu though).
> So I guess we could remove the protocol_features negotiation that is
> currently triggered by VHOST_USER_GPU_SET_SOCKET and use that to prevent
> exposing VIRTIO_GPU_F_EDID at all. Does that make sense?
>
>
Wouldn't this work?

If VIRTIO_GPU_F_EDID is set and during protocol_features the GET_EDID
feature is not negotiated, exit the gpu backend with an error.

Otherwise, if we keep exposing VIRTIO_GPU_F_EDID and just not sending
> VHOST_USER_GPU_GET_EDID then the get_edid feature is not quite
> functional overall, which doesn't sound too great.
>
> Thanks
>
> Erico
>
>

-- 
Marc-André Lureau

On 24/05/2023 13:23, Marc-André Lureau wrote:
> Hi Erico
> 
> On Wed, May 17, 2023 at 8:09 PM Erico Nunes <ernunes@redhat.com
> <mailto:ernunes@redhat.com>> wrote:
> 
>     On 15/05/2023 13:38, Marc-André Lureau wrote:
>     > However, I worry about using the new backend (calling GET_EDID)
>     with an
>     > older front-end/QEMU. It may just hang, since
>     > vhost_user_gpu_handle_display() won't reply to unknown messages.
>     That's
>     > what PROTOCOL_FEATURES were meant for, iirc. Can you check? thanks
> 
>     Indeed as you say, there is a hang with older qemu.
> 
>     From what I see there are the generic protocol_features and also a
>     vhost-user-gpu message for them. I assume it is so that we don't have to
>     add vhost-user-gpu specific features to the generic set?
>     In any case, the current vhost-user-gpu specific protocol_features
>     negotiation happens too late to enable or disable virtio-gpu features
>     (triggered by VHOST_USER_GPU_SET_SOCKET). I suppose we could move it
>     earlier to the time the generic protocol_features are negotiated,
>     through the callback hooks that already exist in the vhost-user layer
>     (not implemented so far by vhost-user-gpu though).
>     So I guess we could remove the protocol_features negotiation that is
>     currently triggered by VHOST_USER_GPU_SET_SOCKET and use that to prevent
>     exposing VIRTIO_GPU_F_EDID at all. Does that make sense?
> 
> 
> Wouldn't this work?
> 
> If VIRTIO_GPU_F_EDID is set and during protocol_features the GET_EDID
> feature is not negotiated, exit the gpu backend with an error.

I sent v2 implementing it this way.
It seems that qemu always requests the virtio feature though (even if
you do pass edid=off to the device, it just doesn't expose it to the
user but does request the feature bit). So given we can't change past
qemu builds I'm not sure if there is a way to make an older qemu with an
updated vhost-user-gpu work at all. The new implementation does print an
error message in that case though.

Thanks

Erico