[Spice-devel] [PATCH spice-server 1/3] red_worker: cleanup red_clear_surface_drawables_from_pipes
Yonit Halperin
yhalperi at redhat.com
Fri Sep 20 06:50:45 PDT 2013
On 09/18/2013 09:37 AM, Marc-André Lureau wrote:
> 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?
RedChannelClient holds the the outgoing data in the send_data structure.
>>
>>
>>>>>
>>>>>>>
>>>>>>> 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