Re: [PATCH 11/15] worker: minor simplification

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> On Tue, Nov 3, 2015 at 6:08 PM, Fabiano Fidêncio <fidencio@xxxxxxxxxx> wrote:
> > On Tue, Nov 3, 2015 at 5:28 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> >>>
> >>> On Tue, Nov 3, 2015 at 11:20 AM, Frediano Ziglio <fziglio@xxxxxxxxxx>
> >>> wrote:
> >>> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> >>> >
> >>> > ---
> >>> >  server/red_worker.c | 42 +++++++++++++++++++++++-------------------
> >>> >  server/tree.h       |  1 +
> >>> >  2 files changed, 24 insertions(+), 19 deletions(-)
> >>> >
> >>> > diff --git a/server/red_worker.c b/server/red_worker.c
> >>> > index 1849cc8..9f42f87 100644
> >>> > --- a/server/red_worker.c
> >>> > +++ b/server/red_worker.c
> >>> > @@ -3056,6 +3056,7 @@ static inline int
> >>> > red_current_add_with_shadow(RedWorker *worker, Ring *ring, Dra
> >>> >  {
> >>> >  #ifdef RED_WORKER_STAT
> >>> >      stat_time_t start_time = stat_now(worker);
> >>> > +    ++worker->add_with_shadow_count;
> >>> >  #endif
> >>> >
> >>> >      Shadow *shadow = __new_shadow(worker, item, delta);
> >>> > @@ -3131,24 +3132,8 @@ static inline void
> >>> > red_update_streamable(RedWorker
> >>> > *worker, Drawable *drawable,
> >>> >      drawable->streamable = TRUE;
> >>> >  }
> >>> >
> >>> > -static inline int red_current_add_qxl(RedWorker *worker, Ring *ring,
> >>> > Drawable *drawable,
> >>> > -                                      RedDrawable *red_drawable)
> >>> > +static void red_print_stats(RedWorker *worker)
> >>> >  {
> >>> > -    int ret;
> >>> > -
> >>> > -    if (has_shadow(red_drawable)) {
> >>> > -        SpicePoint delta;
> >>> > -
> >>> > -#ifdef RED_WORKER_STAT
> >>> > -        ++worker->add_with_shadow_count;
> >>> > -#endif
> >>> > -        delta.x = red_drawable->u.copy_bits.src_pos.x -
> >>> > red_drawable->bbox.left;
> >>> > -        delta.y = red_drawable->u.copy_bits.src_pos.y -
> >>> > red_drawable->bbox.top;
> >>> > -        ret = red_current_add_with_shadow(worker, ring, drawable,
> >>> > &delta);
> >>> > -    } else {
> >>> > -        red_update_streamable(worker, drawable, red_drawable);
> >>> > -        ret = red_current_add(worker, ring, drawable);
> >>> > -    }
> >>> >  #ifdef RED_WORKER_STAT
> >>> >      if ((++worker->add_count % 100) == 0) {
> >>> >          stat_time_t total = worker->add_stat.total;
> >>> > @@ -3173,6 +3158,26 @@ static inline int red_current_add_qxl(RedWorker
> >>> > *worker, Ring *ring, Drawable *d
> >>> >          stat_reset(&worker->__exclude_stat);
> >>> >      }
> >>> >  #endif
> >>> > +}
> >>> > +
> >>> > +static int red_add_drawable(RedWorker *worker, Drawable *drawable)
> >>> > +{
> >>> > +    int ret = FALSE, surface_id = drawable->surface_id;
> >>> > +    RedDrawable *red_drawable = drawable->red_drawable;
> >>> > +    Ring *ring = &worker->surfaces[surface_id].current;
> >>> > +
> >>> > +    if (has_shadow(red_drawable)) {
> >>> > +        SpicePoint delta = {
> >>> > +            .x = red_drawable->u.copy_bits.src_pos.x -
> >>> > red_drawable->bbox.left,
> >>> > +            .y = red_drawable->u.copy_bits.src_pos.y -
> >>> > red_drawable->bbox.top
> >>> > +        };
> >>> > +        ret = red_current_add_with_shadow(worker, ring, drawable,
> >>> > &delta);
> >>> > +    } else {
> >>> > +        red_update_streamable(worker, drawable, red_drawable);
> >>> > +        ret = red_current_add(worker, ring, drawable);
> >>> > +    }
> >>> > +
> >>> > +    red_print_stats(worker);
> >>> >      return ret;
> >>> >  }
> >>> >
> >>> > @@ -3484,8 +3489,7 @@ static gboolean red_process_draw(RedWorker
> >>> > *worker,
> >>> > QXLCommandExt *ext_cmd)
> >>> >          goto end;
> >>> >      }
> >>> >
> >>> > -    if (red_current_add_qxl(worker,
> >>> > &worker->surfaces[surface_id].current,
> >>> > drawable,
> >>> > -                            red_drawable)) {
> >>> > +    if (red_add_drawable(worker, drawable)) {
> >>> >          if (drawable->tree_item.effect != QXL_EFFECT_OPAQUE) {
> >>> >              worker->transparent_count++;
> >>> >          }
> >>> > diff --git a/server/tree.h b/server/tree.h
> >>> > index 77f1fbf..e10fa1c 100644
> >>> > --- a/server/tree.h
> >>> > +++ b/server/tree.h
> >>> > @@ -45,6 +45,7 @@ struct TreeItem {
> >>> >      QRegion rgn;
> >>> >  };
> >>> >
> >>> > +/* A region "below" a copy, or the src region of the copy */
> >>>
> >>> This comment is nice, but doesn't belong to this commit IMHO.
> >>>
> >>> >  struct Shadow {
> >>> >      TreeItem base;
> >>> >      QRegion on_hold;
> >>> > --
> >>> > 2.4.3
> >>> >
> >>> > _______________________________________________
> >>> > Spice-devel mailing list
> >>> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> >>> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >>>
> >>> ack!
> >>>
> >>
> >> There is an ack and a comment. Do you want to split the patch or commit
> >> as it is?
> >
> > Well, I meant: "would be better to have it in another commit, but
> > ignore if you want".
> >
> >> Viewing the 11+12+13 patches looks they move multiple times delta
> >> computation.
> >
> > It didn't come to my attention. I will re-review the patches now with
> > a more focused view on the delta computation.
> 
> I still didn't get exactly the problems you found wrt delta
> computation on these patches. The patches moved it in order to
> directly use it from the drawable, which is okay IMHO.
> 
> >
> >> I would nack it and consider all these strangely related patches.
> >
> 
> What's your proposal? Squash the patches?
> 

I was just updating the patch. Basically:

patch 11
delta computation moved from red_current_add_qxl to red_add_drawable (new function from red_current_add_qxl split).

patch 12
computation moved from red_add_drawable to __new_shadow

patch 13
computation moved from shadow_new (renamed from __new_shadow) to red_current_add_with_shadow (called by red_add_drawable)

So patch 13 does two thing, move code and move the computation from a function to another. Also the move move a piece of code just added in the previous patch to another.
My current change just leave code movement in patch 13 making it easier. So I have currently

patch 11 split red_current_add_qxl
patch 12 move delta computation from red_add_drawable to red_current_add_with_shadow
patch 13 move the 2 functions

(basically I changed patch 12 and 13)

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]