Re: [PATCH spice-server 1/3] red_worker: cleanup red_clear_surface_drawables_from_pipes

[Yonit Halperin <yhalperi@xxxxxxxxxx>]

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.
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.
> 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