[Spice-devel] [PATCH spice-server 1/3] red_worker: cleanup red_clear_surface_drawables_from_pipes
Yonit Halperin
yhalperi at redhat.com
Wed Sep 18 05:58:19 PDT 2013
Hi,
On 09/17/2013 12:11 PM, Marc-André Lureau wrote:
> 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).
I can rename it to block_till_done or something like that
>
>> 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
I wouldn't call it weird. We don't want to block if the "force" wasn't
set to TRUE (which happens quite often when off-screen surfaces are
enabled; for every destroy_surface command). Also, before the patch it
was also called just in case "if (wait_for_outgoing_item)"
> probably this wait should be done before or during
> pipe_remove_and_release..)
This call need to be done once, for the item that we are currently in
the middle of sending. I don't think it is misplaced.
>
>>>
>>> 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
>>>
>>>
>>>
>>>
>>
>
>
>
More information about the Spice-devel
mailing list