| Message ID | 20230511125803.594963-1-ernunes@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | vhost-user-gpu get_edid feature | expand |
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
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 > >
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