On Wed, 19 Aug 2015 11:25:02 +0000 "Ucan, Emre (ADITG/SW1)" <[email protected]> wrote:
> - Introduce the dirty parameter for triggering the render order change > - IVI_NOTIFICATION_REMOVE/ADD flags are set only at commit_layer_list. > > Signed-off-by: Emre Ucan <[email protected]> Hi Ucan-san and Tanibata-san, there are a couple of commit message issues one should pay attention to in the future. The email subject, a.k.a commit summary line, is too long. It should be limited to roughly around 60 characters or so. A good summary is unique, descriptive, and short. It seems you are doing the opposite: the summary is a long prose, while the message body has short bullet points. When you are sending v2, v3, etc. of a patch, it would be good to include a list of changes, see e.g. http://cgit.freedesktop.org/wayland/weston/commit/?id=23ab7159d200883cc0d21db8dc4efdd58e2d60a7 Since we are now doing small patches to ivi-layout.c, I will be more strict on wl_list manipulation. There is a lot to clean up here. (Btw. I am a little confused here on the architecture. If the purpose of struct link_layer is to allow an ivi-surface to be on multiple ivi-layers, how can a direct list like struct ivi_layout_layer::order.surface_list work? Below it looks like struct ivi_layout_surface::order.link is what is stored in that list, but you cannot store the same link in multiple lists at the same time.) > --- > ivi-shell/ivi-layout-private.h | 1 + > ivi-shell/ivi-layout.c | 63 > ++++++++++++++-------------------------- > 2 files changed, 23 insertions(+), 41 deletions(-) > > diff --git a/ivi-shell/ivi-layout-private.h b/ivi-shell/ivi-layout-private.h > index 9c04c30..074d598 100644 > --- a/ivi-shell/ivi-layout-private.h > +++ b/ivi-shell/ivi-layout-private.h > @@ -81,6 +81,7 @@ struct ivi_layout_layer { > } pending; > > struct { > + int dirty; > struct wl_list surface_list; > struct wl_list link; > } order; > diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c > index d412069..bc8aead 100644 > --- a/ivi-shell/ivi-layout.c > +++ b/ivi-shell/ivi-layout.c > @@ -809,53 +809,38 @@ commit_layer_list(struct ivi_layout *layout) > > ivilayer->prop = ivilayer->pending.prop; > > - if (!(ivilayer->event_mask & > - (IVI_NOTIFICATION_ADD | IVI_NOTIFICATION_REMOVE)) ) { > + if (!ivilayer->order.dirty) { > continue; > } > > - if (ivilayer->event_mask & IVI_NOTIFICATION_REMOVE) { > - wl_list_for_each_safe(ivisurf, next, > - &ivilayer->order.surface_list, order.link) { > - remove_ordersurface_from_layer(ivisurf); > + wl_list_for_each_safe(ivisurf, next, > + &ivilayer->order.surface_list, order.link) { > + remove_ordersurface_from_layer(ivisurf); > > - if (!wl_list_empty(&ivisurf->order.link)) { > - wl_list_remove(&ivisurf->order.link); > - } > - > - wl_list_init(&ivisurf->order.link); > - ivisurf->event_mask |= IVI_NOTIFICATION_REMOVE; > + if (!wl_list_empty(&ivisurf->order.link)) { > + wl_list_remove(&ivisurf->order.link); While at it, let's fix these mis-patterns that give a false sense of safety. wl_list_empty() is not a valid way to check if a link can be wl_list_remove()'d. Checking wl_list_empty() on a link offers no information: if it returns true, wl_list_remove() is safe to do. If it returns false, you still do not know if wl_list_remove() is safe; the link could be part of a list, or the link could be "uninitialized" (e.g. just wl_list_remove()'d). You need some other information to know whether a link is safe to remove. It could be another variable, or it could be a design decision (invariant) that it must be always safe to remove. As the wl_list_empty() check is completely bogus here, you can just remove it and do the wl_list_remove() unconditionally. If it leads to a crash, the code was already broken before. I also do not think it can lead to a crash, because the only case when the check avoids calling wl_list_remove() is the only case according to wl_list_empty() that removing actually is safe. > } > > - wl_list_init(&ivilayer->order.surface_list); > + wl_list_init(&ivisurf->order.link); > + ivisurf->event_mask |= IVI_NOTIFICATION_REMOVE; > } > > - if (ivilayer->event_mask & IVI_NOTIFICATION_ADD) { > - wl_list_for_each_safe(ivisurf, next, > - &ivilayer->order.surface_list, > order.link) { > - remove_ordersurface_from_layer(ivisurf); > - > - if (!wl_list_empty(&ivisurf->order.link)) { > - wl_list_remove(&ivisurf->order.link); > - } > + wl_list_init(&ivilayer->order.surface_list); This wl_list_init() is confusing. Either the wl_list_for_each_safe loop above removes all items from the list in which case the init is redundant, or if there are items still in the list then those items are left in a broken list. The list also cannot be "uninitialized" because we just iterated through it. If you want to ensure and document that the list really is empty at this point, do this: assert(wl_list_empty(&ivilayer->order.surface_list)); > > + wl_list_for_each(ivisurf, &ivilayer->pending.surface_list, > + pending.link) { > + if (!wl_list_empty(&ivisurf->order.link)){ > + wl_list_remove(&ivisurf->order.link); Here's another bogus empty check. > wl_list_init(&ivisurf->order.link); Init before insert is redundant. wl_list_insert() does not read the link before overwriting it, so it is ok if the link is "uninitialized". > } > > - wl_list_init(&ivilayer->order.surface_list); > - wl_list_for_each(ivisurf, > &ivilayer->pending.surface_list, > - pending.link) { > - if (!wl_list_empty(&ivisurf->order.link)) { > - wl_list_remove(&ivisurf->order.link); > - wl_list_init(&ivisurf->order.link); > - } > - > - wl_list_insert(&ivilayer->order.surface_list, > - &ivisurf->order.link); > - add_ordersurface_to_layer(ivisurf, ivilayer); > - ivisurf->event_mask |= IVI_NOTIFICATION_ADD; > - } > + wl_list_insert(&ivilayer->order.surface_list, > + &ivisurf->order.link); > + add_ordersurface_to_layer(ivisurf, ivilayer); > + ivisurf->event_mask |= IVI_NOTIFICATION_ADD; > } > + > + ivilayer->order.dirty = 0; > } > } > > @@ -997,8 +982,6 @@ clear_surface_pending_list(struct ivi_layout_layer > *ivilayer) > > wl_list_init(&surface_link->pending.link); > } > - > - ivilayer->event_mask |= IVI_NOTIFICATION_REMOVE; > } > > static void > @@ -1015,8 +998,6 @@ clear_surface_order_list(struct ivi_layout_layer > *ivilayer) > > wl_list_init(&surface_link->order.link); > } > - > - ivilayer->event_mask |= IVI_NOTIFICATION_REMOVE; > } > > static void > @@ -2102,7 +2083,7 @@ ivi_layout_layer_set_render_order(struct > ivi_layout_layer *ivilayer, > } > } > > - ivilayer->event_mask |= IVI_NOTIFICATION_ADD; > + ivilayer->order.dirty = 1; > > return IVI_SUCCEEDED; > } > @@ -2526,7 +2507,7 @@ ivi_layout_layer_add_surface(struct ivi_layout_layer > *ivilayer, > } > } > > - ivilayer->event_mask |= IVI_NOTIFICATION_ADD; > + ivilayer->order.dirty = 1; > > return IVI_SUCCEEDED; > } > @@ -2554,7 +2535,7 @@ ivi_layout_layer_remove_surface(struct ivi_layout_layer > *ivilayer, > } > } > > - remsurf->event_mask |= IVI_NOTIFICATION_REMOVE; > + ivilayer->order.dirty = 1; > } > > static int32_t Other than the mentioned above, the patch looks good to me. Using 'dirty' instead of those flags sounds sensible, though I'm not too familiar with the architecture here. Thanks, pq
pgpGNneFi5jl6.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
