On 2024/1/15 15:55, Parav Pandit wrote: > >> From: Chen, Jiqian <jiqian.c...@amd.com> >> Sent: Monday, January 15, 2024 1:18 PM >> >> On 2024/1/15 15:37, Parav Pandit wrote: >>> >>>> From: Chen, Jiqian <jiqian.c...@amd.com> >>>> Sent: Monday, January 15, 2024 1:03 PM >>>> >>>> On 2024/1/12 17:44, Parav Pandit wrote: >>>>> >>>>> >>>>>> From: Chen, Jiqian <jiqian.c...@amd.com> >>>>>> Sent: Friday, January 12, 2024 2:55 PM >>>>>> >>>>>> On 2024/1/12 16:47, Parav Pandit wrote: >>>>>>> >>>>>>> >>>>>>>> From: Chen, Jiqian <jiqian.c...@amd.com> >>>>>>>> Sent: Friday, January 12, 2024 1:55 PM >>>>>>>> >>>>>>>> >>>>>>>> On 2024/1/12 16:02, Parav Pandit wrote: >>>>>>>>> Hi Jiqian, >>>>>>>>> >>>>>>>>>> From: Chen, Jiqian <jiqian.c...@amd.com> >>>>>>>>>> Sent: Friday, January 12, 2024 1:11 PM >>>>>>>>> >>>>>>>>>> >>>>>>>>>> Hi all, >>>>>>>>>> Sorry to reply late. >>>>>>>>>> I don't know if you still remember this problem, let me briefly >>>>>>>>>> descript >>>>>> it. >>>>>>>>>> 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 resume guest. We can >>>>>>>>>> find that the guest kernel comes back, but the display doesn't. >>>>>>>>>> It just shown a black >>>>>>>> screen. >>>>>>>>>> That is because during suspending, guest called into qemu and >>>>>>>>>> qemu destroyed all display resources and reset renderer. This >>>>>>>>>> made the display gone after guest resumed. >>>>>>>>>> So, I add a new command for virtio-gpu that when guest is >>>>>>>>>> suspending, it will notify qemu and set >>>>>>>>>> parameter(preserver_resource) to 1 and then qemu will preserve >>>>>>>>>> resources, and when resuming, guest will notify qemu to set >>>>>>>>>> parameter to 0, and then qemu will keep the normal actions. >>>>>>>>>> That can >>>>>> help guest's display come back. >>>>>>>>>> When I upstream above implementation, Parav and MST suggest me >>>> to >>>>>>>> use >>>>>>>>>> the PM capability to fix this problem instead of adding a new >>>>>>>>>> command or state bit. >>>>>>>>>> Now, I have tried the PM capability of virtio-gpu, it can't be >>>>>>>>>> used to solve this problem. >>>>>>>>>> The reason is: >>>>>>>>>> during guest suspending, it will write D3 state through PM cap, >>>>>>>>>> then I can save the resources of virtio-gpu on Qemu side(set >>>>>>>>>> preserver_resource to 1), but during the process of resuming, >>>>>>>>>> the state of PM cap will be cleared by qemu >>>>>>>>>> resetting(qemu_system_wakeup-> qemu_devices_reset-> >>>>>>>>>> virtio_vga_base_reset-> virtio_gpu_gl_reset), it causes that >>>>>>>>>> when guest reads state from PM cap, it will find the virtio-gpu >>>>>>>>>> has already been D0 state, >>>>>>>>> This behavior needs to be fixed. As the spec listed out, " This >>>>>>>>> 2-bit field is >>>>>>>> used both to determine the current power state of a Function" >>>>>>>> Do you mean it is wrong to reset PM cap when vritio_gpu reset in >>>>>>>> current qemu code? Why? >>>>>>> Because PM is implementing the support from D3->d0 transition and >>>>>>> if it >>>>>> device in D3, the device needs to respond that it is in D3 to match >>>>>> the PCI spec. >>>>>>> >>>>>>>> Shouldn't all device states, including PM registers, be reset >>>>>>>> during the process of virtio-gpu reset? >>>>>>> If No_Soft_Reset== 0, no device context to be preserved. >>>>>>> If No_Soft_Reset == 1, device to preserve minimal registers and >>>>>>> other >>>>>> things listed in pci spec. >>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> So device needs to return D3 in the PowerState field. This is must. >>>>>>>> But current behavior is a kind of soft reset, I >>>>>>>> think(!PCI_PM_CTRL_NO_SOFT_RESET). The pm cap is reset by qemu >> is >>>>>>>> reasonable. >>>>>>> What you described is, you need to do No_Soft_Reset=1, so please >>>>>>> set the >>>>>> cap accordingly to achieve the restore. >>>>>>> >>>>>>> Also in your case the if the QEMU knows that it will have to >>>>>>> resume the >>>>>> device soon. >>>>>>> Hence, it can well prepare the gpu context before resuming the vcpus. >>>>>>> In such case, the extra register wouldn’t be necessary. >>>>>>> Having PM control register is anyway needed to drive the device >> properly. >>>>>> Even as you said, the reset behavior of PM in the current QEMU code >>>>>> is incorrect, and even if it is modified, it also cannot fix this >>>>>> problem with >> S3. >>>>> You meant D3? >>>>> >>>>>> Because there is twice device reset during resuming. >>>>>> First is when we trigger a resume command to qemu, qemu will reset >>>>>> device(qemu_system_wakeup-> qemu_devices_reset-> >>>>>> virtio_vga_base_reset-> virtio_gpu_gl_reset). >>>>> A device implementation that supports PM should not reset the device. >>>> Please fix it. >>>> Do you mean the devices_reset behavior of qemu shouldn't happen when >>>> a device has PM cap? >>> Right. Device reset should not happen on PM capable device that offers >> no_soft_reset==1. >> But current qemu code, no_soft_reset==0. >> > So please fix the qemu when you are adding the functionality of restoring the > device context on PM. My patch didn't add the functionality of restoring the device context on PM. Is no_soft_reset must set 1 when a device has PM cap? It should also be allowed for situations that are not equal to 1, right? There should be devices that has PM cap but no_soft_reset is 0 because that devices can't store and restore the device context on d3->d0 transition, like current qemu code. So the fix should be feasible for all situations that whether the no_soft_reset is 1 or not. If use PM cap, not every situation can solve current problem (display resources of GPU will be destroyed).
> >>> >>>> Or just the state of PM cap shouldn't be reset? >>>> >>>>> >>>>>> After the first time, then second time happens in guest kernel, >>>>>> virtio_device_restore-> virtio_reset_device. But the PM state >>>>>> changes >>>>>> (D3 to >>>>>> D0) happens between those two, so the display resources will be >>>>>> still destroyed by the second time of device reset. >>>>> The driver that understands that device supports PM, will skip the >>>>> reset >>>> steps. >>>> But it happens for now. >>> This should be enhanced in the guest driver. >>> >>>> In current qemu codes, the bit PCI_PM_CTRL_NO_SOFT_RESET of >>>> virtio-pci devices isn't set, is it a bug? It means the no_soft_reset==0. >>>> >>> It should be set when/if the device can store and restore the device context >> on d3->d0 transition. >> I don't know why it doesn't be set in current qemu code. > May be because QEMU is not restoring the context on PM commands. > >> What do you mean "the device can store and restore the device context on >> d3->d0 transition"? The real physical device or the simulated devices on qemu >> side? > Does not matter, whichever implementation of device is doing the PM should > set it. > In your case it is qemu. > >> How do I confirm it? > If you mean "I" means guest driver, it will just work as long as it refers to > the PM capabilities. > >> >>> >>>>> Hence the 2nd reset will also not occur. >>>>> Therefore, device state will be restored. >>>> >>>> -- >>>> Best regards, >>>> Jiqian Chen. >> >> -- >> Best regards, >> Jiqian Chen. -- Best regards, Jiqian Chen.
