Error while executing Layer manager

Ucan, Emre (ADITG/SW1) eucan at de.adit-jv.com
Fri Feb 5 10:43:32 EST 2016


Hi,

It is better to discuss this topic in genivi-ivi-layer-management at lists.genivi.org. Because LayerManagerControl is a part of wayland-ivi-extension.

The error message: "ivi_controller not available" is sent when ilmControl cannot bind to weston-ivi-shell. This means weston-ivi-shell is no longer running at the point of the LayerManagerControl call.

Best regards

Emre Ucan
Software Group I (ADITG/SW1)

Tel. +49 5121 49 6937

> -----Original Message-----
> From: Kishore Divvela -ERS, HCL Tech [mailto:kishore-d at hcl.com]
> Sent: Freitag, 5. Februar 2016 16:33
> To: Pekka Paalanen; Ucan, Emre (ADITG/SW1)
> Cc: Nobuhiko Tanibata; securitycheck at denso.co.jp; Natsume, Wataru
> (ADITJ/SWG); wayland-devel at lists.freedesktop.org
> Subject: Error while executing Layer manager
> 
> Hello,
> 
> I am getting error while executing LayerManagerControl, ivi extension i am
> using is V1.3
> 
> Myplatform is Wayland 1.6, weston 1.6 , wayland ivi extension is V1.3
> 
>  #LayerManagerControl get surfaces
> ivi_controller not available
> [Warning] The ilm_control_context is already destroyed Interpreter error:
> failed
> 
> 
> Note : it works with wayland ivi extension is  V1.2.
> 
> Regards,
> Kishore
> ________________________________________
> From: wayland-devel <wayland-devel-bounces at lists.freedesktop.org> on
> behalf of Pekka Paalanen <ppaalanen at gmail.com>
> Sent: 05 February 2016 19:38
> To: Ucan, Emre (ADITG/SW1)
> Cc: Nobuhiko Tanibata; securitycheck at denso.co.jp; Natsume,  Wataru
> (ADITJ/SWG); wayland-devel at lists.freedesktop.org
> Subject: Re: [PATCH weston] hmi-controller: remove duplicate
> commit_changes in random mode
> 
> On Thu, 4 Feb 2016 16:31:50 +0000
> "Ucan, Emre (ADITG/SW1)" <eucan at de.adit-jv.com> wrote:
> 
> > Hi Pekka,
> >
> > I am for removing the remaining bits of code which is wrongly assuming
> > that a surface can be shown in multiple layers.
> 
> Hi Emre,
> 
> sounds good.
> 
> >  Afterwards we have to redesign a big part of ivi_layout API to
> > support this feauture.
> >
> > My ideas for the redesign so far:
> > The most basic approach would be that an ivi_surface would have a
> > weston_view for every layer/screen combination.
> 
> Yes, I believe that is how it could/should work.
> 
> > But ivi_layout API
> > does not make much sense than. Because when controller calls
> > ivi_layout_surface_set_destination_rectangle, should we scale all
> > views ? I think it does not make sense to scale all views.
> 
> Indeed, that was a bit weird, like a surface's position on any layer being
> recorded with the surface, which means you cannot have per-layer positions.
> The same with layer vs. screen, IIRC.
> 
> > Therefore,
> > the set APIs should be changed to get ivi_views and not ivi_surfaces.
> > This would mean:
> >
> > - Modifying every ivi_layout_surface_set* API
> > - Implementing new APIs to get layer/screen specific ivi_views for a
> > given ivi_surface
> > - ivi-layers should list ivi_views and not ivi_surfaces
> > - The implementation of commit_changes should be updated accordingly.
> >
> > I am planning to do all these changes, but at first we can remove old
> > code. Because it is very unlikely that we can reuse.
> 
> Do you still think that ivi-layer should have a size and a position and all the
> other attributes?
> Or should it be used only for grouping and z-ordering ivi-surfaces/views, and
> linking them to a screen?
> 
> Could an ivi-layer be on multiple screens? If you can restrict an ivi-layer to a
> single ivi-screen, I think that would simplify things a lot.
> 
> It seems you are moving closer to weston's internal architecture with
> surfaces, views, and layers. :-)
> 
> 
> Thanks,
> pq
> 
> > -----Original Message-----
> > From: wayland-devel
> > [mailto:wayland-devel-bounces at lists.freedesktop.org] On Behalf Of
> > Pekka Paalanen Sent: Dienstag, 2. Februar 2016 16:43 To: Nobuhiko
> > Tanibata Cc: securitycheck at denso.co.jp; Natsume, Wataru (ADITJ/SWG);
> > wayland-devel at lists.freedesktop.org Subject: Re: [PATCH weston]
> > hmi-controller: remove duplicate commit_changes in random mode
> >
> > Hi,
> >
> > I don't think this patch is ready to be merged, more below.
> >
> > TL;DR: It would probably be best to just ignore which ivilayer an
> > ivisurface is currently on, and always just remove and add. That would
> > be the simplest solution, if the ivilayout API just allows it.
> >
> > Specifically, do not clear the layer's surface list in advance. Just
> > go over each surface and reassign the layer for it. Then doing a
> > single commit_changes() in the end will ensure that ivisurfaces get
> > atomically moved from layer to layer as appropriate, and there won't
> > be any multiple assignments.
> >
> >
> > On Fri, 25 Dec 2015 13:47:15 +0900
> > Nobuhiko Tanibata <nobuhiko_tanibata at xddp.denso.co.jp> wrote:
> >
> > > From: Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp>
> > >
> > > Previous code cleaned up surfaces in layer once and then added
> > > surfaces to a layer in random. In this flow, two commitchanges are
> > > required.
> > >
> > > This patch proposes that it avoids calling add_surface if a surface
> > > is already added to a layer in random. In this flow, cleaning up
> > > surfaces is not required.
> > >
> > > Signed-off-by: Nobuhiko Tanibata
> > > <NOBUHIKO_TANIBATA at xddp.denso.co.jp> ---  ivi-shell/hmi-
> controller.c
> > > | 25 ++++++++++++++++---------
> > >  1 file changed, 16 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/ivi-shell/hmi-controller.c b/ivi-shell/hmi-controller.c
> > > index 77426bc..8a81f5c 100644
> > > --- a/ivi-shell/hmi-controller.c
> > > +++ b/ivi-shell/hmi-controller.c
> > > @@ -418,24 +418,18 @@ mode_random_replace(struct hmi_controller
> > > *hmi_ctrl, struct ivi_layout_surface *ivisurf  = NULL;
> > >     const uint32_t duration =
> > > hmi_ctrl->hmi_setting->transition_duration; int32_t i = 0;
> > > +   int32_t j = 0;
> > >     int32_t layer_idx = 0;
> > > +   int32_t surface_len_on_layer = 0;
> > > +   struct ivi_layout_surface **ivisurfs = NULL;
> > >
> > >     layers = MEM_ALLOC(sizeof(*layers) * hmi_ctrl->screen_num);
> > >
> > >     wl_list_for_each(application_layer, layer_list, link) {
> > >             layers[layer_idx] = application_layer;
> > > -
> > > ivi_layout_interface->layer_set_render_order(layers[layer_idx]-
> >ivilayer,
> > > -                                                   NULL, 0);
> > >             layer_idx++;
> > >     }
> > >
> > > -   /*
> > > -    * This commit change is needed because ivisurface can not
> > > belongs to several layers
> > > -    * at the same time. So ivisurfaces shall be removed from
> > > layers once and then set them
> > > -    * to layers randomly.
> > > -    */
> > > -   ivi_layout_interface->commit_changes();
> >
> > This is a lengthy side-comment that came to my mind while looking at
> > the code:
> >
> > An ivisurface indeed cannot belong to several layers at the same time,
> > but I don't think that would happen (break anything) even if you
> > literally removed only this commit_changes() call.
> >
> > That is because the staging list (ivi_layout_surface::pending) and the
> > current list (ivi_layout_surface::order) are separate. I believe it is
> > fine to have an ivi_layout_surface on one layer in the current list
> > and on a different layer in the staging list. You have to make that
> > work anyway, because it is the only way to move an ivisurface from one
> > layer to another without potentially causing it to disappear from the
> > screen in between.
> >
> > When the connecting structures, that were meant to allow an ivisurface
> > to be on several layers, were removed, the code automatically became
> > such that it naturally avoids attempting to have an ivisurface on
> > multiple layers, even if you tried to do that from the ivilayout
> > external API. The list manipulations just work, and the last
> > assignment will prevail.
> >
> > There are also leftovers in the code, that go through a list of all
> > ivisurfaces just to find an ivisurface with the right ivi-id when you
> > already have a pointer to the ivisurface with the ivi-id you are
> > looking for. Since there cannot be multiple ivisurfaces with the same
> > ivi-id, the search will only check if the given ivisurface is on the
> > given list. If the list by definition contains all ivisurfaces, the
> > check is moot. I think this happens in functions like
> > ivi_layout_layer_add_surface() and
> > ivi_layout_layer_set_render_order().
> >
> > But, as said, that is an aside. I think there is a lot that could be
> > simplified and cleaned up in the surface/layer/screen management, but
> > that's off-topic here.
> >
> > > -
> > >     for (i = 0; i < surface_length; i++) {
> > >             ivisurf = pp_surface[i];
> > >
> > > @@ -463,6 +457,19 @@ mode_random_replace(struct hmi_controller
> > > *hmi_ctrl, surface_width,
> > >
> > > surface_height);
> > >
> > > +           ivi_layout_interface
> > > +
> > > + ->get_surfaces_on_layer(layers[layer_idx]->ivilayer,
> > > +
> > > &surface_len_on_layer,
> > > +                                           &ivisurfs);
> >
> > Please, try not to split around -> as it looks odd.
> >
> > This call allocates memory and stores the pointer to 'ivisurfs', which
> > is then never freed, leaking.
> >
> > > +
> > > +           for (j = 0; j < surface_len_on_layer; j++) {
> > > +                   if (ivisurf == ivisurfs[j])
> > > +                           break;
> > > +           }
> > > +
> > > +           if (j < surface_len_on_layer)
> > > +                   continue;
> > > +
> > >
> > > ivi_layout_interface->layer_add_surface(layers[layer_idx]->ivilayer,
> > > ivisurf); }
> > >
> >
> > I understand the idea here: if the ivisurface is already on the right
> > layer, do not reassign the layer. However, the way it is implemented
> > is wasteful. There are several loops over various lists and arrays
> > that could simply be avoided if you embrace the fact that adding an
> > ivisurface to a layer will always remove it from the layer it was on,
> > if any. The list operations allow you to do that in O(1) time without
> > discovering in which list the item was on.
> >
> > In fact, that is what this patch assumes already: the
> > layer_set_render_order(NULL) call is removed, so ivisurfaces are still
> > on their original ivilayers. Then you do a lot of work to determine if
> > the ivisurface is already on the correct ivilayer. If the ivisurface
> > is not on the correct ivilayer, then you assign it to the correct
> > (new) ivilayer - but you do not remove it from the original ivilayer
> > first.
> >
> > In other words all the checking is already in vain.
> >
> > I think there is lingering confusion about what layer_add_surface()
> > does. Previously it was intended that an ivisurface can be on several
> > ivilayers, which implies that layer_add_surface() must not remove the
> > ivisurface from any existing layers. Now that an ivisurface can be at
> > most on one ivilayer, layer_add_surface() must either remove from the
> > old layer or fail if a layer is already assigned.
> >
> > Looking at the current implementation of
> > ivi_layout_layer_add_surface(), if will not fail and will happily
> > remove the ivisurface from its previous ivilayer. It also checks that
> > the ivisurface is in layout->surface_list, which I believe it always
> > true and the loop is useless.
> >
> > It seems like we are well on our way encoding the fact that an
> > ivisurface can be on at most one ivilayer. However, ivilayout API is
> > still written assuming the contrary, which both complicates its use
> > and calls for adding more checks in its implementation.
> >
> > Do you want to keep the ivilayout API with the assumption that an
> > ivisurface can be on multiple ivilayers (but fail if you actually try
> > to do that, because ivilayout.c does not currently support it)?
> >
> > Or, do you want to completely drop the idea that an ivisurface can be
> > on multiple ivilayers?
> >
> >
> > Thanks,
> > pq




More information about the genivi-ivi-layer-management mailing list