[virtio-dev] Re: [VIRTIO GPU PATCH v3 0/1] Add new feature flag VIRTIO_GPU_F_FREEZE_S3

[Chen, Jiqian]

Hi,

On 2023/8/2 15:13, Parav Pandit wrote:
> 
>> From: Chen, Jiqian 
>> Sent: Wednesday, August 2, 2023 11:28 AM
>>
>> On 2023/8/2 12:49, Parav Pandit wrote:
>>>
>>>
 From: virtio-dev@lists.oasis-open.org
  On Behalf Of Chen, Jiqian
 Sent: Wednesday, August 2, 2023 8:51 AM Hi all,

 Do you have any other comments on the modification of virtio-gpu S3?
 Looking forward to your reply and comments.

>>
>> Hi Parav Pandit,
>> Thank you for your reply. Let me try to answer your question.
>>
>>> I am not familiar with the GPU, so a dumb question is, why the S3 state is 
>>> gpu
>> specific?
>> S3 state is not gpu specific. I think different virtio devices may have 
>> different
>> actions/problems when S3.
> I am making assumption that the gpu device is pci. :)
> If so can you please use the transport specific notification from gpu guest 
> driver to notify to qemu?
In my existing implementation, qemu is notified through it(gpu guest driver 
queue).

> 
>> When I do S3 on Xen, I found guest's display can't come back and the root 
>> cause
>> is in virtio-gpu backend in QEMU.
>> So, to solve that problem, I change codes to let guest notify QEMU 
>> virtio-gpu's
>> suspend state, and then QEMU will not destroy resources that used for 
>> display.
>> Please see attached kernel and QEMU patch links.
>> For above reason, Gerd suggest me to add a new feature flag specifically for
>> virtio-gpu, so that guest and host can negotiate whenever to enable above
>> mechanism.
>>
>>> Can a transport specific suspend state be used and apply to all virtio 
>>> devices?
>> Based on my limited knowledge. Different virtio devices have different virtio
>> queues, my modifications let guest to notify QEMU by using virtio-gpu's 
>> control
>> queue.
>> Other virtio devices can't get that notification unless you can traverse all 
>> virtio
>> devices and notify one by one, or other global method?
>> But for now, this patch is to add a new feature flag just used for 
>> virtio-gpu.
>>
> If this is done at the pci transport level, all the virtio device benefit 
> from it without inventing in each device type.
I think there is no need to improve to pci level. Because this feature is a 
compromise solution,
if resources are destroyed on qemu side, guest has not enough data to re-create 
them,
so we choose to keep them when suspension. That is a virtio-gpu specific 
scenario.

What's your opinion, Gerd Hoffmann, and Robert Beckett? Am I right?

> 
>>> And can you please add both the rationale to the commit message?
>> Sure, I will expand the description of my commit message and add them.
>>
>>>
 On 2023/7/20 20:18, Jiqian Chen wrote:
> v3:
>
> Hi all,
> Thanks for Gerd Hoffmann's advice. V3 makes below changes:
> * Use enum for freeze mode, so this can be extended with more
>   modes in the future.
> * Rename functions and paratemers with "_S3" postfix.
> * Explain in more detail
>
> And latest version on QEMU and Linux kernel side:
>   QEMU: https://lore.kernel.org/qemu-devel/20230720120816.8751-1-
 jiqian.c...@amd.com
>   Kernel:
> https://lore.kernel.org/lkml/20230720115805.8206-1-Jiqian.Chen@amd.c
> om
> /T/#t
>
> Best regards,
> Jiqian Chen.
>
>
> v2:
> link,
> https://lists.oasis-open.org/archives/virtio-comment/202307/msg00160
> .h
> tml
>
> Hi all,
> Thanks to Gerd Hoffmann for his suggestions. V2 makes below changes:
> * Elaborate on the types of resources.
> * Add some descriptions for S3 and S4.
>
>
> v1:
> link,
> https://lists.oasis-open.org/archives/virtio-comment/202306/msg00595
> .h
> tml
>
> Hi all,
> I am working to implement virtgpu S3 function on Xen.
>
> Currently on Xen, if we start a guest through Qemu with enabling
> virtgpu, and then suspend and s3resume guest. We can find that the
> guest kernel comes back, but the display doesn't. It just shown a black
>> screen.
>
> That is because when guest was during suspending, it called into
> Qemu and Qemu destroyed all resources and reset renderer. This made
> the display gone after guest resumed.
>
> So, I add a mechanism that when guest is suspending, it will notify
> Qemu, and then Qemu will not destroy resources. That can help
> guest's display come back.
>
> As discussed and suggested by Robert Beckett and Gerd Hoffmann on v1
> qemu's mailing list. Due to that mechanism needs cooperation between
> guest and host. What's more, as virtio drivers by design paravirt
> drivers, it is reasonable for guest to accept some cooperation with
> host to manage suspend/resume. So I request to add a new feature
> flag, so that guest and host can negotiate whenever freezing is supported 
> or
>> not.
>
> Jiqian Chen (1):
>   virtio-gpu: Add new feature flag VIRTIO_GPU_F_FREEZE_S3
>
>  

[virtio-dev] Re: [RFC PATCH v8 1/1] virtio-video: Add virtio video device specification

[Alexander Gordeev]

On 26.07.23 16:32, Albert Esteve wrote:


On Mon, Jul 10, 2023 at 10:52 AM Alexander Gordeev 
> wrote:


Hi Albert,

On 06.07.23 16:59, Albert Esteve wrote:
 > Hi Alexander,
 >
 > Thanks for the patch! It is a long document, so I skimmed a bit
in the
 > first read. Find some comments/questions inlined.
 > I will give it a second deeper read soon, but overall I think is in
 > quite good shape. It feels really matured.

Great! Thank you for taking the time to review it.

 > On Thu, Jun 29, 2023 at 4:49 PM Alexander Gordeev
 > mailto:alexander.gord...@opensynergy.com>
 > >> wrote:

...snip...

 >     +
 >     +\subsubsection{Device Operation: Device Commands}
 >     +\label{sec:Device Types / Video Device / Device Operation /
Device
 >     Operation: Device Commands}
 >     +
 >     +This command allows retrieving the device capabilities.
 >     +
 >     +\paragraph{VIRTIO_VIDEO_CMD_DEVICE_QUERY_CAPS}
 >     +\label{sec:Device Types / Video Device / Device Operation /
Device
 >     Operation: Device Commands / VIRTIO_VIDEO_CMD_DEVICE_QUERY_CAPS}
 >     +
 >     +Retrieve device capabilities for all available stream
parameters.
 >     +
 >     +The driver sends this command with
 >     +\field{struct virtio_video_device_query_caps}:
 >     +
 >     +\begin{lstlisting}
 >     +struct virtio_video_device_query_caps {
 >     +        le32 cmd_type; /* VIRTIO_VIDEO_CMD_DEVICE_QUERY_CAPS */
 >     +};
 >     +\end{lstlisting}
 >     +
 >     +The device responds with
 >     +\field{struct virtio_video_device_query_caps_resp}:
 >     +
 >     +\begin{lstlisting}
 >     +#define MASK(x) (1 << (x))
 >     +
 >     +struct virtio_video_device_query_caps_resp {
 >     +        le32 result; /* VIRTIO_VIDEO_RESULT_* */
 >     +        le32 stream_params_bitmask; /* Bitmask of
 >     MASK(VIRTIO_VIDEO_PARAM_*) */
 >     +        le32 coded_formats_bitmask; /* Bitmaks of
 >     MASK(VIRTIO_VIDEO_CODED_FORMAT_*) */
 >     +        le32 raw_formats_bitmask; /* Bitmask of
 >     MASK(VIRTIO_VIDEO_RAW_FORMAT_*) */
 >     +        le32 num_format_deps;
 >     +        le32 num_raw_format_caps;
 >     +        le32 num_coded_resources_caps;
 >     +        le32 num_raw_resources_caps;
 >     +        le32 num_bitrate_caps; /* If
 >     MASK(VIRTIO_VIDEO_PARAM_BITRATE) is set. */
 >     +        u8 padding[4];
 >     +        /* If corresponding
MASK(VIRTIO_VIDEO_PARAM_GROUP_CODEC_*)
 >     is set. */
 >     +        struct virtio_video_mpeg2_caps mpeg2_caps;
 >     +        struct virtio_video_mpeg4_caps mpeg4_caps;
 >     +        struct virtio_video_h264_caps h264_caps;
 >     +        struct virtio_video_hevc_caps hevc_caps;
 >     +        struct virtio_video_vp8_caps vp8_caps;
 >     +        struct virtio_video_vp9_caps vp9_caps;
 >     +        /**
 >     +         * Followed by
 >     +         * struct virtio_video_format_dependency
 >     format_deps[num_format_deps];
 >     +         */
 >     +        /**
 >     +         * Followed by
 >     +         * struct virtio_video_raw_format_caps
 >     raw_format_caps[num_raw_format_caps];
 >     +         */
 >     +        /**
 >     +         * Followed by
 >     +         * struct virtio_video_coded_resources_caps
 >     +         * coded_resources_caps[num_coded_resources_caps];
 >     +         */
 >     +        /**
 >     +         * Followed by
 >     +         * struct virtio_video_raw_resources_caps
 >     raw_resources_caps[num_raw_resources_caps];
 >     +         */
 >     +        /**
 >     +         * Followed by if MASK(VIRTIO_VIDEO_PARAM_BITRATE)
is set
 >     +         * struct virtio_video_bitrate_caps
 >     bitrate_caps[num_bitrate_caps];
 >     +         */
 >     +};
 >
 >
 > Maybe nitpicking, but some of the member structs are inside a
comment
 > and some are not.
 > Does not seem to correlate with them being conditional.
 > I think is nice to have conditional fields in comment blocks to
 > highlight it, but then the
 > VIRTIO_VIDEO_PARAM_GROUP_CODEC_* structs need to be in their own
comment
 > block.

Yeah, this style comes from draft v5, then I added the conditional
statementson top, so now it is harder to understand. I also would like
to do this in a different way. I was thinking recently about
extendability of this construct, it doesn't look good. If a new
codec or
a new codec-specific parameters is added, it has to be guarded by a new
feature flag, say 

[virtio-dev] [virtio 1.3] Change freeze has started

[Cornelia Huck]

As outlined in
https://lists.oasis-open.org/archives/virtio/202304/msg00036.html ff.,
as part of the release process for the 1.3 version of the virtio spec,
we have now entered change freeze. This means that the only changes to
the main virtio branch will now be those needed for the 1.3 release
(administrative and editorial.) We hope to have something ready for vote
in August.

For any changes for the next virtio release, please target the
virtio-1.4 branch, which will eventually merge back into the main
development branch once 1.3 has been released. (This next release might
not be called 1.4, but that should be inconsequential here.)

virtio 1.3 timeline:

Development

July 3rd: feature freeze (github issue
and change on list)

August 2nd: change freeze (all voted
upon)

virtio-next development branch opens  < we are here

August: prepare draft

August 31st (latest): draft voted upon by TC

September: public review period

October: addtl public review period, if needed

October (November): 1.3 released

merge virtio-next development branch


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

[virtio-dev] RE: [VIRTIO GPU PATCH v3 0/1] Add new feature flag VIRTIO_GPU_F_FREEZE_S3

[Parav Pandit]

> From: Chen, Jiqian 
> Sent: Wednesday, August 2, 2023 11:28 AM
> 
> On 2023/8/2 12:49, Parav Pandit wrote:
> >
> >
> >> From: virtio-dev@lists.oasis-open.org
> >>  On Behalf Of Chen, Jiqian
> >> Sent: Wednesday, August 2, 2023 8:51 AM Hi all,
> >>
> >> Do you have any other comments on the modification of virtio-gpu S3?
> >> Looking forward to your reply and comments.
> >>
> 
> Hi Parav Pandit,
> Thank you for your reply. Let me try to answer your question.
> 
> > I am not familiar with the GPU, so a dumb question is, why the S3 state is 
> > gpu
> specific?
> S3 state is not gpu specific. I think different virtio devices may have 
> different
> actions/problems when S3.
I am making assumption that the gpu device is pci. :)
If so can you please use the transport specific notification from gpu guest 
driver to notify to qemu?

> When I do S3 on Xen, I found guest's display can't come back and the root 
> cause
> is in virtio-gpu backend in QEMU.
> So, to solve that problem, I change codes to let guest notify QEMU 
> virtio-gpu's
> suspend state, and then QEMU will not destroy resources that used for display.
> Please see attached kernel and QEMU patch links.
> For above reason, Gerd suggest me to add a new feature flag specifically for
> virtio-gpu, so that guest and host can negotiate whenever to enable above
> mechanism.
> 
> > Can a transport specific suspend state be used and apply to all virtio 
> > devices?
> Based on my limited knowledge. Different virtio devices have different virtio
> queues, my modifications let guest to notify QEMU by using virtio-gpu's 
> control
> queue.
> Other virtio devices can't get that notification unless you can traverse all 
> virtio
> devices and notify one by one, or other global method?
> But for now, this patch is to add a new feature flag just used for virtio-gpu.
> 
If this is done at the pci transport level, all the virtio device benefit from 
it without inventing in each device type.

> > And can you please add both the rationale to the commit message?
> Sure, I will expand the description of my commit message and add them.
> 
> >
> >> On 2023/7/20 20:18, Jiqian Chen wrote:
> >>> v3:
> >>>
> >>> Hi all,
> >>> Thanks for Gerd Hoffmann's advice. V3 makes below changes:
> >>> * Use enum for freeze mode, so this can be extended with more
> >>>   modes in the future.
> >>> * Rename functions and paratemers with "_S3" postfix.
> >>> * Explain in more detail
> >>>
> >>> And latest version on QEMU and Linux kernel side:
> >>>   QEMU: https://lore.kernel.org/qemu-devel/20230720120816.8751-1-
> >> jiqian.c...@amd.com
> >>>   Kernel:
> >>> https://lore.kernel.org/lkml/20230720115805.8206-1-Jiqian.Chen@amd.c
> >>> om
> >>> /T/#t
> >>>
> >>> Best regards,
> >>> Jiqian Chen.
> >>>
> >>>
> >>> v2:
> >>> link,
> >>> https://lists.oasis-open.org/archives/virtio-comment/202307/msg00160
> >>> .h
> >>> tml
> >>>
> >>> Hi all,
> >>> Thanks to Gerd Hoffmann for his suggestions. V2 makes below changes:
> >>> * Elaborate on the types of resources.
> >>> * Add some descriptions for S3 and S4.
> >>>
> >>>
> >>> v1:
> >>> link,
> >>> https://lists.oasis-open.org/archives/virtio-comment/202306/msg00595
> >>> .h
> >>> tml
> >>>
> >>> Hi all,
> >>> I am working to implement virtgpu S3 function on Xen.
> >>>
> >>> Currently on Xen, if we start a guest through Qemu with enabling
> >>> virtgpu, and then suspend and s3resume guest. We can find that the
> >>> guest kernel comes back, but the display doesn't. It just shown a black
> screen.
> >>>
> >>> That is because when guest was during suspending, it called into
> >>> Qemu and Qemu destroyed all resources and reset renderer. This made
> >>> the display gone after guest resumed.
> >>>
> >>> So, I add a mechanism that when guest is suspending, it will notify
> >>> Qemu, and then Qemu will not destroy resources. That can help
> >>> guest's display come back.
> >>>
> >>> As discussed and suggested by Robert Beckett and Gerd Hoffmann on v1
> >>> qemu's mailing list. Due to that mechanism needs cooperation between
> >>> guest and host. What's more, as virtio drivers by design paravirt
> >>> drivers, it is reasonable for guest to accept some cooperation with
> >>> host to manage suspend/resume. So I request to add a new feature
> >>> flag, so that guest and host can negotiate whenever freezing is supported 
> >>> or
> not.
> >>>
> >>> Jiqian Chen (1):
> >>>   virtio-gpu: Add new feature flag VIRTIO_GPU_F_FREEZE_S3
> >>>
> >>>  device-types/gpu/description.tex | 42
> >>> 
> >>>  1 file changed, 42 insertions(+)
> >>>
> >>
> >> --
> >> Best regards,
> >> Jiqian Chen.
> 
> --
> Best regards,
> Jiqian Chen.