Re: [PATCH v2 1/4] OMAPDSS: APPLY: Add manager timings as extra_info in private data

2012-05-08 Thread Tomi Valkeinen 
On Tue, 2012-05-08 at 09:54 +0530, Archit Taneja wrote:
 Hi,
 
 On Monday 07 May 2012 08:17 PM, Tomi Valkeinen wrote:
  Hi,

  dss_ovl|mgr_enable|disable  functions handle GO, so you could have a
  look at them. However, setting the timings is a bit different, so I'm
  not sure how it should be done.
 
  I think we have a few different options:
 
  - Separate omapdss internal function (in apply.c) that can be used to
  set GO after set_timings
 
  - set GO in dss_mgr_set_timings(), but don't block
 
  - set GO in dss_mgr_set_timings(), and block until the changes are in HW
  (this is more or less what the dss_ovl|mgr_enable|disable  functions
  do).
 
  The first one would be good if the interface drivers would need to set
  multiple configurations, and we don't want to block after each set call.
  But we don't have anything like that, at least currently.
 
  The second one avoids blocking, but could perhaps cause problems because
  the timings are not actually used yet when the function returns.
 
  I don't see any problem with the last option, so I'm slightly leaning
  towards it.
 
 The 3rd option looks good to me too, but I'm wondering if we would need 
 to do the same things with all manager parameters which are in shadow 
 registers. Like in dpi.c, in dpi_set_mode() we set the DISPC_POL_FREQ 
 and DISPC_DIVISORo registers, writing GO for each parameter would be in 
 efficient, it's good that it doesn't happen much often. Maybe we could 
 group the rest of these parameters.

Hmm, that's true.

We could make shortcuts with settings that are only changed while the
display is off. For example, currently DISPC_POL_FREQ cannot be changed
dynamically, so it could only be set when before enabling the output.
However, it seems we currently do call dispc_mgr_set_pol_freq() in
dpi_set_mode(), which could be called when the output is enabled. But
even then we always set the same values, so it doesn't matter.

For DISPC_DIVISORo, I guess we could make a shortcut with that also.
While it is a shadow register, when we are changing the dss clocks we
also set non-shadowed registers. So I think the best way to change clock
frequencies is to turn off the output temporarily.

Perhaps all the shadow registers currently being set directly from
interface drivers are such settings? The code should still be changed so
that we only touch the registers when enabling the output.

But, of course, a better design and also more future proof would be to
handle all shadow registers properly, even if we currently only set them
while the output is off. That may be left for future, though.

In any case, I think we should mark clearly the places where we set
shadow registers directly, without using the apply.c. Perhaps we should
even mark all the dispc functions that set shadow registers, like
dispc_mgr_set_pol_freq_sh(). But let's not do that now either, as we're
quite close to merge window.

 Tomi



signature.asc
Description: This is a digitally signed message part

Re: [PATCH v2 1/4] OMAPDSS: APPLY: Add manager timings as extra_info in private data

2012-05-07 Thread Tomi Valkeinen 
Hi,

On Thu, 2012-05-03 at 12:37 +0530, Archit Taneja wrote:
 DISPC manager size and DISPC manager blanking parameters(for LCD managers)
 follow the shadow register programming model. Currently, they are programmed
 directly by the interface drivers.
 
 To configure manager timings using APPLY, there is a need to introduce extra
 info flags for managers, similar to what is done for overlays. This is needed
 because timings aren't a part of overlay_manager_info struct configured by a
 user of DSS, they are configured internally by the interface or panel drivers.
 
 Add dirty and shadow_dirty extra_info flags for managers, update these flags
 at the appropriate places. Rewrite the function extra_info_update_ongoing()
 slightly as checking for manager's extra_info flags can simplify the code a 
 bit.
 
 Create function dss_mgr_set_timings() which applies the new manager timings to
 extra_info.
 
 Signed-off-by: Archit Taneja arc...@ti.com

snip

 +static void dss_apply_mgr_timings(struct omap_overlay_manager *mgr,
 + struct omap_video_timings *timings)
 +{
 + struct mgr_priv_data *mp = get_mgr_priv(mgr);
 +
 + mp-timings = *timings;
 + mp-extra_info_dirty = true;
 +}
 +
 +void dss_mgr_set_timings(struct omap_overlay_manager *mgr,
 + struct omap_video_timings *timings)
 +{
 + unsigned long flags;
 +
 + mutex_lock(apply_lock);
 +
 + spin_lock_irqsave(data_lock, flags);
 +
 + dss_apply_mgr_timings(mgr, timings);
 +
 + spin_unlock_irqrestore(data_lock, flags);
 +
 + mutex_unlock(apply_lock);
 +}

With this, dpi.c  others still need to use dispc_mgr_go(), which is
something that should be removed and done only from apply.c.

dss_ovl|mgr_enable|disable functions handle GO, so you could have a
look at them. However, setting the timings is a bit different, so I'm
not sure how it should be done.

I think we have a few different options:

- Separate omapdss internal function (in apply.c) that can be used to
set GO after set_timings

- set GO in dss_mgr_set_timings(), but don't block

- set GO in dss_mgr_set_timings(), and block until the changes are in HW
(this is more or less what the dss_ovl|mgr_enable|disable functions
do).

The first one would be good if the interface drivers would need to set
multiple configurations, and we don't want to block after each set call.
But we don't have anything like that, at least currently.

The second one avoids blocking, but could perhaps cause problems because
the timings are not actually used yet when the function returns.

I don't see any problem with the last option, so I'm slightly leaning
towards it.

 Tomi



signature.asc
Description: This is a digitally signed message part

Re: [PATCH v2 1/4] OMAPDSS: APPLY: Add manager timings as extra_info in private data

2012-05-07 Thread Archit Taneja 

Hi,

On Monday 07 May 2012 08:17 PM, Tomi Valkeinen wrote:

Hi,

On Thu, 2012-05-03 at 12:37 +0530, Archit Taneja wrote:

DISPC manager size and DISPC manager blanking parameters(for LCD managers)
follow the shadow register programming model. Currently, they are programmed
directly by the interface drivers.

To configure manager timings using APPLY, there is a need to introduce extra
info flags for managers, similar to what is done for overlays. This is needed
because timings aren't a part of overlay_manager_info struct configured by a
user of DSS, they are configured internally by the interface or panel drivers.

Add dirty and shadow_dirty extra_info flags for managers, update these flags
at the appropriate places. Rewrite the function extra_info_update_ongoing()
slightly as checking for manager's extra_info flags can simplify the code a bit.

Create function dss_mgr_set_timings() which applies the new manager timings to
extra_info.

Signed-off-by: Archit Tanejaarc...@ti.com


snip


+static void dss_apply_mgr_timings(struct omap_overlay_manager *mgr,
+   struct omap_video_timings *timings)
+{
+   struct mgr_priv_data *mp = get_mgr_priv(mgr);
+
+   mp-timings = *timings;
+   mp-extra_info_dirty = true;
+}
+
+void dss_mgr_set_timings(struct omap_overlay_manager *mgr,
+   struct omap_video_timings *timings)
+{
+   unsigned long flags;
+
+   mutex_lock(apply_lock);
+
+   spin_lock_irqsave(data_lock, flags);
+
+   dss_apply_mgr_timings(mgr, timings);
+
+   spin_unlock_irqrestore(data_lock, flags);
+
+   mutex_unlock(apply_lock);
+}


With this, dpi.c  others still need to use dispc_mgr_go(), which is
something that should be removed and done only from apply.c.


Ah ok, so with this set, dss_mgr_set_timings() doesn't ensure that the 
configuration is taken in, the configuration may go in the next 
overlay/manager enable or the next mgr_apply, but that may happen much 
later.




dss_ovl|mgr_enable|disable  functions handle GO, so you could have a
look at them. However, setting the timings is a bit different, so I'm
not sure how it should be done.

I think we have a few different options:

- Separate omapdss internal function (in apply.c) that can be used to
set GO after set_timings

- set GO in dss_mgr_set_timings(), but don't block

- set GO in dss_mgr_set_timings(), and block until the changes are in HW
(this is more or less what the dss_ovl|mgr_enable|disable  functions
do).

The first one would be good if the interface drivers would need to set
multiple configurations, and we don't want to block after each set call.
But we don't have anything like that, at least currently.

The second one avoids blocking, but could perhaps cause problems because
the timings are not actually used yet when the function returns.

I don't see any problem with the last option, so I'm slightly leaning
towards it.


The 3rd option looks good to me too, but I'm wondering if we would need 
to do the same things with all manager parameters which are in shadow 
registers. Like in dpi.c, in dpi_set_mode() we set the DISPC_POL_FREQ 
and DISPC_DIVISORo registers, writing GO for each parameter would be in 
efficient, it's good that it doesn't happen much often. Maybe we could 
group the rest of these parameters.


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

[PATCH v2 1/4] OMAPDSS: APPLY: Add manager timings as extra_info in private data

2012-05-03 Thread Archit Taneja 
DISPC manager size and DISPC manager blanking parameters(for LCD managers)
follow the shadow register programming model. Currently, they are programmed
directly by the interface drivers.

To configure manager timings using APPLY, there is a need to introduce extra
info flags for managers, similar to what is done for overlays. This is needed
because timings aren't a part of overlay_manager_info struct configured by a
user of DSS, they are configured internally by the interface or panel drivers.

Add dirty and shadow_dirty extra_info flags for managers, update these flags
at the appropriate places. Rewrite the function extra_info_update_ongoing()
slightly as checking for manager's extra_info flags can simplify the code a bit.

Create function dss_mgr_set_timings() which applies the new manager timings to
extra_info.

Signed-off-by: Archit Taneja arc...@ti.com
---
 drivers/video/omap2/dss/apply.c |   91 +--
 drivers/video/omap2/dss/dss.h   |2 +
 2 files changed, 80 insertions(+), 13 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index b10b3bc..42c3854 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -99,6 +99,11 @@ struct mgr_priv_data {
 
/* If true, a display is enabled using this manager */
bool enabled;
+
+   bool extra_info_dirty;
+   bool shadow_extra_info_dirty;
+
+   struct omap_video_timings timings;
 };
 
 static struct {
@@ -261,6 +266,20 @@ static bool need_isr(void)
if (mp-shadow_info_dirty)
return true;
 
+   /*
+* NOTE: we don't check extra_info flags for disabled
+* managers, once the manager is enabled, the extra_info
+* related manager changes will be taken in by HW.
+*/
+
+   /* to write new values to registers */
+   if (mp-extra_info_dirty)
+   return true;
+
+   /* to set GO bit */
+   if (mp-shadow_extra_info_dirty)
+   return true;
+
list_for_each_entry(ovl, mgr-overlays, list) {
struct ovl_priv_data *op;
 
@@ -305,7 +324,7 @@ static bool need_go(struct omap_overlay_manager *mgr)
 
mp = get_mgr_priv(mgr);
 
-   if (mp-shadow_info_dirty)
+   if (mp-shadow_info_dirty || mp-shadow_extra_info_dirty)
return true;
 
list_for_each_entry(ovl, mgr-overlays, list) {
@@ -320,20 +339,16 @@ static bool need_go(struct omap_overlay_manager *mgr)
 /* returns true if an extra_info field is currently being updated */
 static bool extra_info_update_ongoing(void)
 {
-   const int num_ovls = omap_dss_get_num_overlays();
-   struct ovl_priv_data *op;
-   struct omap_overlay *ovl;
-   struct mgr_priv_data *mp;
+   const int num_mgrs = dss_feat_get_num_mgrs();
int i;
 
-   for (i = 0; i  num_ovls; ++i) {
-   ovl = omap_dss_get_overlay(i);
-   op = get_ovl_priv(ovl);
-
-   if (!ovl-manager)
-   continue;
+   for (i = 0; i  num_mgrs; ++i) {
+   struct omap_overlay_manager *mgr;
+   struct omap_overlay *ovl;
+   struct mgr_priv_data *mp;
 
-   mp = get_mgr_priv(ovl-manager);
+   mgr = omap_dss_get_overlay_manager(i);
+   mp = get_mgr_priv(mgr);
 
if (!mp-enabled)
continue;
@@ -341,8 +356,15 @@ static bool extra_info_update_ongoing(void)
if (!mp-updating)
continue;
 
-   if (op-extra_info_dirty || op-shadow_extra_info_dirty)
+   if (mp-extra_info_dirty || mp-shadow_extra_info_dirty)
return true;
+
+   list_for_each_entry(ovl, mgr-overlays, list) {
+   struct ovl_priv_data *op = get_ovl_priv(ovl);
+
+   if (op-extra_info_dirty || op-shadow_extra_info_dirty)
+   return true;
+   }
}
 
return false;
@@ -601,6 +623,22 @@ static void dss_mgr_write_regs(struct omap_overlay_manager 
*mgr)
}
 }
 
+static void dss_mgr_write_regs_extra(struct omap_overlay_manager *mgr)
+{
+   struct mgr_priv_data *mp = get_mgr_priv(mgr);
+
+   DSSDBGF(%d, mgr-id);
+
+   if (!mp-extra_info_dirty)
+   return;
+
+   dispc_mgr_set_timings(mgr-id, mp-timings);
+
+   mp-extra_info_dirty = false;
+   if (mp-updating)
+   mp-shadow_extra_info_dirty = true;
+}
+
 static void dss_write_regs_common(void)
 {
const int num_mgrs = omap_dss_get_num_overlay_managers();
@@ -654,6 +692,7 @@ static void dss_write_regs(void)
}