Re: [Qemu-devel] [PATCH 2/2] qxl: add QXL_IO_UPDATE_MEM for guest S3&S4 support

[Yonit Halperin <yhalperi@redhat.com>]

On 06/20/2011 07:32 PM, Alon Levy wrote:
> On Mon, Jun 20, 2011 at 05:50:32PM +0200, Gerd Hoffmann wrote:
>> On 06/20/11 17:11, Alon Levy wrote:
>>> On Mon, Jun 20, 2011 at 04:07:59PM +0200, Gerd Hoffmann wrote:
>>>>>> What is the difference to one worker->stop() + worker->start() cycle?
>>>>>>
>>>>>
>>>>> ok, stop+start won't disconnect any clients either. But does stop render all waiting commands?
>>>>> I'll have to look, I don't know if it does.
>>>>
>>>> It does.  This is what qemu uses to flush all spice server state to
>>>> device memory on migration.
I don't see that STOP flushes the command rings. We must read and empty 
the command rings before going to S3.
>>>>
>>>> What is the reason for deleting all surfaces?
>>>
>>> Making sure all references are dropped to pci memory in devram.
>>
>> Ah, because the spice server keeps a reference to the create command
>> until the surface is destroyed, right?
>
> Actually right, so my correction stands corrected.
>
>>
>> There is is QXL_IO_DESTROY_ALL_SURFACES + worker->destroy_surfaces() ...
>>
>
> Regarding QXL_IO_DESTROY_ALL_SURFACES, it destroys the primary surface too,
> which is a little special, that's another difference - update_mem destroys
> everything except the primary. I know I tried to destroy the primary but it
> didn't work right, don't recall why right now, so I guess I'll have to retry.
>
I guess it is because DrvAssertMode(disable) destroyed the primary 
surface in a separate call. Don't think we need to separate the calls 
any more (we probably needed it when we thought S3 and resolution 
changes will have different paths).

>> The QXL_IO_UPDATE_MEM command does too much special stuff IMHO.
>> I also think we don't need to extend the libspice-server API.
>>
>> We can add a I/O command which renders everything to device memory
>> via stop+start.  We can zap all surfaces with the existing command +
> Yes, start+stop work nicely, didn't realize (saw it before, assumed
> it wouldn't be good enough), just need to destroy the surfaces too.
>
>> worker call.  We can add a I/O command to ask qxl to push the
>> release queue head to the release ring.
>
> So you suggest to replace QXL_IO_UPDATE_MEM with what, two io commands instead
> of using the val parameter?
>   QXL_IO_UPDATE_MEM
>   QXL_IO_FLUSH_RELEASE
> ?
>
>>
>> Comments?
>>
>> cheers,
>>    Gerd
>>