Re: [PATCH v2 4/4] OMAPDSS: APPLY: Remove display dependency from overlay and manager checks
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] OMAPDSS: APPLY: Remove display dependency from overlay and manager checks
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. > 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. Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH v2 4/4] OMAPDSS: APPLY: Remove display dependency from overlay and manager checks
On Tuesday 08 May 2012 12:46 PM, Tomi Valkeinen wrote:
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
+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?
Yes.
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.
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?
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.
Archit
Tomi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] OMAPDSS: APPLY: Remove display dependency from overlay and manager checks
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
> >
> >> +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
Re: [PATCH v2 4/4] OMAPDSS: APPLY: Remove display dependency from overlay and manager checks
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
+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?
Archit
Tomi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] OMAPDSS: APPLY: Remove display dependency from overlay and manager checks
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
> +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.
Tomi
signature.asc
Description: This is a digitally signed message part
