Re: [PATCH v2 4/4] OMAPDSS: APPLY: Remove display dependency from overlay and manager checks

[Archit Taneja <a0393947@ti.com>]

On Tuesday 08 May 2012 02:22 PM, Tomi Valkeinen wrote:
> On Tue, 2012-05-08 at 13:08 +0530, Archit Taneja wrote:
>> On Tuesday 08 May 2012 12:46 PM, Tomi Valkeinen wrote:
>
>>> 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.

>
>> 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
>