[Spice-devel] [PATCH spice-server 1/3] red_worker: cleanup red_clear_surface_drawables_from_pipes

[Marc-André Lureau]

On Mon, Sep 16, 2013 at 5:34 PM, Yonit Halperin <yhalperi at redhat.com> wrote:
> Hi,
>
> 'force' means: wait till there is no pipe item that references the surface.
> If force = FALSE, try to release any such pipe item, but as long as it
> doesn't require blocking.
>
>
> On 09/16/2013 09:00 AM, Marc-André Lureau wrote:
>>
>> Hi,
>>
>> Since I don't understand the "force" argument, it would be easier to
>> review to keep current behaviour and just call
>> red_wait_outgoing_item() at the end.
>>
>> To me the meaning of "force" argument is rather "do not wait if dep".
>> When a drawable depend on the surface:
>> - force == false: return immediately when found
>> - force == true: wait for the first found item to be sent (I guess it
>> is the last to be sent, but isn't it supposed to be sent before the
>> one that depends on it?)
>
> red_clear_surface_drawables_from_pipe(surface=S) scans the pipe from the
> newest item to the oldest item (the first to be sent). It removes the newer
> pipe items that are directly drawn over surface S, till it finds an item
> that depends on S (i.e., S is its src/mask/brush). Such item might require
> that older pipe items that are drawn over S will be rendered on the client
> side. Thus, if force=TRUE, we wait till the newest item that depends on S is
> sent to the client.

Ok, that's pretty clear in the code. However why is it called "force"?
It's rather "wait if used" (trying to find an appropriate name).

> red_wait_outgoing_item() is called when a dependent item is not found, for
> case that the last item that was dequeued from the pipe, and is currently
> during sending, refers to surface S (we could have had a method that returns
> the currently sent item, and check it, but we don't have one yet)...
> I can add this comments to the code if it helps.

Ok, I still think it would be simpler if it wouldn't depend on this
weird "if (force)" conditionnal block. Ie, do it unconditionnally,
like before, with a comment explaining why it is necessary. (although
probably this wait should be done before or during
pipe_remove_and_release..)

>>
>> If you could clarified the above, and comment it in the code that
>> would be helpful! :)
>>
>>
>> On Thu, Sep 12, 2013 at 10:04 PM, Yonit Halperin <yhalperi at redhat.com>
>> wrote:
>>>
>>> (1) merge 'force' and 'wait_for_outgoing_item' to one parameter.
>>>      'wait_for_outgoing_item' is a derivative of 'force'.
>>> (2) move the call to red_wait_outgoing_item to
>>> red_clear_surface_drawables_from_pipe
>>> ---
>>>   server/red_worker.c | 20 +++++++++++---------
>>>   1 file changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/server/red_worker.c b/server/red_worker.c
>>> index 0c611d0..e9a7fa1 100644
>>> --- a/server/red_worker.c
>>> +++ b/server/red_worker.c
>>> @@ -2064,22 +2064,24 @@ static void
>>> red_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int
>>>
>>>       if (item) {
>>>           red_channel_client_wait_pipe_item_sent(&dcc->common.base,
>>> item);
>>> +    } else if (force) {
>>> +        /*
>>> +         * in case that the pipe didn't contain any item that is
>>> dependent on the surface, but
>>> +         * there is one during sending.
>>> +         */
>>> +        red_wait_outgoing_item(&dcc->common.base);
>>>       }
>>>   }
>>>
>>> -static void red_clear_surface_drawables_from_pipes(RedWorker *worker,
>>> int surface_id,
>>> -    int force, int wait_for_outgoing_item)
>>> +static void red_clear_surface_drawables_from_pipes(RedWorker *worker,
>>> +                                                   int surface_id,
>>> +                                                   int force)
>>>   {
>>>       RingItem *item, *next;
>>>       DisplayChannelClient *dcc;
>>>
>>>       WORKER_FOREACH_DCC_SAFE(worker, item, next, dcc) {
>>>           red_clear_surface_drawables_from_pipe(dcc, surface_id, force);
>>> -        if (wait_for_outgoing_item) {
>>> -            // in case that the pipe didn't contain any item that is
>>> dependent on the surface, but
>>> -            // there is one during sending.
>>> -            red_wait_outgoing_item(&dcc->common.base);
>>> -        }
>>>       }
>>>   }
>>>
>>> @@ -4248,7 +4250,7 @@ static inline void red_process_surface(RedWorker
>>> *worker, RedSurfaceCmd *surface
>>>              otherwise "current" will hold items that other drawables may
>>> depend on, and then
>>>              red_current_clear will remove them from the pipe. */
>>>           red_current_clear(worker, surface_id);
>>> -        red_clear_surface_drawables_from_pipes(worker, surface_id,
>>> FALSE, FALSE);
>>> +        red_clear_surface_drawables_from_pipes(worker, surface_id,
>>> FALSE);
>>>           red_destroy_surface(worker, surface_id);
>>>           break;
>>>       default:
>>> @@ -10921,7 +10923,7 @@ static inline void destroy_surface_wait(RedWorker
>>> *worker, int surface_id)
>>>          otherwise "current" will hold items that other drawables may
>>> depend on, and then
>>>          red_current_clear will remove them from the pipe. */
>>>       red_current_clear(worker, surface_id);
>>> -    red_clear_surface_drawables_from_pipes(worker, surface_id, TRUE,
>>> TRUE);
>>> +    red_clear_surface_drawables_from_pipes(worker, surface_id, TRUE);
>>>   }
>>>
>>>   static void dev_destroy_surface_wait(RedWorker *worker, uint32_t
>>> surface_id)
>>> --
>>> 1.8.1.4
>>>
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>>
>>
>>
>



-- 
Marc-André Lureau