On 09/18/2013 09:19 AM, Marc-André Lureau wrote:
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.Hi On Wed, Sep 18, 2013 at 2:58 PM, Yonit Halperin <yhalperi@xxxxxxxxxx> 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@xxxxxxxxxx> 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 thatPlease 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. (althoughI 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.
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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel