[Spice-devel] [PATCH spice-server 1/3] red_worker: cleanup red_clear_surface_drawables_from_pipes
Marc-André Lureau
marcandre.lureau at gmail.com
Wed Sep 18 06:37:05 PDT 2013
On Wed, Sep 18, 2013 at 3:34 PM, Yonit Halperin <yhalperi at redhat.com> wrote:
> On 09/18/2013 09:19 AM, Marc-André Lureau wrote:
>>
>> Hi
>>
>> On Wed, Sep 18, 2013 at 2:58 PM, Yonit Halperin <yhalperi at redhat.com>
>> wrote:
>>>
>>> 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
>>
>>
>> Please keep the "wait" in the name, since it calls "wait_item"
>>
>>>>
>>>>> 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)"
>>
>>
>> I think it would be easier to read written like this:
>>
>> if (!wait)
>> return;
>>
>> if (item)
>> wait_pipe_item()
>> else
>> red_wait_outgoing_item()
>>
>> but the last call "just in case" clearly indicate it is somehow misplaced.
>>
>>>> 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.
>>
>>
>> After a call to something called
>> red_channel_client_pipe_remove_and_release, I expect to be done with
>> it, not to care later about it being still around. That's why I think
>> it's misplaced.
>>
>> I could still "be done once" when necessary, even inside
>> remove_and_release. I haven't looked at the details though.
>>
> The pipe item in remove_and_release is not the item we are waiting on.
> remove_and_release is called only for items that are still in the pipe.
> Once remove_and_release is called, we are done with them.
> red_wait_outgoing_item refers to an item that was dequeued from the pipe,
> and is still during sending. So we didn't encounter this item when we
> scanned the pipe.
Ok, I thought ongoing item would still be in the pipe (I think it
would make more sense, but whatever)
In this case, how could we check the current outgoing item before waiting?
>
>
>>>>
>>>>>>
>>>>>> 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
More information about the Spice-devel
mailing list