On Tue, 2012-05-08 at 10:33 +0530, Archit Taneja wrote:
> On Monday 07 May 2012 08:33 PM, Tomi Valkeinen wrote:
> > On Thu, 2012-05-03 at 12:37 +0530, Archit Taneja wrote:
> >> In order to check the validity of overlay and manager info, there was a
> >> need to
> >> use the omap_dss_device struct to get the panel resolution. The manager's
> >> private data in APPLY now contains the manager timings. Hence, we don't
> >> need to
> >> rely on the display resolution any more.
> >>
> >> Create a function dss_mgr_get_timings() which returns the timings in
> >> manager's
> >> private data. Remove the need of passing omap_dss_device structs in the
> >> functions which check for overlay and managers.
> >>
> >> Have some initial values for manager timings in apply_init(), these would
> >> ensure
> >> that manager checks don't fail if an interface driver or a panel driver
> >> hasn't
> >> set the manager timings yet.
> >>
> >> Signed-off-by: Archit Taneja<arc...@ti.com>
> >
> >> +struct omap_video_timings *dss_mgr_get_timings(struct
> >> omap_overlay_manager *mgr)
> >> +{
> >> + struct mgr_priv_data *mp = get_mgr_priv(mgr);
> >> +
> >> + return&mp->timings;
> >> +}
> >
> > This one returns a pointer into apply.c's internal data structures. The
> > safest way would be to return a copy, but as it's an omapdss internal
> > function, I think it's enough to return a pointer to a const struct.
>
> Okay I'll fix that, I was a bit concerned about the locking here, I use
> this function in the later series to remove some dssdev references in
> dispc.c. I traced the paths and saw that in all cases this function
> would be protected by the data_lock spinlock, but not the apply_lock
> mutex in all cases. Any thoughts on this?Hmm, you're right, locking here gets a bit confusing. set_timings has locks, so logically get_timings should also. But I guess all the uses of get_timings happens via apply.c, and apply.c already holds the data_lock, as you said? Making the get_timings function public (inside omapdss) is a bit nasty, as it's quite easy to call it without having the appropriate locks. And actually there's no way to acquire the locks outside apply.c I'm not sure if it applies here, but as a general strategy, I suggest doing things in apply.c that require data from apply.c's internal data. When that's not possible, apply.c should call the functions outside apply.c, and pass the internal data as parameters (like calls to dispc). In your case, I for example see dss_mgr_check() calling dss_mgr_get_timings(). It would be quite easy to pass the timings to dss_mgr_check() from apply.c, thus removing the need to call the function. And, as you see, dss_mgr_check() already has a bunch of parameters, and the idea is the same with those: give the params, so that dss_mgr_check() doesn't need to ask for them. Tomi
signature.asc
Description: This is a digitally signed message part
