On Tuesday 08 May 2012 02:22 PM, Tomi Valkeinen wrote:

On Tue, 2012-05-08 at 13:08 +0530, Archit Taneja wrote:

quoted

On Tuesday 08 May 2012 12:46 PM, Tomi Valkeinen wrote:

quoted

quoted

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.

Right, I can do this for dss_mgr_check() and dss_ovl_check(), but if I
you see the misc cleanup patch series I posted, I use
dss_mgr_get_timings()  in dispc.c functions to remove some
omap_dss_device references. These DISPC functions are called in
dispc_ovl_setup(), I could pass manager timings in dispc_ovl_setup() so
that it comes directly from apply.c. What do you say?

Yes, I guess dispc_ovl_setup should get the timings pointer also. It's
getting a bit complex, but I don't see any simpler way. Especially dispc
functions should be quite self contained, dispc.c should not call
elsewhere, but it should get all the needed data as parameters.

Ok, you are right about dispc.c querying stuff from apply, it should all 
be one way from apply/interface drivers to the dispc, and if that's the 
case, then it has to get more complex. We could just pass the data in a 
nicer way to make the number of arguments passed to dispc functions seem 
small.

quoted

I guess we can get away from exposing private data in apply to DSS, but
it might get a bit harder when we try to remove other omap_dss_device
references in the future. I'm just guessing though.

We have two cases here: 1) using timings when doing apply.c things, as
your patches do. In these cases the timings can be passed from apply.c
as parameters. 2) using timings "outside" apply.c. I guess we currently
don't have these, but for those we could have a get_timings() function
that does take the locks. And similarly for other configs.

Of course, that's still not perfect. Even if get_timings() is protected
properly, there's no guarantee that the timings won't change right after
get_timings has returned. But hopefully all writes and reads to timings
would go via the same place, for example dpi.c. And dpi.c's own lock
will protect the change of timings while another thread is using them.

Btw, there's a typo in "OMAPDSS: DISPC: Remove usage of
dispc_mgr_get_device()"'s description, dss_mgr_get_device() should be
get_timings in one place.

Ah ok, I am removing the dss_mgr_get_timings() usage anyway, I'll pass 
timings through dispc_ovl_setup().

I am also going to make the first patch of the newer set 'OMAPDSS: 
DPI/HDMI: Apply manager timings even if panel is disabled' a part of 
this series since it's more related to applying timings. I posted it in 
the next series since I had already posted out this series.

Archit

  Tomi