[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 06:34:03 PDT 2013
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.
>>>
>>>>>
>>>>> 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