[PATCH 0/6] OMAPDSS: APPLY: Treat overlay manager timings as shadow registers

[PATCH 0/6] OMAPDSS: APPLY: Treat overlay manager timings as shadow registers

[Archit Taneja]

An overlay manager's timings (the manager size, and blanking parameters if an
LCD manager) are DISPC shadow registers, and they should hence follow the
correct programming model.

This set makes the timings a manager_info parameter. The interface drivers now
set the timings in manager_info instead of directly writing to registers.

This change also prevents the need to use display resolution for overlay
checks, hence making some of the APPLY functions less dependent on the display.

These patches apply over:

git://gitorious.org/linux-omap-dss2/linux.git dev

Archit Taneja (6):
  OMAPDSS: DISPC/RFBI: Use dispc_mgr_set_lcd_timings() for setting lcd
    size
  OMAPDSS: DISPC: Use a common function to set manager timings
  OMAPDSS: DISPC: Clean up manager timing/size functions
  OMAPDSS: MANAGER: Make DISPC timings a manager_info parameter
  OMAPDSS: MANAGER: Check validity of manager timings
  OMAPDSS: APPLY: Remove display dependency from overlay and manager
    checks

 drivers/video/omap2/dss/apply.c        |   46 +++++++++++++-----
 drivers/video/omap2/dss/dispc.c        |   78 +++++++++++++++++++-------------
 drivers/video/omap2/dss/dpi.c          |    6 ++-
 drivers/video/omap2/dss/dsi.c          |   11 ++++-
 drivers/video/omap2/dss/dss.h          |   16 +++---
 drivers/video/omap2/dss/dss_features.c |    6 +++
 drivers/video/omap2/dss/dss_features.h |    2 +
 drivers/video/omap2/dss/hdmi.c         |    6 ++-
 drivers/video/omap2/dss/manager.c      |   39 +++++++++++++++-
 drivers/video/omap2/dss/overlay.c      |   20 ++++-----
 drivers/video/omap2/dss/rfbi.c         |   27 ++++++++++-
 drivers/video/omap2/dss/sdi.c          |    5 ++-
 drivers/video/omap2/dss/venc.c         |   17 +++++---
 include/video/omapdss.h                |    2 +
 14 files changed, 200 insertions(+), 81 deletions(-)

-- 
1.7.5.4

[PATCH 1/6] OMAPDSS: DISPC/RFBI: Use dispc_mgr_set_lcd_timings() for setting lcd size

[Archit Taneja]

The RFBI driver uses dispc_mgr_set_lcd_size() to set the width and height of
the LCD manager. Replace this to use dispc_mgr_set_lcd_timings(), pass dummy
blanking parameters like done in the DSI driver.

This prevents the need to export dispc_mgr_set_lcd_size(), and use a common
function to set lcd timings.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dispc.c |    3 ++-
 drivers/video/omap2/dss/dss.h   |    1 -
 drivers/video/omap2/dss/rfbi.c  |   24 ++++++++++++++++++++++--
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index bddd64b..c6ac161 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -983,7 +983,8 @@ static void dispc_ovl_enable_replication(enum omap_plane plane, bool enable)
 	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), enable, shift, shift);
 }
 
-void dispc_mgr_set_lcd_size(enum omap_channel channel, u16 width, u16 height)
+static void dispc_mgr_set_lcd_size(enum omap_channel channel, u16 width,
+		u16 height)
 {
 	u32 val;
 	BUG_ON((width > (1 << 11)) || (height > (1 << 11)));
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 7080f5f..e8dea74 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -430,7 +430,6 @@ void dispc_ovl_set_channel_out(enum omap_plane plane,
 		enum omap_channel channel);
 
 void dispc_mgr_enable_fifohandcheck(enum omap_channel channel, bool enable);
-void dispc_mgr_set_lcd_size(enum omap_channel channel, u16 width, u16 height);
 u32 dispc_mgr_get_vsync_irq(enum omap_channel channel);
 u32 dispc_mgr_get_framedone_irq(enum omap_channel channel);
 bool dispc_mgr_go_busy(enum omap_channel channel);
diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
index 788a0ef..97b6c4a 100644
--- a/drivers/video/omap2/dss/rfbi.c
+++ b/drivers/video/omap2/dss/rfbi.c
@@ -304,13 +304,23 @@ static void rfbi_transfer_area(struct omap_dss_device *dssdev, u16 width,
 		u16 height, void (*callback)(void *data), void *data)
 {
 	u32 l;
+	struct omap_video_timings timings = {
+		.hsw		= 1,
+		.hfp		= 1,
+		.hbp		= 1,
+		.vsw		= 1,
+		.vfp		= 0,
+		.vbp		= 0,
+		.x_res		= width,
+		.y_res		= height,
+	};
 
 	/*BUG_ON(callback = 0);*/
 	BUG_ON(rfbi.framedone_callback != NULL);
 
 	DSSDBG("rfbi_transfer_area %dx%d\n", width, height);
 
-	dispc_mgr_set_lcd_size(dssdev->manager->id, width, height);
+	dispc_mgr_set_lcd_timings(dssdev->manager->id, &timings);
 
 	dispc_mgr_enable(dssdev->manager->id, true);
 
@@ -766,6 +776,16 @@ int omap_rfbi_prepare_update(struct omap_dss_device *dssdev,
 		u16 *x, u16 *y, u16 *w, u16 *h)
 {
 	u16 dw, dh;
+	struct omap_video_timings timings = {
+		.hsw		= 1,
+		.hfp		= 1,
+		.hbp		= 1,
+		.vsw		= 1,
+		.vfp		= 0,
+		.vbp		= 0,
+		.x_res		= *w,
+		.y_res		= *h,
+	};
 
 	dssdev->driver->get_resolution(dssdev, &dw, &dh);
 
@@ -784,7 +804,7 @@ int omap_rfbi_prepare_update(struct omap_dss_device *dssdev,
 	if (*w = 0 || *h = 0)
 		return -EINVAL;
 
-	dispc_mgr_set_lcd_size(dssdev->manager->id, *w, *h);
+	dispc_mgr_set_lcd_timings(dssdev->manager->id, &timings);
 
 	return 0;
 }
-- 
1.7.5.4

[PATCH 2/6] OMAPDSS: DISPC: Use a common function to set manager timings

[Archit Taneja]

Currently, a LCD manager's timings is set by dispc_mgr_set_lcd_timings() and TV
manager's timings is set by dispc_set_digit_size(). Use a common function called
dispc_mgr_set_timings() which sets timings for both type of managers.

We finally want the interface drivers to use an overlay manager function to
configure it's timings, having a common DISPC function would make things
cleaner.

For LCD managers, dispc_mgr_set_timings() sets LCD size and blanking values, for
TV manager, it sets only the TV size since blanking values don't exist for TV.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dispc.c |   43 +++++++++++++++++++++-----------------
 drivers/video/omap2/dss/dpi.c   |    2 +-
 drivers/video/omap2/dss/dsi.c   |    4 +-
 drivers/video/omap2/dss/dss.h   |    3 +-
 drivers/video/omap2/dss/hdmi.c  |    3 +-
 drivers/video/omap2/dss/rfbi.c  |    4 +-
 drivers/video/omap2/dss/sdi.c   |    2 +-
 drivers/video/omap2/dss/venc.c  |    7 ++++-
 8 files changed, 37 insertions(+), 31 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index c6ac161..f1e5337 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -992,7 +992,7 @@ static void dispc_mgr_set_lcd_size(enum omap_channel channel, u16 width,
 	dispc_write_reg(DISPC_SIZE_MGR(channel), val);
 }
 
-void dispc_set_digit_size(u16 width, u16 height)
+static void dispc_mgr_set_digit_size(u16 width, u16 height)
 {
 	u32 val;
 	BUG_ON((width > (1 << 11)) || (height > (1 << 11)));
@@ -2341,37 +2341,42 @@ static void _dispc_mgr_set_lcd_timings(enum omap_channel channel, int hsw,
 }
 
 /* change name to mode? */
-void dispc_mgr_set_lcd_timings(enum omap_channel channel,
+void dispc_mgr_set_timings(enum omap_channel channel,
 		struct omap_video_timings *timings)
 {
 	unsigned xtot, ytot;
 	unsigned long ht, vt;
 
-	if (!_dispc_lcd_timings_ok(timings->hsw, timings->hfp,
-				timings->hbp, timings->vsw,
-				timings->vfp, timings->vbp))
-		BUG();
+	DSSDBG("channel %d xres %u yres %u\n", channel, timings->x_res,
+			timings->y_res);
+
+	if (dispc_mgr_is_lcd(channel)) {
+		if (!dispc_lcd_timings_ok(timings))
+			BUG();
 
-	_dispc_mgr_set_lcd_timings(channel, timings->hsw, timings->hfp,
-			timings->hbp, timings->vsw, timings->vfp,
-			timings->vbp);
+		_dispc_mgr_set_lcd_timings(channel, timings->hsw, timings->hfp,
+				timings->hbp, timings->vsw, timings->vfp,
+				timings->vbp);
 
-	dispc_mgr_set_lcd_size(channel, timings->x_res, timings->y_res);
+		dispc_mgr_set_lcd_size(channel, timings->x_res, timings->y_res);
 
-	xtot = timings->x_res + timings->hfp + timings->hsw + timings->hbp;
-	ytot = timings->y_res + timings->vfp + timings->vsw + timings->vbp;
+		xtot = timings->x_res + timings->hfp + timings->hsw +
+				timings->hbp;
+		ytot = timings->y_res + timings->vfp + timings->vsw +
+				timings->vbp;
 
-	ht = (timings->pixel_clock * 1000) / xtot;
-	vt = (timings->pixel_clock * 1000) / xtot / ytot;
+		ht = (timings->pixel_clock * 1000) / xtot;
+		vt = (timings->pixel_clock * 1000) / xtot / ytot;
 
-	DSSDBG("channel %d xres %u yres %u\n", channel, timings->x_res,
-			timings->y_res);
-	DSSDBG("pck %u\n", timings->pixel_clock);
-	DSSDBG("hsw %d hfp %d hbp %d vsw %d vfp %d vbp %d\n",
+		DSSDBG("pck %u\n", timings->pixel_clock);
+		DSSDBG("hsw %d hfp %d hbp %d vsw %d vfp %d vbp %d\n",
 			timings->hsw, timings->hfp, timings->hbp,
 			timings->vsw, timings->vfp, timings->vbp);
 
-	DSSDBG("hsync %luHz, vsync %luHz\n", ht, vt);
+		DSSDBG("hsync %luHz, vsync %luHz\n", ht, vt);
+	} else {
+		dispc_mgr_set_digit_size(timings->x_res, timings->y_res);
+	}
 }
 
 static void dispc_mgr_set_lcd_divisor(enum omap_channel channel, u16 lck_div,
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index faaf305..7dd7f9d 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -156,7 +156,7 @@ static int dpi_set_mode(struct omap_dss_device *dssdev)
 		t->pixel_clock = pck;
 	}
 
-	dispc_mgr_set_lcd_timings(dssdev->manager->id, t);
+	dispc_mgr_set_timings(dssdev->manager->id, t);
 
 	return 0;
 }
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 0d2c53f..b6cf03c 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -4219,12 +4219,12 @@ static int dsi_display_init_dispc(struct omap_dss_device *dssdev)
 		dispc_mgr_enable_stallmode(dssdev->manager->id, true);
 		dispc_mgr_enable_fifohandcheck(dssdev->manager->id, 1);
 
-		dispc_mgr_set_lcd_timings(dssdev->manager->id, &timings);
+		dispc_mgr_set_timings(dssdev->manager->id, &timings);
 	} else {
 		dispc_mgr_enable_stallmode(dssdev->manager->id, false);
 		dispc_mgr_enable_fifohandcheck(dssdev->manager->id, 0);
 
-		dispc_mgr_set_lcd_timings(dssdev->manager->id,
+		dispc_mgr_set_timings(dssdev->manager->id,
 			&dssdev->panel.timings);
 	}
 
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index e8dea74..da91822 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -407,7 +407,6 @@ void dispc_disable_sidle(void);
 void dispc_lcd_enable_signal_polarity(bool act_high);
 void dispc_lcd_enable_signal(bool enable);
 void dispc_pck_free_enable(bool enable);
-void dispc_set_digit_size(u16 width, u16 height);
 void dispc_enable_fifomerge(bool enable);
 void dispc_enable_gamma_table(bool enable);
 void dispc_set_loadmode(enum omap_dss_load_mode mode);
@@ -442,7 +441,7 @@ void dispc_mgr_enable_stallmode(enum omap_channel channel, bool enable);
 void dispc_mgr_set_tft_data_lines(enum omap_channel channel, u8 data_lines);
 void dispc_mgr_set_lcd_display_type(enum omap_channel channel,
 		enum omap_lcd_display_type type);
-void dispc_mgr_set_lcd_timings(enum omap_channel channel,
+void dispc_mgr_set_timings(enum omap_channel channel,
 		struct omap_video_timings *timings);
 void dispc_mgr_set_pol_freq(enum omap_channel channel,
 		enum omap_panel_config config, u8 acbi, u8 acb);
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index c4b4f69..56f6e9c 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -376,8 +376,7 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)
 	dispc_enable_gamma_table(0);
 
 	/* tv size */
-	dispc_set_digit_size(dssdev->panel.timings.x_res,
-			dssdev->panel.timings.y_res);
+	dispc_mgr_set_timings(dssdev->manager->id, &dssdev->panel.timings);
 
 	hdmi.ip_data.ops->video_enable(&hdmi.ip_data, 1);
 
diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
index 97b6c4a..a81ffcb 100644
--- a/drivers/video/omap2/dss/rfbi.c
+++ b/drivers/video/omap2/dss/rfbi.c
@@ -320,7 +320,7 @@ static void rfbi_transfer_area(struct omap_dss_device *dssdev, u16 width,
 
 	DSSDBG("rfbi_transfer_area %dx%d\n", width, height);
 
-	dispc_mgr_set_lcd_timings(dssdev->manager->id, &timings);
+	dispc_mgr_set_timings(dssdev->manager->id, &timings);
 
 	dispc_mgr_enable(dssdev->manager->id, true);
 
@@ -804,7 +804,7 @@ int omap_rfbi_prepare_update(struct omap_dss_device *dssdev,
 	if (*w = 0 || *h = 0)
 		return -EINVAL;
 
-	dispc_mgr_set_lcd_timings(dssdev->manager->id, &timings);
+	dispc_mgr_set_timings(dssdev->manager->id, &timings);
 
 	return 0;
 }
diff --git a/drivers/video/omap2/dss/sdi.c b/drivers/video/omap2/dss/sdi.c
index 8266ca0..741b834 100644
--- a/drivers/video/omap2/dss/sdi.c
+++ b/drivers/video/omap2/dss/sdi.c
@@ -107,7 +107,7 @@ int omapdss_sdi_display_enable(struct omap_dss_device *dssdev)
 	}
 
 
-	dispc_mgr_set_lcd_timings(dssdev->manager->id, t);
+	dispc_mgr_set_timings(dssdev->manager->id, t);
 
 	r = dss_set_clock_div(&dss_cinfo);
 	if (r)
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 13a20da..30bbb63 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -421,6 +421,7 @@ static int venc_power_on(struct omap_dss_device *dssdev)
 {
 	u32 l;
 	int r;
+	struct omap_video_timings timings;
 
 	venc_reset();
 	venc_write_config(venc_timings_to_config(&dssdev->panel.timings));
@@ -440,8 +441,10 @@ static int venc_power_on(struct omap_dss_device *dssdev)
 
 	venc_write_reg(VENC_OUTPUT_CONTROL, l);
 
-	dispc_set_digit_size(dssdev->panel.timings.x_res,
-			dssdev->panel.timings.y_res/2);
+	timings = dssdev->panel.timings;
+	timings.y_res /= 2;
+
+	dispc_mgr_set_timings(dssdev->manager->id, &timings);
 
 	r = regulator_enable(venc.vdda_dac_reg);
 	if (r)
-- 
1.7.5.4

[PATCH 3/6] OMAPDSS: DISPC: Clean up manager timing/size functions

[Archit Taneja]

Clean up the DISPC manager timings related function by:

- Create a common function to set size for LCD and TV.
- Create a common function to check timings for LCD and TV.
- Add dss params to get the range of manager size.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dispc.c        |   47 +++++++++++++++++--------------
 drivers/video/omap2/dss/dpi.c          |    2 +-
 drivers/video/omap2/dss/dss.h          |    3 +-
 drivers/video/omap2/dss/dss_features.c |    6 ++++
 drivers/video/omap2/dss/dss_features.h |    2 +
 5 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index f1e5337..46bcb55 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -983,21 +983,13 @@ static void dispc_ovl_enable_replication(enum omap_plane plane, bool enable)
 	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), enable, shift, shift);
 }
 
-static void dispc_mgr_set_lcd_size(enum omap_channel channel, u16 width,
+static void dispc_mgr_set_size(enum omap_channel channel, u16 width,
 		u16 height)
 {
 	u32 val;
-	BUG_ON((width > (1 << 11)) || (height > (1 << 11)));
-	val = FLD_VAL(height - 1, 26, 16) | FLD_VAL(width - 1, 10, 0);
-	dispc_write_reg(DISPC_SIZE_MGR(channel), val);
-}
 
-static void dispc_mgr_set_digit_size(u16 width, u16 height)
-{
-	u32 val;
-	BUG_ON((width > (1 << 11)) || (height > (1 << 11)));
 	val = FLD_VAL(height - 1, 26, 16) | FLD_VAL(width - 1, 10, 0);
-	dispc_write_reg(DISPC_SIZE_MGR(OMAP_DSS_CHANNEL_DIGIT), val);
+	dispc_write_reg(DISPC_SIZE_MGR(channel), val);
 }
 
 static void dispc_read_plane_fifo_sizes(void)
@@ -2286,6 +2278,12 @@ void dispc_mgr_enable_stallmode(enum omap_channel channel, bool enable)
 		REG_FLD_MOD(DISPC_CONTROL, enable, 11, 11);
 }
 
+static bool _dispc_mgr_size_ok(u16 width, u16 height)
+{
+	return width <= dss_feat_get_param_max(FEAT_PARAM_MGR_WIDTH) &&
+		height <= dss_feat_get_param_max(FEAT_PARAM_MGR_HEIGHT);
+}
+
 static bool _dispc_lcd_timings_ok(int hsw, int hfp, int hbp,
 		int vsw, int vfp, int vbp)
 {
@@ -2310,11 +2308,20 @@ static bool _dispc_lcd_timings_ok(int hsw, int hfp, int hbp,
 	return true;
 }
 
-bool dispc_lcd_timings_ok(struct omap_video_timings *timings)
+bool dispc_mgr_timings_ok(enum omap_channel channel,
+		struct omap_video_timings *timings)
 {
-	return _dispc_lcd_timings_ok(timings->hsw, timings->hfp,
-			timings->hbp, timings->vsw,
-			timings->vfp, timings->vbp);
+	bool timings_ok;
+
+	timings_ok = _dispc_mgr_size_ok(timings->x_res, timings->y_res);
+
+	if (dispc_mgr_is_lcd(channel))
+		timings_ok =  timings_ok && _dispc_lcd_timings_ok(timings->hsw,
+						timings->hfp, timings->hbp,
+						timings->vsw, timings->vfp,
+						timings->vbp);
+
+	return timings_ok;
 }
 
 static void _dispc_mgr_set_lcd_timings(enum omap_channel channel, int hsw,
@@ -2350,16 +2357,14 @@ void dispc_mgr_set_timings(enum omap_channel channel,
 	DSSDBG("channel %d xres %u yres %u\n", channel, timings->x_res,
 			timings->y_res);
 
-	if (dispc_mgr_is_lcd(channel)) {
-		if (!dispc_lcd_timings_ok(timings))
-			BUG();
+	if (!dispc_mgr_timings_ok(channel, timings))
+		BUG();
 
+	if (dispc_mgr_is_lcd(channel)) {
 		_dispc_mgr_set_lcd_timings(channel, timings->hsw, timings->hfp,
 				timings->hbp, timings->vsw, timings->vfp,
 				timings->vbp);
 
-		dispc_mgr_set_lcd_size(channel, timings->x_res, timings->y_res);
-
 		xtot = timings->x_res + timings->hfp + timings->hsw +
 				timings->hbp;
 		ytot = timings->y_res + timings->vfp + timings->vsw +
@@ -2374,9 +2379,9 @@ void dispc_mgr_set_timings(enum omap_channel channel,
 			timings->vsw, timings->vfp, timings->vbp);
 
 		DSSDBG("hsync %luHz, vsync %luHz\n", ht, vt);
-	} else {
-		dispc_mgr_set_digit_size(timings->x_res, timings->y_res);
 	}
+
+	dispc_mgr_set_size(channel, timings->x_res, timings->y_res);
 }
 
 static void dispc_mgr_set_lcd_divisor(enum omap_channel channel, u16 lck_div,
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index 7dd7f9d..cec1166 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -312,7 +312,7 @@ int dpi_check_timings(struct omap_dss_device *dssdev,
 	unsigned long pck;
 	struct dispc_clock_info dispc_cinfo;
 
-	if (!dispc_lcd_timings_ok(timings))
+	if (!dispc_mgr_timings_ok(dssdev->manager->id, timings))
 		return -EINVAL;
 
 	if (timings->pixel_clock = 0)
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index da91822..1dc336b 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -411,7 +411,8 @@ void dispc_enable_fifomerge(bool enable);
 void dispc_enable_gamma_table(bool enable);
 void dispc_set_loadmode(enum omap_dss_load_mode mode);
 
-bool dispc_lcd_timings_ok(struct omap_video_timings *timings);
+bool dispc_mgr_timings_ok(enum omap_channel channel,
+		struct omap_video_timings *timings);
 unsigned long dispc_fclk_rate(void);
 void dispc_find_clk_divs(bool is_tft, unsigned long req_pck, unsigned long fck,
 		struct dispc_clock_info *cinfo);
diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
index ce14aa6..1d10a01 100644
--- a/drivers/video/omap2/dss/dss_features.c
+++ b/drivers/video/omap2/dss/dss_features.c
@@ -311,6 +311,8 @@ static const struct dss_param_range omap2_dss_param_range[] = {
 	 * scaler cannot scale a image with width more than 768.
 	 */
 	[FEAT_PARAM_LINEWIDTH]			= { 1, 768 },
+	[FEAT_PARAM_MGR_WIDTH]			= { 1, 2048 },
+	[FEAT_PARAM_MGR_HEIGHT]			= { 1, 2048 },
 };
 
 static const struct dss_param_range omap3_dss_param_range[] = {
@@ -324,6 +326,8 @@ static const struct dss_param_range omap3_dss_param_range[] = {
 	[FEAT_PARAM_DSIPLL_LPDIV]		= { 1, (1 << 13) - 1},
 	[FEAT_PARAM_DOWNSCALE]			= { 1, 4 },
 	[FEAT_PARAM_LINEWIDTH]			= { 1, 1024 },
+	[FEAT_PARAM_MGR_WIDTH]			= { 1, 2048 },
+	[FEAT_PARAM_MGR_HEIGHT]			= { 1, 2048 },
 };
 
 static const struct dss_param_range omap4_dss_param_range[] = {
@@ -337,6 +341,8 @@ static const struct dss_param_range omap4_dss_param_range[] = {
 	[FEAT_PARAM_DSIPLL_LPDIV]		= { 0, (1 << 13) - 1 },
 	[FEAT_PARAM_DOWNSCALE]			= { 1, 4 },
 	[FEAT_PARAM_LINEWIDTH]			= { 1, 2048 },
+	[FEAT_PARAM_MGR_WIDTH]			= { 1, 2048 },
+	[FEAT_PARAM_MGR_HEIGHT]			= { 1, 2048 },
 };
 
 static const enum dss_feat_id omap2_dss_feat_list[] = {
diff --git a/drivers/video/omap2/dss/dss_features.h b/drivers/video/omap2/dss/dss_features.h
index c332e7d..3736367 100644
--- a/drivers/video/omap2/dss/dss_features.h
+++ b/drivers/video/omap2/dss/dss_features.h
@@ -91,6 +91,8 @@ enum dss_range_param {
 	FEAT_PARAM_DSIPLL_LPDIV,
 	FEAT_PARAM_DOWNSCALE,
 	FEAT_PARAM_LINEWIDTH,
+	FEAT_PARAM_MGR_WIDTH,
+	FEAT_PARAM_MGR_HEIGHT,
 };
 
 /* DSS Feature Functions */
-- 
1.7.5.4

[PATCH 4/6] OMAPDSS: MANAGER: Make DISPC timings a manager_info parameter

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

Make timings(omap_video_timing struct) an overlay_manager_info member, they are
now programmed via the apply mechanism used for programming shadow registers.

The interface driver now call the function dss_mgr_set_timings() which applies
the new timing parameters, rather than directly writing to DISPC registers.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dispc.c   |    5 ++++-
 drivers/video/omap2/dss/dpi.c     |    4 +++-
 drivers/video/omap2/dss/dsi.c     |   11 +++++++++--
 drivers/video/omap2/dss/dss.h     |    4 ++--
 drivers/video/omap2/dss/hdmi.c    |    5 ++++-
 drivers/video/omap2/dss/manager.c |   21 +++++++++++++++++++++
 drivers/video/omap2/dss/rfbi.c    |    7 +++++--
 drivers/video/omap2/dss/sdi.c     |    5 ++++-
 drivers/video/omap2/dss/venc.c    |   12 +++++++-----
 include/video/omapdss.h           |    2 ++
 10 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 46bcb55..63de49d 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -120,6 +120,8 @@ enum omap_color_component {
 };
 
 static void _omap_dispc_set_irqs(void);
+static void dispc_mgr_set_timings(enum omap_channel channel,
+		struct omap_video_timings *timings);
 
 static inline void dispc_write_reg(const u16 idx, u32 val)
 {
@@ -2211,6 +2213,7 @@ void dispc_mgr_setup(enum omap_channel channel,
 		dispc_mgr_enable_cpr(channel, info->cpr_enable);
 		dispc_mgr_set_cpr_coef(channel, &info->cpr_coefs);
 	}
+	dispc_mgr_set_timings(channel, &info->timings);
 }
 
 void dispc_mgr_set_tft_data_lines(enum omap_channel channel, u8 data_lines)
@@ -2348,7 +2351,7 @@ static void _dispc_mgr_set_lcd_timings(enum omap_channel channel, int hsw,
 }
 
 /* change name to mode? */
-void dispc_mgr_set_timings(enum omap_channel channel,
+static void dispc_mgr_set_timings(enum omap_channel channel,
 		struct omap_video_timings *timings)
 {
 	unsigned xtot, ytot;
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index cec1166..76e2cae 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -156,7 +156,9 @@ static int dpi_set_mode(struct omap_dss_device *dssdev)
 		t->pixel_clock = pck;
 	}
 
-	dispc_mgr_set_timings(dssdev->manager->id, t);
+	r = dss_mgr_set_timings(dssdev->manager, t);
+	if (r)
+		return r;
 
 	return 0;
 }
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index b6cf03c..fbca76c 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -4219,13 +4219,20 @@ static int dsi_display_init_dispc(struct omap_dss_device *dssdev)
 		dispc_mgr_enable_stallmode(dssdev->manager->id, true);
 		dispc_mgr_enable_fifohandcheck(dssdev->manager->id, 1);
 
-		dispc_mgr_set_timings(dssdev->manager->id, &timings);
+		r = dss_mgr_set_timings(dssdev->manager, &timings);
+		if (r) {
+			omap_dispc_unregister_isr(dsi_framedone_irq_callback,
+				(void *) dssdev, irq);
+			return r;
+		}
 	} else {
 		dispc_mgr_enable_stallmode(dssdev->manager->id, false);
 		dispc_mgr_enable_fifohandcheck(dssdev->manager->id, 0);
 
-		dispc_mgr_set_timings(dssdev->manager->id,
+		r = dss_mgr_set_timings(dssdev->manager,
 			&dssdev->panel.timings);
+		if (r)
+			return r;
 	}
 
 		dispc_mgr_set_lcd_display_type(dssdev->manager->id,
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 1dc336b..0bff325 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -210,6 +210,8 @@ int dss_mgr_check(struct omap_overlay_manager *mgr,
 		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
 		struct omap_overlay_info **overlay_infos);
+int dss_mgr_set_timings(struct omap_overlay_manager *mgr,
+		struct omap_video_timings *timings);
 
 /* overlay */
 void dss_init_overlays(struct platform_device *pdev);
@@ -442,8 +444,6 @@ void dispc_mgr_enable_stallmode(enum omap_channel channel, bool enable);
 void dispc_mgr_set_tft_data_lines(enum omap_channel channel, u8 data_lines);
 void dispc_mgr_set_lcd_display_type(enum omap_channel channel,
 		enum omap_lcd_display_type type);
-void dispc_mgr_set_timings(enum omap_channel channel,
-		struct omap_video_timings *timings);
 void dispc_mgr_set_pol_freq(enum omap_channel channel,
 		enum omap_panel_config config, u8 acbi, u8 acb);
 unsigned long dispc_mgr_lclk_rate(enum omap_channel channel);
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 56f6e9c..4563781 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -376,7 +376,9 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)
 	dispc_enable_gamma_table(0);
 
 	/* tv size */
-	dispc_mgr_set_timings(dssdev->manager->id, &dssdev->panel.timings);
+	r = dss_mgr_set_timings(dssdev->manager, &dssdev->panel.timings);
+	if (r)
+		goto err_mgr_set_timings;
 
 	hdmi.ip_data.ops->video_enable(&hdmi.ip_data, 1);
 
@@ -387,6 +389,7 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)
 	return 0;
 
 err_mgr_enable:
+err_mgr_set_timings:
 	hdmi.ip_data.ops->video_enable(&hdmi.ip_data, 0);
 	hdmi.ip_data.ops->phy_disable(&hdmi.ip_data);
 	hdmi.ip_data.ops->pll_disable(&hdmi.ip_data);
diff --git a/drivers/video/omap2/dss/manager.c b/drivers/video/omap2/dss/manager.c
index e736460..2c85988 100644
--- a/drivers/video/omap2/dss/manager.c
+++ b/drivers/video/omap2/dss/manager.c
@@ -684,3 +684,24 @@ int dss_mgr_check(struct omap_overlay_manager *mgr,
 
 	return 0;
 }
+
+int dss_mgr_set_timings(struct omap_overlay_manager *mgr,
+		struct omap_video_timings *timings)
+{
+	int r;
+	struct omap_overlay_manager_info info;
+
+	mgr->get_manager_info(mgr, &info);
+
+	info.timings = *timings;
+
+	r = mgr->set_manager_info(mgr, &info);
+	if (r)
+		return r;
+
+	r = mgr->apply(mgr);
+	if (r)
+		return r;
+
+	return 0;
+}
diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
index a81ffcb..588160a 100644
--- a/drivers/video/omap2/dss/rfbi.c
+++ b/drivers/video/omap2/dss/rfbi.c
@@ -320,7 +320,7 @@ static void rfbi_transfer_area(struct omap_dss_device *dssdev, u16 width,
 
 	DSSDBG("rfbi_transfer_area %dx%d\n", width, height);
 
-	dispc_mgr_set_timings(dssdev->manager->id, &timings);
+	dss_mgr_set_timings(dssdev->manager, &timings);
 
 	dispc_mgr_enable(dssdev->manager->id, true);
 
@@ -776,6 +776,7 @@ int omap_rfbi_prepare_update(struct omap_dss_device *dssdev,
 		u16 *x, u16 *y, u16 *w, u16 *h)
 {
 	u16 dw, dh;
+	int r;
 	struct omap_video_timings timings = {
 		.hsw		= 1,
 		.hfp		= 1,
@@ -804,7 +805,9 @@ int omap_rfbi_prepare_update(struct omap_dss_device *dssdev,
 	if (*w = 0 || *h = 0)
 		return -EINVAL;
 
-	dispc_mgr_set_timings(dssdev->manager->id, &timings);
+	r = dss_mgr_set_timings(dssdev->manager, &timings);
+	if (r)
+		return r;
 
 	return 0;
 }
diff --git a/drivers/video/omap2/dss/sdi.c b/drivers/video/omap2/dss/sdi.c
index 741b834..2e70f46 100644
--- a/drivers/video/omap2/dss/sdi.c
+++ b/drivers/video/omap2/dss/sdi.c
@@ -107,7 +107,9 @@ int omapdss_sdi_display_enable(struct omap_dss_device *dssdev)
 	}
 
 
-	dispc_mgr_set_timings(dssdev->manager->id, t);
+	r = dss_mgr_set_timings(dssdev->manager, t);
+	if (r)
+		goto err_set_mgr_timings;
 
 	r = dss_set_clock_div(&dss_cinfo);
 	if (r)
@@ -134,6 +136,7 @@ err_mgr_enable:
 err_sdi_enable:
 err_set_dispc_clock_div:
 err_set_dss_clock_div:
+err_set_mgr_timings:
 err_calc_clock_div:
 	dispc_runtime_put();
 err_get_dispc:
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 30bbb63..073b223 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -444,22 +444,24 @@ static int venc_power_on(struct omap_dss_device *dssdev)
 	timings = dssdev->panel.timings;
 	timings.y_res /= 2;
 
-	dispc_mgr_set_timings(dssdev->manager->id, &timings);
+	r = dss_mgr_set_timings(dssdev->manager, &timings);
+	if (r)
+		goto err0;
 
 	r = regulator_enable(venc.vdda_dac_reg);
 	if (r)
-		goto err;
+		goto err1;
 
 	if (dssdev->platform_enable)
 		dssdev->platform_enable(dssdev);
 
 	r = dss_mgr_enable(dssdev->manager);
 	if (r)
-		goto err;
+		goto err1;
 
 	return 0;
 
-err:
+err1:
 	venc_write_reg(VENC_OUTPUT_CONTROL, 0);
 	dss_set_dac_pwrdn_bgz(0);
 
@@ -467,7 +469,7 @@ err:
 		dssdev->platform_disable(dssdev);
 
 	regulator_disable(venc.vdda_dac_reg);
-
+err0:
 	return r;
 }
 
diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index 5f36ddd..dbc62e8 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -429,6 +429,8 @@ struct omap_overlay_manager_info {
 
 	bool cpr_enable;
 	struct omap_dss_cpr_coefs cpr_coefs;
+
+	struct omap_video_timings timings;
 };
 
 struct omap_overlay_manager {
-- 
1.7.5.4

Re: [PATCH 4/6] OMAPDSS: MANAGER: Make DISPC timings a manager_info parameter

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]

On Mon, 2012-04-16 at 12:53 +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.
> 
> Make timings(omap_video_timing struct) an overlay_manager_info member, they are
> now programmed via the apply mechanism used for programming shadow registers.
> 
> The interface driver now call the function dss_mgr_set_timings() which applies
> the new timing parameters, rather than directly writing to DISPC registers.

I don't think that works correctly. The omap_overlay_manager_info is
supposed to be set with set_manager_info() by the user of omapdss, to
configure the manager's features. The timings are not supposed to be set
via that mechanism, but with dssdev->set_timings().

This is similar to the info and extra_info for overlay. info has stuff
that omapdss doesn't change, it just uses what the user gives.
extra_info, on the other hand, has omapdss private stuff that the user
does not see. Timings are clearly private stuff in this sense, because
they are set via dssdev->set_timings(). 

One reason for this is the programming model we use. If the user of
omapdss does get_info() for two overlays, changes the infos, and then
calls set_info() for both overlays and finally apply() for the manager,
we don't do any locking there because omapdss presumes the info is
handled by one user. If, say, the dpi.c would change the info and call
apply at the same time, the configuration could go badly wrong.

So I think what should be done is to add similar "extra" flags and code
to mgr_priv_data that we have for ovl_priv_data, and add
omap_video_timings to mgr_priv_data (the same way as we have, say,
fifo_low for ovl_priv_data).

And then add similar function to dss_ovl_write_regs_extra() for manager,
and a function like dss_apply_ovl_fifo_thresholds() for timings. And
finally a non-static function to set the timings (used by dpi.c etc),
which calls the similar function to dss_apply_ovl_fifo_thresholds(), and
dss_write_regs() and dss_set_go_bits().

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/6] OMAPDSS: MANAGER: Make DISPC timings a manager_info parameter

[Archit Taneja]

On Wednesday 18 April 2012 08:28 PM, Tomi Valkeinen wrote:
> On Mon, 2012-04-16 at 12:53 +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.
>>
>> Make timings(omap_video_timing struct) an overlay_manager_info member, they are
>> now programmed via the apply mechanism used for programming shadow registers.
>>
>> The interface driver now call the function dss_mgr_set_timings() which applies
>> the new timing parameters, rather than directly writing to DISPC registers.
>
> I don't think that works correctly. The omap_overlay_manager_info is
> supposed to be set with set_manager_info() by the user of omapdss, to
> configure the manager's features. The timings are not supposed to be set
> via that mechanism, but with dssdev->set_timings().
>
> This is similar to the info and extra_info for overlay. info has stuff
> that omapdss doesn't change, it just uses what the user gives.
> extra_info, on the other hand, has omapdss private stuff that the user
> does not see. Timings are clearly private stuff in this sense, because
> they are set via dssdev->set_timings().
>
> One reason for this is the programming model we use. If the user of
> omapdss does get_info() for two overlays, changes the infos, and then
> calls set_info() for both overlays and finally apply() for the manager,
> we don't do any locking there because omapdss presumes the info is
> handled by one user. If, say, the dpi.c would change the info and call
> apply at the same time, the configuration could go badly wrong.

I think I get your point. So even though get_info() and set_info() fn's 
are spinlock protected, if there are 2 users setting the info, it 
doesn't mean that the info they finally written is correct. Is this 
example the same thing as what you mean ? :

In order of time:

-user 1 gets an overlay's info

-user 2 gets an overlay's info

-user 1 modifies and sets overlay info

-user 2 sets overlay info without the knowledge of what user 1 did.

So even though we ensure these events happen sequentially, we don't 
protect the info across multiple users.

>
> So I think what should be done is to add similar "extra" flags and code
> to mgr_priv_data that we have for ovl_priv_data, and add
> omap_video_timings to mgr_priv_data (the same way as we have, say,
> fifo_low for ovl_priv_data).
>
> And then add similar function to dss_ovl_write_regs_extra() for manager,
> and a function like dss_apply_ovl_fifo_thresholds() for timings. And
> finally a non-static function to set the timings (used by dpi.c etc),
> which calls the similar function to dss_apply_ovl_fifo_thresholds(), and
> dss_write_regs() and dss_set_go_bits().

Okay, I'll work on it along these lines.

>
>   Tomi
>

Re: [PATCH 4/6] OMAPDSS: MANAGER: Make DISPC timings a manager_info parameter

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3139 bytes --]

On Thu, 2012-04-19 at 11:43 +0530, Archit Taneja wrote:
> On Wednesday 18 April 2012 08:28 PM, Tomi Valkeinen wrote:
> > On Mon, 2012-04-16 at 12:53 +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.
> >>
> >> Make timings(omap_video_timing struct) an overlay_manager_info member, they are
> >> now programmed via the apply mechanism used for programming shadow registers.
> >>
> >> The interface driver now call the function dss_mgr_set_timings() which applies
> >> the new timing parameters, rather than directly writing to DISPC registers.
> >
> > I don't think that works correctly. The omap_overlay_manager_info is
> > supposed to be set with set_manager_info() by the user of omapdss, to
> > configure the manager's features. The timings are not supposed to be set
> > via that mechanism, but with dssdev->set_timings().
> >
> > This is similar to the info and extra_info for overlay. info has stuff
> > that omapdss doesn't change, it just uses what the user gives.
> > extra_info, on the other hand, has omapdss private stuff that the user
> > does not see. Timings are clearly private stuff in this sense, because
> > they are set via dssdev->set_timings().
> >
> > One reason for this is the programming model we use. If the user of
> > omapdss does get_info() for two overlays, changes the infos, and then
> > calls set_info() for both overlays and finally apply() for the manager,
> > we don't do any locking there because omapdss presumes the info is
> > handled by one user. If, say, the dpi.c would change the info and call
> > apply at the same time, the configuration could go badly wrong.
> 
> I think I get your point. So even though get_info() and set_info() fn's 
> are spinlock protected, if there are 2 users setting the info, it 
> doesn't mean that the info they finally written is correct. Is this 
> example the same thing as what you mean ? :
> 
> In order of time:
> 
> -user 1 gets an overlay's info
> 
> -user 2 gets an overlay's info
> 
> -user 1 modifies and sets overlay info
> 
> -user 2 sets overlay info without the knowledge of what user 1 did.
> 
> So even though we ensure these events happen sequentially, we don't 
> protect the info across multiple users.

Yes. The spinlocks ensure that the info is "whole", so we don't get a
few fields from user1 and a few fields from user2. But they don't
protect us from the case you described above.

For that we would need a "dss lock" that the user would acquire before
using get_info and set_info. But I don't want to go to that direction,
because we really only support one user anyway.

The problem in this particular case is that omapdss itself becomes
another user if it uses get_info & set_info. And that can be easily
avoided by splitting the configuration into public (the "info") and
internal ("extra_info"). The users of omapdss never touch the
extra_info, and omapdss never touches the info.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/6] OMAPDSS: MANAGER: Make DISPC timings a manager_info parameter

[Archit Taneja]

On Thursday 19 April 2012 12:07 PM, Tomi Valkeinen wrote:
> On Thu, 2012-04-19 at 11:43 +0530, Archit Taneja wrote:
>> On Wednesday 18 April 2012 08:28 PM, Tomi Valkeinen wrote:
>>> On Mon, 2012-04-16 at 12:53 +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.
>>>>
>>>> Make timings(omap_video_timing struct) an overlay_manager_info member, they are
>>>> now programmed via the apply mechanism used for programming shadow registers.
>>>>
>>>> The interface driver now call the function dss_mgr_set_timings() which applies
>>>> the new timing parameters, rather than directly writing to DISPC registers.
>>>
>>> I don't think that works correctly. The omap_overlay_manager_info is
>>> supposed to be set with set_manager_info() by the user of omapdss, to
>>> configure the manager's features. The timings are not supposed to be set
>>> via that mechanism, but with dssdev->set_timings().
>>>
>>> This is similar to the info and extra_info for overlay. info has stuff
>>> that omapdss doesn't change, it just uses what the user gives.
>>> extra_info, on the other hand, has omapdss private stuff that the user
>>> does not see. Timings are clearly private stuff in this sense, because
>>> they are set via dssdev->set_timings().
>>>
>>> One reason for this is the programming model we use. If the user of
>>> omapdss does get_info() for two overlays, changes the infos, and then
>>> calls set_info() for both overlays and finally apply() for the manager,
>>> we don't do any locking there because omapdss presumes the info is
>>> handled by one user. If, say, the dpi.c would change the info and call
>>> apply at the same time, the configuration could go badly wrong.
>>
>> I think I get your point. So even though get_info() and set_info() fn's
>> are spinlock protected, if there are 2 users setting the info, it
>> doesn't mean that the info they finally written is correct. Is this
>> example the same thing as what you mean ? :
>>
>> In order of time:
>>
>> -user 1 gets an overlay's info
>>
>> -user 2 gets an overlay's info
>>
>> -user 1 modifies and sets overlay info
>>
>> -user 2 sets overlay info without the knowledge of what user 1 did.
>>
>> So even though we ensure these events happen sequentially, we don't
>> protect the info across multiple users.
>
> Yes. The spinlocks ensure that the info is "whole", so we don't get a
> few fields from user1 and a few fields from user2. But they don't
> protect us from the case you described above.
>
> For that we would need a "dss lock" that the user would acquire before
> using get_info and set_info. But I don't want to go to that direction,
> because we really only support one user anyway.
>
> The problem in this particular case is that omapdss itself becomes
> another user if it uses get_info&  set_info. And that can be easily
> avoided by splitting the configuration into public (the "info") and
> internal ("extra_info"). The users of omapdss never touch the
> extra_info, and omapdss never touches the info.

omapdss touches info via sysfs, so if we use sysfs(in a fast way, using 
a it in scripts, for example) while fb uses DSS2, then we might hit this 
issue.

Archit

>
>   Tomi
>

Re: [PATCH 4/6] OMAPDSS: MANAGER: Make DISPC timings a manager_info parameter

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3861 bytes --]

On Thu, 2012-04-19 at 15:38 +0530, Archit Taneja wrote:
> On Thursday 19 April 2012 12:07 PM, Tomi Valkeinen wrote:
> > On Thu, 2012-04-19 at 11:43 +0530, Archit Taneja wrote:
> >> On Wednesday 18 April 2012 08:28 PM, Tomi Valkeinen wrote:
> >>> On Mon, 2012-04-16 at 12:53 +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.
> >>>>
> >>>> Make timings(omap_video_timing struct) an overlay_manager_info member, they are
> >>>> now programmed via the apply mechanism used for programming shadow registers.
> >>>>
> >>>> The interface driver now call the function dss_mgr_set_timings() which applies
> >>>> the new timing parameters, rather than directly writing to DISPC registers.
> >>>
> >>> I don't think that works correctly. The omap_overlay_manager_info is
> >>> supposed to be set with set_manager_info() by the user of omapdss, to
> >>> configure the manager's features. The timings are not supposed to be set
> >>> via that mechanism, but with dssdev->set_timings().
> >>>
> >>> This is similar to the info and extra_info for overlay. info has stuff
> >>> that omapdss doesn't change, it just uses what the user gives.
> >>> extra_info, on the other hand, has omapdss private stuff that the user
> >>> does not see. Timings are clearly private stuff in this sense, because
> >>> they are set via dssdev->set_timings().
> >>>
> >>> One reason for this is the programming model we use. If the user of
> >>> omapdss does get_info() for two overlays, changes the infos, and then
> >>> calls set_info() for both overlays and finally apply() for the manager,
> >>> we don't do any locking there because omapdss presumes the info is
> >>> handled by one user. If, say, the dpi.c would change the info and call
> >>> apply at the same time, the configuration could go badly wrong.
> >>
> >> I think I get your point. So even though get_info() and set_info() fn's
> >> are spinlock protected, if there are 2 users setting the info, it
> >> doesn't mean that the info they finally written is correct. Is this
> >> example the same thing as what you mean ? :
> >>
> >> In order of time:
> >>
> >> -user 1 gets an overlay's info
> >>
> >> -user 2 gets an overlay's info
> >>
> >> -user 1 modifies and sets overlay info
> >>
> >> -user 2 sets overlay info without the knowledge of what user 1 did.
> >>
> >> So even though we ensure these events happen sequentially, we don't
> >> protect the info across multiple users.
> >
> > Yes. The spinlocks ensure that the info is "whole", so we don't get a
> > few fields from user1 and a few fields from user2. But they don't
> > protect us from the case you described above.
> >
> > For that we would need a "dss lock" that the user would acquire before
> > using get_info and set_info. But I don't want to go to that direction,
> > because we really only support one user anyway.
> >
> > The problem in this particular case is that omapdss itself becomes
> > another user if it uses get_info&  set_info. And that can be easily
> > avoided by splitting the configuration into public (the "info") and
> > internal ("extra_info"). The users of omapdss never touch the
> > extra_info, and omapdss never touches the info.
> 
> omapdss touches info via sysfs, so if we use sysfs(in a fast way, using 
> a it in scripts, for example) while fb uses DSS2, then we might hit this 
> issue.

The sysfs calls come from the user, so in that sense it's not omapdss
that is doing the change.

But it's true that with sysfs we may have two users for omapdss, for
example the scripts using sysfs and omapfb. Hopefully we can deprecate
the sysfs files at some point...

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 5/6] OMAPDSS: MANAGER: Check validity of manager timings

[Archit Taneja]

The manager timings is a member of the overlay_manager_info struct. We can now
check it's validity in dss_mgr_check(). Add a function dss_mgr_check_timings()
which checks if the timings are correct.

Currently, the manager timings are initialized in an interface's enable call.
Since a mgr_apply() can happen before the connected display is enabled, we
initialize the manager timings to a dummy valid value in apply_init().

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/apply.c   |   22 ++++++++++++++++++++++
 drivers/video/omap2/dss/dpi.c     |    2 +-
 drivers/video/omap2/dss/dss.h     |    2 ++
 drivers/video/omap2/dss/manager.c |   15 +++++++++++++++
 4 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index b10b3bc..53a198f 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -132,10 +132,32 @@ static struct mgr_priv_data *get_mgr_priv(struct omap_overlay_manager *mgr)
 void dss_apply_init(void)
 {
 	const int num_ovls = dss_feat_get_num_ovls();
+	const int num_mgrs = dss_feat_get_num_ovls();
 	int i;
+	/* Use dummy manager timings during initialization */
+	struct omap_video_timings timings = {
+		.hsw		= 1,
+		.hfp		= 1,
+		.hbp		= 1,
+		.vsw		= 1,
+		.vfp		= 0,
+		.vbp		= 0,
+		.x_res	= dss_feat_get_param_max(FEAT_PARAM_MGR_WIDTH),
+		.y_res	= dss_feat_get_param_max(FEAT_PARAM_MGR_HEIGHT),
+	};
 
 	spin_lock_init(&data_lock);
 
+	for (i = 0; i < num_mgrs; i++) {
+		struct mgr_priv_data *mp;
+
+		mp = &dss_data.mgr_priv_data_array[i];
+
+		mp->info.timings = timings;
+
+		mp->user_info = mp->info;
+	}
+
 	for (i = 0; i < num_ovls; ++i) {
 		struct ovl_priv_data *op;
 
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index 76e2cae..a1bdbf7 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -314,7 +314,7 @@ int dpi_check_timings(struct omap_dss_device *dssdev,
 	unsigned long pck;
 	struct dispc_clock_info dispc_cinfo;
 
-	if (!dispc_mgr_timings_ok(dssdev->manager->id, timings))
+	if (!dss_mgr_check_timings(dssdev->manager, timings))
 		return -EINVAL;
 
 	if (timings->pixel_clock = 0)
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 0bff325..9e47bdf 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -206,6 +206,8 @@ int dss_init_overlay_managers(struct platform_device *pdev);
 void dss_uninit_overlay_managers(struct platform_device *pdev);
 int dss_mgr_simple_check(struct omap_overlay_manager *mgr,
 		const struct omap_overlay_manager_info *info);
+int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
+		struct omap_video_timings *timings);
 int dss_mgr_check(struct omap_overlay_manager *mgr,
 		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
diff --git a/drivers/video/omap2/dss/manager.c b/drivers/video/omap2/dss/manager.c
index 2c85988..fb61b76 100644
--- a/drivers/video/omap2/dss/manager.c
+++ b/drivers/video/omap2/dss/manager.c
@@ -654,6 +654,17 @@ static int dss_mgr_check_zorder(struct omap_overlay_manager *mgr,
 	return 0;
 }
 
+int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
+		struct omap_video_timings *timings)
+{
+	if (!dispc_mgr_timings_ok(mgr->id, timings)) {
+		DSSERR("check_manager: invalid timings\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int dss_mgr_check(struct omap_overlay_manager *mgr,
 		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
@@ -668,6 +679,10 @@ int dss_mgr_check(struct omap_overlay_manager *mgr,
 			return r;
 	}
 
+	r = dss_mgr_check_timings(mgr, &info->timings);
+	if (r)
+		return r;
+
 	list_for_each_entry(ovl, &mgr->overlays, list) {
 		struct omap_overlay_info *oi;
 		int r;
-- 
1.7.5.4

[PATCH 6/6] OMAPDSS: APPLY: Remove display dependency from overlay and manager checks

[Archit Taneja]

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. manager_info now
contains the manager size(which should be the panel resolution in most cases).
Hence, we don't need to depend on the display resolution.

manager_info is now passed to dss_ovl_check() to see if the overlay fits within
the manager area.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/apply.c   |   24 +++++++++++-------------
 drivers/video/omap2/dss/dss.h     |    5 ++---
 drivers/video/omap2/dss/manager.c |    3 +--
 drivers/video/omap2/dss/overlay.c |   20 +++++++++-----------
 4 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 53a198f..5073009 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -198,7 +198,7 @@ static bool mgr_manual_update(struct omap_overlay_manager *mgr)
 }
 
 static int dss_check_settings_low(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev, bool applying)
+		bool applying)
 {
 	struct omap_overlay_info *oi;
 	struct omap_overlay_manager_info *mi;
@@ -228,26 +228,24 @@ static int dss_check_settings_low(struct omap_overlay_manager *mgr,
 		ois[ovl->id] = oi;
 	}
 
-	return dss_mgr_check(mgr, dssdev, mi, ois);
+	return dss_mgr_check(mgr, mi, ois);
 }
 
 /*
  * check manager and overlay settings using overlay_info from data->info
  */
-static int dss_check_settings(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev)
+static int dss_check_settings(struct omap_overlay_manager *mgr)
 {
-	return dss_check_settings_low(mgr, dssdev, false);
+	return dss_check_settings_low(mgr, false);
 }
 
 /*
  * check manager and overlay settings using overlay_info from ovl->info if
  * dirty and from data->info otherwise
  */
-static int dss_check_settings_apply(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev)
+static int dss_check_settings_apply(struct omap_overlay_manager *mgr)
 {
-	return dss_check_settings_low(mgr, dssdev, true);
+	return dss_check_settings_low(mgr, true);
 }
 
 static bool need_isr(void)
@@ -668,7 +666,7 @@ static void dss_write_regs(void)
 		if (!mp->enabled || mgr_manual_update(mgr) || mp->busy)
 			continue;
 
-		r = dss_check_settings(mgr, mgr->device);
+		r = dss_check_settings(mgr);
 		if (r) {
 			DSSERR("cannot write registers for manager %s: "
 					"illegal configuration\n", mgr->name);
@@ -733,7 +731,7 @@ void dss_mgr_start_update(struct omap_overlay_manager *mgr)
 
 	WARN_ON(mp->updating);
 
-	r = dss_check_settings(mgr, mgr->device);
+	r = dss_check_settings(mgr);
 	if (r) {
 		DSSERR("cannot start manual update: illegal configuration\n");
 		spin_unlock_irqrestore(&data_lock, flags);
@@ -879,7 +877,7 @@ int omap_dss_mgr_apply(struct omap_overlay_manager *mgr)
 
 	spin_lock_irqsave(&data_lock, flags);
 
-	r = dss_check_settings_apply(mgr, mgr->device);
+	r = dss_check_settings_apply(mgr);
 	if (r) {
 		spin_unlock_irqrestore(&data_lock, flags);
 		DSSERR("failed to apply settings: illegal configuration.\n");
@@ -1072,7 +1070,7 @@ int dss_mgr_enable(struct omap_overlay_manager *mgr)
 
 	mp->enabled = true;
 
-	r = dss_check_settings(mgr, mgr->device);
+	r = dss_check_settings(mgr);
 	if (r) {
 		DSSERR("failed to enable manager %d: check_settings failed\n",
 				mgr->id);
@@ -1415,7 +1413,7 @@ int dss_ovl_enable(struct omap_overlay *ovl)
 
 	op->enabling = true;
 
-	r = dss_check_settings(ovl->manager, ovl->manager->device);
+	r = dss_check_settings(ovl->manager);
 	if (r) {
 		DSSERR("failed to enable overlay %d: check_settings failed\n",
 				ovl->id);
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 9e47bdf..fb5ca51 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -209,7 +209,6 @@ int dss_mgr_simple_check(struct omap_overlay_manager *mgr,
 int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
 		struct omap_video_timings *timings);
 int dss_mgr_check(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
 		struct omap_overlay_info **overlay_infos);
 int dss_mgr_set_timings(struct omap_overlay_manager *mgr,
@@ -222,8 +221,8 @@ void dss_overlay_setup_dispc_manager(struct omap_overlay_manager *mgr);
 void dss_recheck_connections(struct omap_dss_device *dssdev, bool force);
 int dss_ovl_simple_check(struct omap_overlay *ovl,
 		const struct omap_overlay_info *info);
-int dss_ovl_check(struct omap_overlay *ovl,
-		struct omap_overlay_info *info, struct omap_dss_device *dssdev);
+int dss_ovl_check(struct omap_overlay *ovl, struct omap_overlay_info *info,
+		struct omap_overlay_manager_info *mgr_info);
 
 /* DSS */
 int dss_init_platform_driver(void);
diff --git a/drivers/video/omap2/dss/manager.c b/drivers/video/omap2/dss/manager.c
index fb61b76..9821015 100644
--- a/drivers/video/omap2/dss/manager.c
+++ b/drivers/video/omap2/dss/manager.c
@@ -666,7 +666,6 @@ int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
 }
 
 int dss_mgr_check(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
 		struct omap_overlay_info **overlay_infos)
 {
@@ -692,7 +691,7 @@ int dss_mgr_check(struct omap_overlay_manager *mgr,
 		if (oi = NULL)
 			continue;
 
-		r = dss_ovl_check(ovl, oi, dssdev);
+		r = dss_ovl_check(ovl, oi, info);
 		if (r)
 			return r;
 	}
diff --git a/drivers/video/omap2/dss/overlay.c b/drivers/video/omap2/dss/overlay.c
index 6e82181..4436376 100644
--- a/drivers/video/omap2/dss/overlay.c
+++ b/drivers/video/omap2/dss/overlay.c
@@ -631,16 +631,14 @@ int dss_ovl_simple_check(struct omap_overlay *ovl,
 	return 0;
 }
 
-int dss_ovl_check(struct omap_overlay *ovl,
-		struct omap_overlay_info *info, struct omap_dss_device *dssdev)
+int dss_ovl_check(struct omap_overlay *ovl, struct omap_overlay_info *info,
+		struct omap_overlay_manager_info *mgr_info)
 {
 	u16 outw, outh;
-	u16 dw, dh;
+	u16 mgr_width, mgr_height;
 
-	if (dssdev = NULL)
-		return 0;
-
-	dssdev->driver->get_resolution(dssdev, &dw, &dh);
+	mgr_width = mgr_info->timings.x_res;
+	mgr_height = mgr_info->timings.y_res;
 
 	if ((ovl->caps & OMAP_DSS_OVL_CAP_SCALE) = 0) {
 		outw = info->width;
@@ -657,17 +655,17 @@ int dss_ovl_check(struct omap_overlay *ovl,
 			outh = info->out_height;
 	}
 
-	if (dw < info->pos_x + outw) {
+	if (mgr_width < info->pos_x + outw) {
 		DSSERR("overlay %d horizontally not inside the display area "
 				"(%d + %d >= %d)\n",
-				ovl->id, info->pos_x, outw, dw);
+				ovl->id, info->pos_x, outw, mgr_width);
 		return -EINVAL;
 	}
 
-	if (dh < info->pos_y + outh) {
+	if (mgr_height < info->pos_y + outh) {
 		DSSERR("overlay %d vertically not inside the display area "
 				"(%d + %d >= %d)\n",
-				ovl->id, info->pos_y, outh, dh);
+				ovl->id, info->pos_y, outh, mgr_height);
 		return -EINVAL;
 	}
 
-- 
1.7.5.4

Re: [PATCH 0/6] OMAPDSS: APPLY: Treat overlay manager timings as shadow registers

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]

On Mon, 2012-04-16 at 12:53 +0530, Archit Taneja wrote:
> An overlay manager's timings (the manager size, and blanking parameters if an
> LCD manager) are DISPC shadow registers, and they should hence follow the
> correct programming model.
> 
> This set makes the timings a manager_info parameter. The interface drivers now
> set the timings in manager_info instead of directly writing to registers.
> 
> This change also prevents the need to use display resolution for overlay
> checks, hence making some of the APPLY functions less dependent on the display.
> 
> These patches apply over:
> 
> git://gitorious.org/linux-omap-dss2/linux.git dev
> 
> Archit Taneja (6):
>   OMAPDSS: DISPC/RFBI: Use dispc_mgr_set_lcd_timings() for setting lcd
>     size
>   OMAPDSS: DISPC: Use a common function to set manager timings
>   OMAPDSS: DISPC: Clean up manager timing/size functions
>   OMAPDSS: MANAGER: Make DISPC timings a manager_info parameter
>   OMAPDSS: MANAGER: Check validity of manager timings
>   OMAPDSS: APPLY: Remove display dependency from overlay and manager
>     checks

The patches 1, 2 and 3 in this series look like independent cleanups.
Should I apply those, or do you think there may be changes required?

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/6] OMAPDSS: APPLY: Treat overlay manager timings as shadow registers

[Archit Taneja]

On Thursday 19 April 2012 05:18 PM, Tomi Valkeinen wrote:
> On Mon, 2012-04-16 at 12:53 +0530, Archit Taneja wrote:
>> An overlay manager's timings (the manager size, and blanking parameters if an
>> LCD manager) are DISPC shadow registers, and they should hence follow the
>> correct programming model.
>>
>> This set makes the timings a manager_info parameter. The interface drivers now
>> set the timings in manager_info instead of directly writing to registers.
>>
>> This change also prevents the need to use display resolution for overlay
>> checks, hence making some of the APPLY functions less dependent on the display.
>>
>> These patches apply over:
>>
>> git://gitorious.org/linux-omap-dss2/linux.git dev
>>
>> Archit Taneja (6):
>>    OMAPDSS: DISPC/RFBI: Use dispc_mgr_set_lcd_timings() for setting lcd
>>      size
>>    OMAPDSS: DISPC: Use a common function to set manager timings
>>    OMAPDSS: DISPC: Clean up manager timing/size functions
>>    OMAPDSS: MANAGER: Make DISPC timings a manager_info parameter
>>    OMAPDSS: MANAGER: Check validity of manager timings
>>    OMAPDSS: APPLY: Remove display dependency from overlay and manager
>>      checks
>
> The patches 1, 2 and 3 in this series look like independent cleanups.
> Should I apply those, or do you think there may be changes required?

I think you can pull these 3, they were sort of a prerequisite for the 
apply-manager-timings stuff. I don't think they need to change.

I'll post another series for applying the manager timings correctly(i.e, 
the fixed version of the last 3 patches).

Archit

>
>   Tomi
>

Re: [PATCH 0/6] OMAPDSS: APPLY: Treat overlay manager timings as shadow registers

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1799 bytes --]

On Thu, 2012-04-19 at 17:28 +0530, Archit Taneja wrote:
> On Thursday 19 April 2012 05:18 PM, Tomi Valkeinen wrote:
> > On Mon, 2012-04-16 at 12:53 +0530, Archit Taneja wrote:
> >> An overlay manager's timings (the manager size, and blanking parameters if an
> >> LCD manager) are DISPC shadow registers, and they should hence follow the
> >> correct programming model.
> >>
> >> This set makes the timings a manager_info parameter. The interface drivers now
> >> set the timings in manager_info instead of directly writing to registers.
> >>
> >> This change also prevents the need to use display resolution for overlay
> >> checks, hence making some of the APPLY functions less dependent on the display.
> >>
> >> These patches apply over:
> >>
> >> git://gitorious.org/linux-omap-dss2/linux.git dev
> >>
> >> Archit Taneja (6):
> >>    OMAPDSS: DISPC/RFBI: Use dispc_mgr_set_lcd_timings() for setting lcd
> >>      size
> >>    OMAPDSS: DISPC: Use a common function to set manager timings
> >>    OMAPDSS: DISPC: Clean up manager timing/size functions
> >>    OMAPDSS: MANAGER: Make DISPC timings a manager_info parameter
> >>    OMAPDSS: MANAGER: Check validity of manager timings
> >>    OMAPDSS: APPLY: Remove display dependency from overlay and manager
> >>      checks
> >
> > The patches 1, 2 and 3 in this series look like independent cleanups.
> > Should I apply those, or do you think there may be changes required?
> 
> I think you can pull these 3, they were sort of a prerequisite for the 
> apply-manager-timings stuff. I don't think they need to change.
> 
> I'll post another series for applying the manager timings correctly(i.e, 
> the fixed version of the last 3 patches).

Ok, I applied the first three patches to my dev branch.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2 0/4] OMAPDSS: APPLY: Treat overlay manager timings as shadow registers

[Archit Taneja]

An overlay manager's timings (the manager size, and blanking parameters if an
LCD manager) are DISPC shadow registers, and they should hence follow the
correct programming model.

This set makes the timings an extra_info parameter in manager's private data .
The interface drivers now apply the timings in instead of directly writing to
registers.

This change also prevents the need to use display resolution for overlay
checks, hence making some of the APPLY functions less dependent on the display.

Changes since v2:

- Manager timings are represented as extra_info.
- The DISPC related cleanups are removed since they have already been taken.

These patches apply over:

git://gitorious.org/linux-omap-dss2/linux.git dev

Archit Taneja (4):
  OMAPDSS: APPLY: Add manager timings as extra_info in private data
  OMAPDSS: Apply manager timings instead of direct DISPC writes
  OMAPDSS: MANAGER: Create a function to check manager timings
  OMAPDSS: APPLY: Remove display dependency from overlay and manager
    checks

 drivers/video/omap2/dss/apply.c   |  142 ++++++++++++++++++++++++++++++-------
 drivers/video/omap2/dss/dpi.c     |    4 +-
 drivers/video/omap2/dss/dsi.c     |    5 +-
 drivers/video/omap2/dss/dss.h     |    9 ++-
 drivers/video/omap2/dss/hdmi.c    |    2 +-
 drivers/video/omap2/dss/manager.c |   19 +++++-
 drivers/video/omap2/dss/overlay.c |   20 +++---
 drivers/video/omap2/dss/rfbi.c    |    4 +-
 drivers/video/omap2/dss/sdi.c     |    2 +-
 drivers/video/omap2/dss/venc.c    |    2 +-
 10 files changed, 158 insertions(+), 51 deletions(-)

-- 
1.7.5.4

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

[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 <archit@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)
 		}
 
 		dss_mgr_write_regs(mgr);
+		dss_mgr_write_regs_extra(mgr);
 	}
 }
 
@@ -693,6 +732,7 @@ static void mgr_clear_shadow_dirty(struct omap_overlay_manager *mgr)
 
 	mp = get_mgr_priv(mgr);
 	mp->shadow_info_dirty = false;
+	mp->shadow_extra_info_dirty = false;
 
 	list_for_each_entry(ovl, &mgr->overlays, list) {
 		op = get_ovl_priv(ovl);
@@ -719,6 +759,7 @@ void dss_mgr_start_update(struct omap_overlay_manager *mgr)
 	}
 
 	dss_mgr_write_regs(mgr);
+	dss_mgr_write_regs_extra(mgr);
 
 	dss_write_regs_common();
 
@@ -1225,6 +1266,30 @@ err:
 	return r;
 }
 
+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);
+}
 
 int dss_ovl_set_info(struct omap_overlay *ovl,
 		struct omap_overlay_info *info)
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 5ca67f1..ca59481 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -177,6 +177,8 @@ void dss_mgr_get_info(struct omap_overlay_manager *mgr,
 int dss_mgr_set_device(struct omap_overlay_manager *mgr,
 		struct omap_dss_device *dssdev);
 int dss_mgr_unset_device(struct omap_overlay_manager *mgr);
+void dss_mgr_set_timings(struct omap_overlay_manager *mgr,
+		struct omap_video_timings *timings);
 
 bool dss_ovl_is_enabled(struct omap_overlay *ovl);
 int dss_ovl_enable(struct omap_overlay *ovl);
-- 
1.7.5.4

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2723 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[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 Taneja<archit@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
>

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3059 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2 2/4] OMAPDSS: Apply manager timings instead of direct DISPC writes

[Archit Taneja]

Replace the function dispc_mgr_set_timings() with dss_mgr_set_timings() in the
interface drivers. The latter function ensures that the timing related DISPC
registers are configured according to the shadow register programming model.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dpi.c  |    2 +-
 drivers/video/omap2/dss/dsi.c  |    5 ++---
 drivers/video/omap2/dss/hdmi.c |    2 +-
 drivers/video/omap2/dss/rfbi.c |    4 ++--
 drivers/video/omap2/dss/sdi.c  |    2 +-
 drivers/video/omap2/dss/venc.c |    2 +-
 6 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index cec1166..5d84ab0 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -156,7 +156,7 @@ static int dpi_set_mode(struct omap_dss_device *dssdev)
 		t->pixel_clock = pck;
 	}
 
-	dispc_mgr_set_timings(dssdev->manager->id, t);
+	dss_mgr_set_timings(dssdev->manager, t);
 
 	return 0;
 }
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index b6cf03c..db73598 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -4219,13 +4219,12 @@ static int dsi_display_init_dispc(struct omap_dss_device *dssdev)
 		dispc_mgr_enable_stallmode(dssdev->manager->id, true);
 		dispc_mgr_enable_fifohandcheck(dssdev->manager->id, 1);
 
-		dispc_mgr_set_timings(dssdev->manager->id, &timings);
+		dss_mgr_set_timings(dssdev->manager, &timings);
 	} else {
 		dispc_mgr_enable_stallmode(dssdev->manager->id, false);
 		dispc_mgr_enable_fifohandcheck(dssdev->manager->id, 0);
 
-		dispc_mgr_set_timings(dssdev->manager->id,
-			&dssdev->panel.timings);
+		dss_mgr_set_timings(dssdev->manager, &dssdev->panel.timings);
 	}
 
 		dispc_mgr_set_lcd_display_type(dssdev->manager->id,
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 56f6e9c..8d4ff8c 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -376,7 +376,7 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)
 	dispc_enable_gamma_table(0);
 
 	/* tv size */
-	dispc_mgr_set_timings(dssdev->manager->id, &dssdev->panel.timings);
+	dss_mgr_set_timings(dssdev->manager, &dssdev->panel.timings);
 
 	hdmi.ip_data.ops->video_enable(&hdmi.ip_data, 1);
 
diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
index a81ffcb..feadfab 100644
--- a/drivers/video/omap2/dss/rfbi.c
+++ b/drivers/video/omap2/dss/rfbi.c
@@ -320,7 +320,7 @@ static void rfbi_transfer_area(struct omap_dss_device *dssdev, u16 width,
 
 	DSSDBG("rfbi_transfer_area %dx%d\n", width, height);
 
-	dispc_mgr_set_timings(dssdev->manager->id, &timings);
+	dss_mgr_set_timings(dssdev->manager, &timings);
 
 	dispc_mgr_enable(dssdev->manager->id, true);
 
@@ -804,7 +804,7 @@ int omap_rfbi_prepare_update(struct omap_dss_device *dssdev,
 	if (*w = 0 || *h = 0)
 		return -EINVAL;
 
-	dispc_mgr_set_timings(dssdev->manager->id, &timings);
+	dss_mgr_set_timings(dssdev->manager, &timings);
 
 	return 0;
 }
diff --git a/drivers/video/omap2/dss/sdi.c b/drivers/video/omap2/dss/sdi.c
index 741b834..67fbe7c 100644
--- a/drivers/video/omap2/dss/sdi.c
+++ b/drivers/video/omap2/dss/sdi.c
@@ -107,7 +107,7 @@ int omapdss_sdi_display_enable(struct omap_dss_device *dssdev)
 	}
 
 
-	dispc_mgr_set_timings(dssdev->manager->id, t);
+	dss_mgr_set_timings(dssdev->manager, t);
 
 	r = dss_set_clock_div(&dss_cinfo);
 	if (r)
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 30bbb63..e237464 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -444,7 +444,7 @@ static int venc_power_on(struct omap_dss_device *dssdev)
 	timings = dssdev->panel.timings;
 	timings.y_res /= 2;
 
-	dispc_mgr_set_timings(dssdev->manager->id, &timings);
+	dss_mgr_set_timings(dssdev->manager, &timings);
 
 	r = regulator_enable(venc.vdda_dac_reg);
 	if (r)
-- 
1.7.5.4

[PATCH v2 3/4] OMAPDSS: MANAGER: Create a function to check manager timings

[Archit Taneja]

Create a function dss_mgr_check_timings() which wraps around the function
dispc_mgr_timings_ok(). This is mainly a clean up to hide dispc functions
from interface drivers.

dss_mgr_check_timings() is added in the function dss_mgr_check(), it currently
takes the timings maintained in the omap_dss_device struct. This would be later
replaced by the timings stored in the manager's private data.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dpi.c     |    2 +-
 drivers/video/omap2/dss/dss.h     |    2 ++
 drivers/video/omap2/dss/manager.c |   15 +++++++++++++++
 3 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index 5d84ab0..8127f46 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -312,7 +312,7 @@ int dpi_check_timings(struct omap_dss_device *dssdev,
 	unsigned long pck;
 	struct dispc_clock_info dispc_cinfo;
 
-	if (!dispc_mgr_timings_ok(dssdev->manager->id, timings))
+	if (!dss_mgr_check_timings(dssdev->manager, timings))
 		return -EINVAL;
 
 	if (timings->pixel_clock = 0)
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index ca59481..a148712 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -208,6 +208,8 @@ int dss_init_overlay_managers(struct platform_device *pdev);
 void dss_uninit_overlay_managers(struct platform_device *pdev);
 int dss_mgr_simple_check(struct omap_overlay_manager *mgr,
 		const struct omap_overlay_manager_info *info);
+int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
+		struct omap_video_timings *timings);
 int dss_mgr_check(struct omap_overlay_manager *mgr,
 		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
diff --git a/drivers/video/omap2/dss/manager.c b/drivers/video/omap2/dss/manager.c
index e736460..264ad7c 100644
--- a/drivers/video/omap2/dss/manager.c
+++ b/drivers/video/omap2/dss/manager.c
@@ -654,6 +654,17 @@ static int dss_mgr_check_zorder(struct omap_overlay_manager *mgr,
 	return 0;
 }
 
+int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
+		struct omap_video_timings *timings)
+{
+	if (!dispc_mgr_timings_ok(mgr->id, timings)) {
+		DSSERR("check_manager: invalid timings\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int dss_mgr_check(struct omap_overlay_manager *mgr,
 		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
@@ -668,6 +679,10 @@ int dss_mgr_check(struct omap_overlay_manager *mgr,
 			return r;
 	}
 
+	r = dss_mgr_check_timings(mgr, &dssdev->panel.timings);
+	if (r)
+		return r;
+
 	list_for_each_entry(ovl, &mgr->overlays, list) {
 		struct omap_overlay_info *oi;
 		int r;
-- 
1.7.5.4

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

[Archit Taneja]

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 <archit@ti.com>
---
 drivers/video/omap2/dss/apply.c   |   51 +++++++++++++++++++++++++++---------
 drivers/video/omap2/dss/dss.h     |    5 +--
 drivers/video/omap2/dss/manager.c |    6 ++--
 drivers/video/omap2/dss/overlay.c |   20 +++++++-------
 4 files changed, 53 insertions(+), 29 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 42c3854..052d9a2 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -137,10 +137,30 @@ static struct mgr_priv_data *get_mgr_priv(struct omap_overlay_manager *mgr)
 void dss_apply_init(void)
 {
 	const int num_ovls = dss_feat_get_num_ovls();
+	const int num_mgrs = dss_feat_get_num_ovls();
 	int i;
+	/* Use dummy manager timings during initialization */
+	struct omap_video_timings timings = {
+		.hsw		= 1,
+		.hfp		= 1,
+		.hbp		= 1,
+		.vsw		= 1,
+		.vfp		= 0,
+		.vbp		= 0,
+		.x_res	= dss_feat_get_param_max(FEAT_PARAM_MGR_WIDTH),
+		.y_res	= dss_feat_get_param_max(FEAT_PARAM_MGR_HEIGHT),
+	};
 
 	spin_lock_init(&data_lock);
 
+	for (i = 0; i < num_mgrs; i++) {
+		struct mgr_priv_data *mp;
+
+		mp = &dss_data.mgr_priv_data_array[i];
+
+		mp->timings = timings;
+	}
+
 	for (i = 0; i < num_ovls; ++i) {
 		struct ovl_priv_data *op;
 
@@ -181,7 +201,7 @@ static bool mgr_manual_update(struct omap_overlay_manager *mgr)
 }
 
 static int dss_check_settings_low(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev, bool applying)
+		bool applying)
 {
 	struct omap_overlay_info *oi;
 	struct omap_overlay_manager_info *mi;
@@ -211,26 +231,24 @@ static int dss_check_settings_low(struct omap_overlay_manager *mgr,
 		ois[ovl->id] = oi;
 	}
 
-	return dss_mgr_check(mgr, dssdev, mi, ois);
+	return dss_mgr_check(mgr, mi, ois);
 }
 
 /*
  * check manager and overlay settings using overlay_info from data->info
  */
-static int dss_check_settings(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev)
+static int dss_check_settings(struct omap_overlay_manager *mgr)
 {
-	return dss_check_settings_low(mgr, dssdev, false);
+	return dss_check_settings_low(mgr, false);
 }
 
 /*
  * check manager and overlay settings using overlay_info from ovl->info if
  * dirty and from data->info otherwise
  */
-static int dss_check_settings_apply(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev)
+static int dss_check_settings_apply(struct omap_overlay_manager *mgr)
 {
-	return dss_check_settings_low(mgr, dssdev, true);
+	return dss_check_settings_low(mgr, true);
 }
 
 static bool need_isr(void)
@@ -684,7 +702,7 @@ static void dss_write_regs(void)
 		if (!mp->enabled || mgr_manual_update(mgr) || mp->busy)
 			continue;
 
-		r = dss_check_settings(mgr, mgr->device);
+		r = dss_check_settings(mgr);
 		if (r) {
 			DSSERR("cannot write registers for manager %s: "
 					"illegal configuration\n", mgr->name);
@@ -751,7 +769,7 @@ void dss_mgr_start_update(struct omap_overlay_manager *mgr)
 
 	WARN_ON(mp->updating);
 
-	r = dss_check_settings(mgr, mgr->device);
+	r = dss_check_settings(mgr);
 	if (r) {
 		DSSERR("cannot start manual update: illegal configuration\n");
 		spin_unlock_irqrestore(&data_lock, flags);
@@ -898,7 +916,7 @@ int omap_dss_mgr_apply(struct omap_overlay_manager *mgr)
 
 	spin_lock_irqsave(&data_lock, flags);
 
-	r = dss_check_settings_apply(mgr, mgr->device);
+	r = dss_check_settings_apply(mgr);
 	if (r) {
 		spin_unlock_irqrestore(&data_lock, flags);
 		DSSERR("failed to apply settings: illegal configuration.\n");
@@ -1091,7 +1109,7 @@ int dss_mgr_enable(struct omap_overlay_manager *mgr)
 
 	mp->enabled = true;
 
-	r = dss_check_settings(mgr, mgr->device);
+	r = dss_check_settings(mgr);
 	if (r) {
 		DSSERR("failed to enable manager %d: check_settings failed\n",
 				mgr->id);
@@ -1291,6 +1309,13 @@ void dss_mgr_set_timings(struct omap_overlay_manager *mgr,
 	mutex_unlock(&apply_lock);
 }
 
+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;
+}
+
 int dss_ovl_set_info(struct omap_overlay *ovl,
 		struct omap_overlay_info *info)
 {
@@ -1458,7 +1483,7 @@ int dss_ovl_enable(struct omap_overlay *ovl)
 
 	op->enabling = true;
 
-	r = dss_check_settings(ovl->manager, ovl->manager->device);
+	r = dss_check_settings(ovl->manager);
 	if (r) {
 		DSSERR("failed to enable overlay %d: check_settings failed\n",
 				ovl->id);
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index a148712..f2a51c6 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -179,6 +179,7 @@ int dss_mgr_set_device(struct omap_overlay_manager *mgr,
 int dss_mgr_unset_device(struct omap_overlay_manager *mgr);
 void dss_mgr_set_timings(struct omap_overlay_manager *mgr,
 		struct omap_video_timings *timings);
+struct omap_video_timings *dss_mgr_get_timings(struct omap_overlay_manager *mgr);
 
 bool dss_ovl_is_enabled(struct omap_overlay *ovl);
 int dss_ovl_enable(struct omap_overlay *ovl);
@@ -211,7 +212,6 @@ int dss_mgr_simple_check(struct omap_overlay_manager *mgr,
 int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
 		struct omap_video_timings *timings);
 int dss_mgr_check(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
 		struct omap_overlay_info **overlay_infos);
 
@@ -222,8 +222,7 @@ void dss_overlay_setup_dispc_manager(struct omap_overlay_manager *mgr);
 void dss_recheck_connections(struct omap_dss_device *dssdev, bool force);
 int dss_ovl_simple_check(struct omap_overlay *ovl,
 		const struct omap_overlay_info *info);
-int dss_ovl_check(struct omap_overlay *ovl,
-		struct omap_overlay_info *info, struct omap_dss_device *dssdev);
+int dss_ovl_check(struct omap_overlay *ovl, struct omap_overlay_info *info);
 
 /* DSS */
 int dss_init_platform_driver(void);
diff --git a/drivers/video/omap2/dss/manager.c b/drivers/video/omap2/dss/manager.c
index 264ad7c..1fd339b 100644
--- a/drivers/video/omap2/dss/manager.c
+++ b/drivers/video/omap2/dss/manager.c
@@ -666,11 +666,11 @@ int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
 }
 
 int dss_mgr_check(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
 		struct omap_overlay_info **overlay_infos)
 {
 	struct omap_overlay *ovl;
+	struct omap_video_timings *mgr_timings = dss_mgr_get_timings(mgr);
 	int r;
 
 	if (dss_has_feature(FEAT_ALPHA_FREE_ZORDER)) {
@@ -679,7 +679,7 @@ int dss_mgr_check(struct omap_overlay_manager *mgr,
 			return r;
 	}
 
-	r = dss_mgr_check_timings(mgr, &dssdev->panel.timings);
+	r = dss_mgr_check_timings(mgr, mgr_timings);
 	if (r)
 		return r;
 
@@ -692,7 +692,7 @@ int dss_mgr_check(struct omap_overlay_manager *mgr,
 		if (oi = NULL)
 			continue;
 
-		r = dss_ovl_check(ovl, oi, dssdev);
+		r = dss_ovl_check(ovl, oi);
 		if (r)
 			return r;
 	}
diff --git a/drivers/video/omap2/dss/overlay.c b/drivers/video/omap2/dss/overlay.c
index 6e82181..8629e1a 100644
--- a/drivers/video/omap2/dss/overlay.c
+++ b/drivers/video/omap2/dss/overlay.c
@@ -631,16 +631,16 @@ int dss_ovl_simple_check(struct omap_overlay *ovl,
 	return 0;
 }
 
-int dss_ovl_check(struct omap_overlay *ovl,
-		struct omap_overlay_info *info, struct omap_dss_device *dssdev)
+int dss_ovl_check(struct omap_overlay *ovl, struct omap_overlay_info *info)
 {
 	u16 outw, outh;
-	u16 dw, dh;
+	struct omap_video_timings *mgr_timings;
+	u16 mgr_width, mgr_height;
 
-	if (dssdev = NULL)
-		return 0;
+	mgr_timings = dss_mgr_get_timings(ovl->manager);
 
-	dssdev->driver->get_resolution(dssdev, &dw, &dh);
+	mgr_width = mgr_timings->x_res;
+	mgr_height = mgr_timings->y_res;
 
 	if ((ovl->caps & OMAP_DSS_OVL_CAP_SCALE) = 0) {
 		outw = info->width;
@@ -657,17 +657,17 @@ int dss_ovl_check(struct omap_overlay *ovl,
 			outh = info->out_height;
 	}
 
-	if (dw < info->pos_x + outw) {
+	if (mgr_width < info->pos_x + outw) {
 		DSSERR("overlay %d horizontally not inside the display area "
 				"(%d + %d >= %d)\n",
-				ovl->id, info->pos_x, outw, dw);
+				ovl->id, info->pos_x, outw, mgr_width);
 		return -EINVAL;
 	}
 
-	if (dh < info->pos_y + outh) {
+	if (mgr_height < info->pos_y + outh) {
 		DSSERR("overlay %d vertically not inside the display area "
 				"(%d + %d >= %d)\n",
-				ovl->id, info->pos_y, outh, dh);
+				ovl->id, info->pos_y, outh, mgr_height);
 		return -EINVAL;
 	}
 
-- 
1.7.5.4

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

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 <archit@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.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Archit Taneja]

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<archit@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?

Archit

>
>   Tomi
>

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2872 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Archit Taneja]

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<archit@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?

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
>

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2602 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Archit Taneja]

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
>

[PATCH v3 0/5] OMAPDSS: APPLY: Treat overlay manager timings as shadow registers

[Archit Taneja]

An overlay manager's timings (the manager size, and blanking parameters if an
LCD manager) are DISPC shadow registers, and they should hence follow the
correct programming model.

This set makes the timings an extra_info parameter in manager's private data .
The interface drivers now apply the timings in instead of directly writing to
registers.

This change also prevents the need to use display resolution for overlay
checks, hence making some of the APPLY functions less dependent on the display.

Changes since v3:

- Make sure that we write to the timing registers and set go bits when calling
  dss_mgr_set_timings
- Don't create dss_mgr_get_timings(), apply should pass the timings rather than
  outside DSS dunctions querying form it.

These patches apply over:

git://gitorious.org/linux-omap-dss2/linux.git dev

Archit Taneja (5):
  OMAPDSS: APPLY: Add manager timings as extra_info in private data
  OMAPDSS: Apply manager timings instead of direct DISPC writes
  OMAPDSS: MANAGER: Create a function to check manager timings
  OMAPDSS: APPLY: Remove display dependency from overlay and manager
    checks
  OMAPDSS: DPI/HDMI: Apply manager timings even if panel is disabled

 drivers/video/omap2/dss/apply.c   |  140 ++++++++++++++++++++++++++++++-------
 drivers/video/omap2/dss/dispc.c   |    2 +-
 drivers/video/omap2/dss/dpi.c     |    6 +-
 drivers/video/omap2/dss/dsi.c     |    5 +-
 drivers/video/omap2/dss/dss.h     |   13 +++-
 drivers/video/omap2/dss/hdmi.c    |    4 +-
 drivers/video/omap2/dss/manager.c |   19 +++++-
 drivers/video/omap2/dss/overlay.c |   20 +++---
 drivers/video/omap2/dss/rfbi.c    |    4 +-
 drivers/video/omap2/dss/sdi.c     |    2 +-
 drivers/video/omap2/dss/venc.c    |    2 +-
 11 files changed, 163 insertions(+), 54 deletions(-)

-- 
1.7.5.4

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

[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 and writes it to the registers and set the go bits.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/apply.c |   96 +++++++++++++++++++++++++++++++++-----
 drivers/video/omap2/dss/dss.h   |    2 +
 2 files changed, 85 insertions(+), 13 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index b10b3bc..57686f6 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)
 		}
 
 		dss_mgr_write_regs(mgr);
+		dss_mgr_write_regs_extra(mgr);
 	}
 }
 
@@ -693,6 +732,7 @@ static void mgr_clear_shadow_dirty(struct omap_overlay_manager *mgr)
 
 	mp = get_mgr_priv(mgr);
 	mp->shadow_info_dirty = false;
+	mp->shadow_extra_info_dirty = false;
 
 	list_for_each_entry(ovl, &mgr->overlays, list) {
 		op = get_ovl_priv(ovl);
@@ -719,6 +759,7 @@ void dss_mgr_start_update(struct omap_overlay_manager *mgr)
 	}
 
 	dss_mgr_write_regs(mgr);
+	dss_mgr_write_regs_extra(mgr);
 
 	dss_write_regs_common();
 
@@ -1225,6 +1266,35 @@ err:
 	return r;
 }
 
+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);
+
+	dss_write_regs();
+	dss_set_go_bits();
+
+	spin_unlock_irqrestore(&data_lock, flags);
+
+	wait_pending_extra_info_updates();
+
+	mutex_unlock(&apply_lock);
+}
 
 int dss_ovl_set_info(struct omap_overlay *ovl,
 		struct omap_overlay_info *info)
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 5ca67f1..ca59481 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -177,6 +177,8 @@ void dss_mgr_get_info(struct omap_overlay_manager *mgr,
 int dss_mgr_set_device(struct omap_overlay_manager *mgr,
 		struct omap_dss_device *dssdev);
 int dss_mgr_unset_device(struct omap_overlay_manager *mgr);
+void dss_mgr_set_timings(struct omap_overlay_manager *mgr,
+		struct omap_video_timings *timings);
 
 bool dss_ovl_is_enabled(struct omap_overlay *ovl);
 int dss_ovl_enable(struct omap_overlay *ovl);
-- 
1.7.5.4

[PATCH v3 2/5] OMAPDSS: Apply manager timings instead of direct DISPC writes

[Archit Taneja]

Replace the function dispc_mgr_set_timings() with dss_mgr_set_timings() in the
interface drivers. The latter function ensures that the timing related DISPC
registers are configured according to the shadow register programming model.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dpi.c  |    2 +-
 drivers/video/omap2/dss/dsi.c  |    5 ++---
 drivers/video/omap2/dss/hdmi.c |    2 +-
 drivers/video/omap2/dss/rfbi.c |    4 ++--
 drivers/video/omap2/dss/sdi.c  |    2 +-
 drivers/video/omap2/dss/venc.c |    2 +-
 6 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index cec1166..5d84ab0 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -156,7 +156,7 @@ static int dpi_set_mode(struct omap_dss_device *dssdev)
 		t->pixel_clock = pck;
 	}
 
-	dispc_mgr_set_timings(dssdev->manager->id, t);
+	dss_mgr_set_timings(dssdev->manager, t);
 
 	return 0;
 }
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index b6cf03c..db73598 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -4219,13 +4219,12 @@ static int dsi_display_init_dispc(struct omap_dss_device *dssdev)
 		dispc_mgr_enable_stallmode(dssdev->manager->id, true);
 		dispc_mgr_enable_fifohandcheck(dssdev->manager->id, 1);
 
-		dispc_mgr_set_timings(dssdev->manager->id, &timings);
+		dss_mgr_set_timings(dssdev->manager, &timings);
 	} else {
 		dispc_mgr_enable_stallmode(dssdev->manager->id, false);
 		dispc_mgr_enable_fifohandcheck(dssdev->manager->id, 0);
 
-		dispc_mgr_set_timings(dssdev->manager->id,
-			&dssdev->panel.timings);
+		dss_mgr_set_timings(dssdev->manager, &dssdev->panel.timings);
 	}
 
 		dispc_mgr_set_lcd_display_type(dssdev->manager->id,
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 56f6e9c..8d4ff8c 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -376,7 +376,7 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)
 	dispc_enable_gamma_table(0);
 
 	/* tv size */
-	dispc_mgr_set_timings(dssdev->manager->id, &dssdev->panel.timings);
+	dss_mgr_set_timings(dssdev->manager, &dssdev->panel.timings);
 
 	hdmi.ip_data.ops->video_enable(&hdmi.ip_data, 1);
 
diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
index a81ffcb..feadfab 100644
--- a/drivers/video/omap2/dss/rfbi.c
+++ b/drivers/video/omap2/dss/rfbi.c
@@ -320,7 +320,7 @@ static void rfbi_transfer_area(struct omap_dss_device *dssdev, u16 width,
 
 	DSSDBG("rfbi_transfer_area %dx%d\n", width, height);
 
-	dispc_mgr_set_timings(dssdev->manager->id, &timings);
+	dss_mgr_set_timings(dssdev->manager, &timings);
 
 	dispc_mgr_enable(dssdev->manager->id, true);
 
@@ -804,7 +804,7 @@ int omap_rfbi_prepare_update(struct omap_dss_device *dssdev,
 	if (*w = 0 || *h = 0)
 		return -EINVAL;
 
-	dispc_mgr_set_timings(dssdev->manager->id, &timings);
+	dss_mgr_set_timings(dssdev->manager, &timings);
 
 	return 0;
 }
diff --git a/drivers/video/omap2/dss/sdi.c b/drivers/video/omap2/dss/sdi.c
index 741b834..67fbe7c 100644
--- a/drivers/video/omap2/dss/sdi.c
+++ b/drivers/video/omap2/dss/sdi.c
@@ -107,7 +107,7 @@ int omapdss_sdi_display_enable(struct omap_dss_device *dssdev)
 	}
 
 
-	dispc_mgr_set_timings(dssdev->manager->id, t);
+	dss_mgr_set_timings(dssdev->manager, t);
 
 	r = dss_set_clock_div(&dss_cinfo);
 	if (r)
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 30bbb63..e237464 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -444,7 +444,7 @@ static int venc_power_on(struct omap_dss_device *dssdev)
 	timings = dssdev->panel.timings;
 	timings.y_res /= 2;
 
-	dispc_mgr_set_timings(dssdev->manager->id, &timings);
+	dss_mgr_set_timings(dssdev->manager, &timings);
 
 	r = regulator_enable(venc.vdda_dac_reg);
 	if (r)
-- 
1.7.5.4

Re: [PATCH v3 2/5] OMAPDSS: Apply manager timings instead of direct DISPC writes

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1768 bytes --]

On Tue, 2012-05-08 at 15:28 +0530, Archit Taneja wrote:
> Replace the function dispc_mgr_set_timings() with dss_mgr_set_timings() in the
> interface drivers. The latter function ensures that the timing related DISPC
> registers are configured according to the shadow register programming model.
> 
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
>  drivers/video/omap2/dss/dpi.c  |    2 +-
>  drivers/video/omap2/dss/dsi.c  |    5 ++---
>  drivers/video/omap2/dss/hdmi.c |    2 +-
>  drivers/video/omap2/dss/rfbi.c |    4 ++--
>  drivers/video/omap2/dss/sdi.c  |    2 +-
>  drivers/video/omap2/dss/venc.c |    2 +-
>  6 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
> index cec1166..5d84ab0 100644
> --- a/drivers/video/omap2/dss/dpi.c
> +++ b/drivers/video/omap2/dss/dpi.c
> @@ -156,7 +156,7 @@ static int dpi_set_mode(struct omap_dss_device *dssdev)
>  		t->pixel_clock = pck;
>  	}
>  
> -	dispc_mgr_set_timings(dssdev->manager->id, t);
> +	dss_mgr_set_timings(dssdev->manager, t);
>  
>  	return 0;
>  }

I think you can now remove the dispc_mgr_go() from dpi.c in this patch.

And something else, which doesn't need to be fixed now, but just to
point out: dpi_set_timings() currently uses runtime_get to enable the
HW. If everything was in proper shape, this wouldn't be needed.
dpi_set_timings() would call apply.c's functions, and if the output is
disabled, the settings would just be stored in ram. So there wouldn't be
any need to enable the HW with runtime_get().

Then again, if we do change clock settings or non-shadow registers in
dpi_set_timings(), then that's not possible (at least via apply.c's
caching system).

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v3 3/5] OMAPDSS: MANAGER: Create a function to check manager timings

[Archit Taneja]

Create a function dss_mgr_check_timings() which wraps around the function
dispc_mgr_timings_ok(). This is mainly a clean up to hide dispc functions
from interface drivers.

dss_mgr_check_timings() is added in the function dss_mgr_check(), it currently
takes the timings maintained in the omap_dss_device struct. This would be later
replaced by the timings stored in the manager's private data.

Make dss_mgr_check_timings() and dispc_mgr_timings_ok() take a const
omap_video_timings pointer since these functions just check the timings.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dispc.c   |    2 +-
 drivers/video/omap2/dss/dpi.c     |    2 +-
 drivers/video/omap2/dss/dss.h     |    4 +++-
 drivers/video/omap2/dss/manager.c |   15 +++++++++++++++
 4 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 49015b8..2829a92 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -2479,7 +2479,7 @@ static bool _dispc_lcd_timings_ok(int hsw, int hfp, int hbp,
 }
 
 bool dispc_mgr_timings_ok(enum omap_channel channel,
-		struct omap_video_timings *timings)
+		const struct omap_video_timings *timings)
 {
 	bool timings_ok;
 
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index 5d84ab0..8127f46 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -312,7 +312,7 @@ int dpi_check_timings(struct omap_dss_device *dssdev,
 	unsigned long pck;
 	struct dispc_clock_info dispc_cinfo;
 
-	if (!dispc_mgr_timings_ok(dssdev->manager->id, timings))
+	if (!dss_mgr_check_timings(dssdev->manager, timings))
 		return -EINVAL;
 
 	if (timings->pixel_clock = 0)
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index ca59481..7c9b9bc 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -208,6 +208,8 @@ int dss_init_overlay_managers(struct platform_device *pdev);
 void dss_uninit_overlay_managers(struct platform_device *pdev);
 int dss_mgr_simple_check(struct omap_overlay_manager *mgr,
 		const struct omap_overlay_manager_info *info);
+int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
+		const struct omap_video_timings *timings);
 int dss_mgr_check(struct omap_overlay_manager *mgr,
 		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
@@ -414,7 +416,7 @@ void dispc_enable_gamma_table(bool enable);
 void dispc_set_loadmode(enum omap_dss_load_mode mode);
 
 bool dispc_mgr_timings_ok(enum omap_channel channel,
-		struct omap_video_timings *timings);
+		const struct omap_video_timings *timings);
 unsigned long dispc_fclk_rate(void);
 void dispc_find_clk_divs(bool is_tft, unsigned long req_pck, unsigned long fck,
 		struct dispc_clock_info *cinfo);
diff --git a/drivers/video/omap2/dss/manager.c b/drivers/video/omap2/dss/manager.c
index e736460..566fbba 100644
--- a/drivers/video/omap2/dss/manager.c
+++ b/drivers/video/omap2/dss/manager.c
@@ -654,6 +654,17 @@ static int dss_mgr_check_zorder(struct omap_overlay_manager *mgr,
 	return 0;
 }
 
+int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
+		const struct omap_video_timings *timings)
+{
+	if (!dispc_mgr_timings_ok(mgr->id, timings)) {
+		DSSERR("check_manager: invalid timings\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int dss_mgr_check(struct omap_overlay_manager *mgr,
 		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
@@ -668,6 +679,10 @@ int dss_mgr_check(struct omap_overlay_manager *mgr,
 			return r;
 	}
 
+	r = dss_mgr_check_timings(mgr, &dssdev->panel.timings);
+	if (r)
+		return r;
+
 	list_for_each_entry(ovl, &mgr->overlays, list) {
 		struct omap_overlay_info *oi;
 		int r;
-- 
1.7.5.4

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

[Archit Taneja]

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.

Pass the manager's timings(in private data) to dss_mgr_check(). 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 <archit@ti.com>
---
 drivers/video/omap2/dss/apply.c   |   44 ++++++++++++++++++++++++++-----------
 drivers/video/omap2/dss/dss.h     |    7 +++--
 drivers/video/omap2/dss/manager.c |    6 ++--
 drivers/video/omap2/dss/overlay.c |   20 +++++++---------
 4 files changed, 47 insertions(+), 30 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 57686f6..7d59caf 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -137,10 +137,30 @@ static struct mgr_priv_data *get_mgr_priv(struct omap_overlay_manager *mgr)
 void dss_apply_init(void)
 {
 	const int num_ovls = dss_feat_get_num_ovls();
+	const int num_mgrs = dss_feat_get_num_ovls();
 	int i;
+	/* Use dummy manager timings during initialization */
+	struct omap_video_timings timings = {
+		.hsw		= 1,
+		.hfp		= 1,
+		.hbp		= 1,
+		.vsw		= 1,
+		.vfp		= 0,
+		.vbp		= 0,
+		.x_res	= dss_feat_get_param_max(FEAT_PARAM_MGR_WIDTH),
+		.y_res	= dss_feat_get_param_max(FEAT_PARAM_MGR_HEIGHT),
+	};
 
 	spin_lock_init(&data_lock);
 
+	for (i = 0; i < num_mgrs; i++) {
+		struct mgr_priv_data *mp;
+
+		mp = &dss_data.mgr_priv_data_array[i];
+
+		mp->timings = timings;
+	}
+
 	for (i = 0; i < num_ovls; ++i) {
 		struct ovl_priv_data *op;
 
@@ -181,7 +201,7 @@ static bool mgr_manual_update(struct omap_overlay_manager *mgr)
 }
 
 static int dss_check_settings_low(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev, bool applying)
+		bool applying)
 {
 	struct omap_overlay_info *oi;
 	struct omap_overlay_manager_info *mi;
@@ -211,26 +231,24 @@ static int dss_check_settings_low(struct omap_overlay_manager *mgr,
 		ois[ovl->id] = oi;
 	}
 
-	return dss_mgr_check(mgr, dssdev, mi, ois);
+	return dss_mgr_check(mgr, mi, &mp->timings, ois);
 }
 
 /*
  * check manager and overlay settings using overlay_info from data->info
  */
-static int dss_check_settings(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev)
+static int dss_check_settings(struct omap_overlay_manager *mgr)
 {
-	return dss_check_settings_low(mgr, dssdev, false);
+	return dss_check_settings_low(mgr, false);
 }
 
 /*
  * check manager and overlay settings using overlay_info from ovl->info if
  * dirty and from data->info otherwise
  */
-static int dss_check_settings_apply(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev)
+static int dss_check_settings_apply(struct omap_overlay_manager *mgr)
 {
-	return dss_check_settings_low(mgr, dssdev, true);
+	return dss_check_settings_low(mgr, true);
 }
 
 static bool need_isr(void)
@@ -684,7 +702,7 @@ static void dss_write_regs(void)
 		if (!mp->enabled || mgr_manual_update(mgr) || mp->busy)
 			continue;
 
-		r = dss_check_settings(mgr, mgr->device);
+		r = dss_check_settings(mgr);
 		if (r) {
 			DSSERR("cannot write registers for manager %s: "
 					"illegal configuration\n", mgr->name);
@@ -751,7 +769,7 @@ void dss_mgr_start_update(struct omap_overlay_manager *mgr)
 
 	WARN_ON(mp->updating);
 
-	r = dss_check_settings(mgr, mgr->device);
+	r = dss_check_settings(mgr);
 	if (r) {
 		DSSERR("cannot start manual update: illegal configuration\n");
 		spin_unlock_irqrestore(&data_lock, flags);
@@ -898,7 +916,7 @@ int omap_dss_mgr_apply(struct omap_overlay_manager *mgr)
 
 	spin_lock_irqsave(&data_lock, flags);
 
-	r = dss_check_settings_apply(mgr, mgr->device);
+	r = dss_check_settings_apply(mgr);
 	if (r) {
 		spin_unlock_irqrestore(&data_lock, flags);
 		DSSERR("failed to apply settings: illegal configuration.\n");
@@ -1091,7 +1109,7 @@ int dss_mgr_enable(struct omap_overlay_manager *mgr)
 
 	mp->enabled = true;
 
-	r = dss_check_settings(mgr, mgr->device);
+	r = dss_check_settings(mgr);
 	if (r) {
 		DSSERR("failed to enable manager %d: check_settings failed\n",
 				mgr->id);
@@ -1463,7 +1481,7 @@ int dss_ovl_enable(struct omap_overlay *ovl)
 
 	op->enabling = true;
 
-	r = dss_check_settings(ovl->manager, ovl->manager->device);
+	r = dss_check_settings(ovl->manager);
 	if (r) {
 		DSSERR("failed to enable overlay %d: check_settings failed\n",
 				ovl->id);
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 7c9b9bc..eff3d7f 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -179,6 +179,7 @@ int dss_mgr_set_device(struct omap_overlay_manager *mgr,
 int dss_mgr_unset_device(struct omap_overlay_manager *mgr);
 void dss_mgr_set_timings(struct omap_overlay_manager *mgr,
 		struct omap_video_timings *timings);
+const struct omap_video_timings *dss_mgr_get_timings(struct omap_overlay_manager *mgr);
 
 bool dss_ovl_is_enabled(struct omap_overlay *ovl);
 int dss_ovl_enable(struct omap_overlay *ovl);
@@ -211,8 +212,8 @@ int dss_mgr_simple_check(struct omap_overlay_manager *mgr,
 int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
 		const struct omap_video_timings *timings);
 int dss_mgr_check(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
+		const struct omap_video_timings *mgr_timings,
 		struct omap_overlay_info **overlay_infos);
 
 /* overlay */
@@ -222,8 +223,8 @@ void dss_overlay_setup_dispc_manager(struct omap_overlay_manager *mgr);
 void dss_recheck_connections(struct omap_dss_device *dssdev, bool force);
 int dss_ovl_simple_check(struct omap_overlay *ovl,
 		const struct omap_overlay_info *info);
-int dss_ovl_check(struct omap_overlay *ovl,
-		struct omap_overlay_info *info, struct omap_dss_device *dssdev);
+int dss_ovl_check(struct omap_overlay *ovl, struct omap_overlay_info *info,
+		const struct omap_video_timings *mgr_timings);
 
 /* DSS */
 int dss_init_platform_driver(void);
diff --git a/drivers/video/omap2/dss/manager.c b/drivers/video/omap2/dss/manager.c
index 566fbba..0cbcde4 100644
--- a/drivers/video/omap2/dss/manager.c
+++ b/drivers/video/omap2/dss/manager.c
@@ -666,8 +666,8 @@ int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
 }
 
 int dss_mgr_check(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
+		const struct omap_video_timings *mgr_timings,
 		struct omap_overlay_info **overlay_infos)
 {
 	struct omap_overlay *ovl;
@@ -679,7 +679,7 @@ int dss_mgr_check(struct omap_overlay_manager *mgr,
 			return r;
 	}
 
-	r = dss_mgr_check_timings(mgr, &dssdev->panel.timings);
+	r = dss_mgr_check_timings(mgr, mgr_timings);
 	if (r)
 		return r;
 
@@ -692,7 +692,7 @@ int dss_mgr_check(struct omap_overlay_manager *mgr,
 		if (oi = NULL)
 			continue;
 
-		r = dss_ovl_check(ovl, oi, dssdev);
+		r = dss_ovl_check(ovl, oi, mgr_timings);
 		if (r)
 			return r;
 	}
diff --git a/drivers/video/omap2/dss/overlay.c b/drivers/video/omap2/dss/overlay.c
index 6e82181..74db90c 100644
--- a/drivers/video/omap2/dss/overlay.c
+++ b/drivers/video/omap2/dss/overlay.c
@@ -631,16 +631,14 @@ int dss_ovl_simple_check(struct omap_overlay *ovl,
 	return 0;
 }
 
-int dss_ovl_check(struct omap_overlay *ovl,
-		struct omap_overlay_info *info, struct omap_dss_device *dssdev)
+int dss_ovl_check(struct omap_overlay *ovl, struct omap_overlay_info *info,
+		const struct omap_video_timings *mgr_timings)
 {
 	u16 outw, outh;
-	u16 dw, dh;
+	u16 mgr_width, mgr_height;
 
-	if (dssdev = NULL)
-		return 0;
-
-	dssdev->driver->get_resolution(dssdev, &dw, &dh);
+	mgr_width = mgr_timings->x_res;
+	mgr_height = mgr_timings->y_res;
 
 	if ((ovl->caps & OMAP_DSS_OVL_CAP_SCALE) = 0) {
 		outw = info->width;
@@ -657,17 +655,17 @@ int dss_ovl_check(struct omap_overlay *ovl,
 			outh = info->out_height;
 	}
 
-	if (dw < info->pos_x + outw) {
+	if (mgr_width < info->pos_x + outw) {
 		DSSERR("overlay %d horizontally not inside the display area "
 				"(%d + %d >= %d)\n",
-				ovl->id, info->pos_x, outw, dw);
+				ovl->id, info->pos_x, outw, mgr_width);
 		return -EINVAL;
 	}
 
-	if (dh < info->pos_y + outh) {
+	if (mgr_height < info->pos_y + outh) {
 		DSSERR("overlay %d vertically not inside the display area "
 				"(%d + %d >= %d)\n",
-				ovl->id, info->pos_y, outh, dh);
+				ovl->id, info->pos_y, outh, mgr_height);
 		return -EINVAL;
 	}
 
-- 
1.7.5.4

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2000 bytes --]

On Tue, 2012-05-08 at 15:28 +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.
> 
> Pass the manager's timings(in private data) to dss_mgr_check(). 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.

In what code patch were the initial values needed?

Checking the validity of all the settings is a bit tricky, but currently
I think, as a rule of thumb, we should accept any settings when things
are disabled. So, until the interface driver sets the timings before
enabling the output, all ovl/mgr settings should be allowed. And we
shouldn't even write any shadow registers until the moment the output is
enabled.

Except settings that do not depend on anything, like, if max ovl width
is 2048, it's fine to reject 2049+ width anytime.

> -int dss_ovl_check(struct omap_overlay *ovl,
> -		struct omap_overlay_info *info, struct omap_dss_device *dssdev)
> +int dss_ovl_check(struct omap_overlay *ovl, struct omap_overlay_info *info,
> +		const struct omap_video_timings *mgr_timings)
>  {
>  	u16 outw, outh;
> -	u16 dw, dh;
> +	u16 mgr_width, mgr_height;
>  
> -	if (dssdev == NULL)
> -		return 0;
> -
> -	dssdev->driver->get_resolution(dssdev, &dw, &dh);
> +	mgr_width = mgr_timings->x_res;
> +	mgr_height = mgr_timings->y_res;

I think you could've just used the existing dw and dh variables, thus
avoiding the need to change the rest of the function. The 'd' refers to
display, which is more or less the same as mgr width.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Archit Taneja]

On Tuesday 08 May 2012 04:20 PM, Tomi Valkeinen wrote:
> On Tue, 2012-05-08 at 15:28 +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.
>>
>> Pass the manager's timings(in private data) to dss_mgr_check(). 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.
>
> In what code patch were the initial values needed?

I guess you meant code path. If a panel driver doesn't have set_timings 
op, then we get into trouble with omapfb. The following steps happen:

- try to set timings of display
- create buffers, configure overlays according to omap_dss_device
- call mgr->apply()
- enable the panel

For panels which don't have set_timings populated, if apply is called, 
the manager timings are still zero, and the mgr->apply fails when 
dss_mgr_check() is called.

When the panel driver's enable is called, the timings are configured in 
the interface drivers 'enable'. The enables(like 
dpi_display_enable,dsi_display_enable) of all interface driver's call 
dss_mgr_set_timings(), so we are sure that the timings are configured by 
then. But there is no such guarantee at set timings.

Things used to work previously because we used to simply get the 
connected panel and use its dimensions, if there was no panel connected, 
we used to skip the check.

>
> Checking the validity of all the settings is a bit tricky, but currently
> I think, as a rule of thumb, we should accept any settings when things
> are disabled. So, until the interface driver sets the timings before
> enabling the output, all ovl/mgr settings should be allowed. And we

We have 2 ways to go about this, one is to have an initial set of 
'always valid' values like I have done, the other option is to ignore 
manager timing related checks if the manager is disabled, i.e all 
configs are okay. To implement the second option, I think our function 
dss_check_settings_low() would get more complicated. We would now have 
to pass mp->enabled outside of apply, and pass it to dss_mgr_check() 
which would further need to pass this to dss_ovl_check(). Hence I used 
the first approach.

> shouldn't even write any shadow registers until the moment the output is
> enabled.

That's being done correctly even now I guess, with the checks for 
mp->enabled in wrtie_regs() and set_go_bits().

>
> Except settings that do not depend on anything, like, if max ovl width
> is 2048, it's fine to reject 2049+ width anytime.
>
>> -int dss_ovl_check(struct omap_overlay *ovl,
>> -		struct omap_overlay_info *info, struct omap_dss_device *dssdev)
>> +int dss_ovl_check(struct omap_overlay *ovl, struct omap_overlay_info *info,
>> +		const struct omap_video_timings *mgr_timings)
>>   {
>>   	u16 outw, outh;
>> -	u16 dw, dh;
>> +	u16 mgr_width, mgr_height;
>>
>> -	if (dssdev = NULL)
>> -		return 0;
>> -
>> -	dssdev->driver->get_resolution(dssdev,&dw,&dh);
>> +	mgr_width = mgr_timings->x_res;
>> +	mgr_height = mgr_timings->y_res;
>
> I think you could've just used the existing dw and dh variables, thus
> avoiding the need to change the rest of the function. The 'd' refers to
> display, which is more or less the same as mgr width.

I'll fix this.

Archit

>
>   Tomi
>

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1949 bytes --]

On Tue, 2012-05-08 at 16:52 +0530, Archit Taneja wrote:
> On Tuesday 08 May 2012 04:20 PM, Tomi Valkeinen wrote:

> > Checking the validity of all the settings is a bit tricky, but currently
> > I think, as a rule of thumb, we should accept any settings when things
> > are disabled. So, until the interface driver sets the timings before
> > enabling the output, all ovl/mgr settings should be allowed. And we
> 
> We have 2 ways to go about this, one is to have an initial set of 
> 'always valid' values like I have done, the other option is to ignore 
> manager timing related checks if the manager is disabled, i.e all 
> configs are okay. To implement the second option, I think our function 
> dss_check_settings_low() would get more complicated. We would now have 
> to pass mp->enabled outside of apply, and pass it to dss_mgr_check() 
> which would further need to pass this to dss_ovl_check(). Hence I used 
> the first approach.

I'm not sure about that. We already do it for overlay. In ovl.c we have
dss_ovl_simple_check() and dss_ovl_check(). The simple check sees if the
settings pass basic sanity check. The check sees if all the ovl & mgr
settings are compatible with each other.

Simple check is done when setting the config, called from
dss_ovl_set_info(). The proper check is done later when the actual
config is about to be taken into use.

If mp->enabled == false, can't we just skip dss_check_settings_low()
totally for that manager? We skip the check for ovls the same way.

> > shouldn't even write any shadow registers until the moment the output is
> > enabled.
> 
> That's being done correctly even now I guess, with the checks for 
> mp->enabled in wrtie_regs() and set_go_bits().

Yes, for timings. I was thinking more about the other settings done in
dpi.c currently, like dispc_mgr_set_pol_freq(). That writes directly to
registers, so we need runtime_get for that. 

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Archit Taneja]

On Tuesday 08 May 2012 05:25 PM, Tomi Valkeinen wrote:
> On Tue, 2012-05-08 at 16:52 +0530, Archit Taneja wrote:
>> On Tuesday 08 May 2012 04:20 PM, Tomi Valkeinen wrote:
>
>>> Checking the validity of all the settings is a bit tricky, but currently
>>> I think, as a rule of thumb, we should accept any settings when things
>>> are disabled. So, until the interface driver sets the timings before
>>> enabling the output, all ovl/mgr settings should be allowed. And we
>>
>> We have 2 ways to go about this, one is to have an initial set of
>> 'always valid' values like I have done, the other option is to ignore
>> manager timing related checks if the manager is disabled, i.e all
>> configs are okay. To implement the second option, I think our function
>> dss_check_settings_low() would get more complicated. We would now have
>> to pass mp->enabled outside of apply, and pass it to dss_mgr_check()
>> which would further need to pass this to dss_ovl_check(). Hence I used
>> the first approach.
>
> I'm not sure about that. We already do it for overlay. In ovl.c we have
> dss_ovl_simple_check() and dss_ovl_check(). The simple check sees if the
> settings pass basic sanity check. The check sees if all the ovl&  mgr
> settings are compatible with each other.
>
> Simple check is done when setting the config, called from
> dss_ovl_set_info(). The proper check is done later when the actual
> config is about to be taken into use.
>
> If mp->enabled = false, can't we just skip dss_check_settings_low()
> totally for that manager? We skip the check for ovls the same way.

Okay, this would work better I guess. If someone tries to enable the 
manager without setting the timings, that check should fail, and in my 
approach, it would have passed, and would have led to bad stuff later.

>
>>> shouldn't even write any shadow registers until the moment the output is
>>> enabled.
>>
>> That's being done correctly even now I guess, with the checks for
>> mp->enabled in wrtie_regs() and set_go_bits().
>
> Yes, for timings. I was thinking more about the other settings done in
> dpi.c currently, like dispc_mgr_set_pol_freq(). That writes directly to
> registers, so we need runtime_get for that.

Ok.

Archit

>
>   Tomi
>

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

[Archit Taneja]

On Tuesday 08 May 2012 06:05 PM, Archit Taneja wrote:
> On Tuesday 08 May 2012 05:25 PM, Tomi Valkeinen wrote:
>> On Tue, 2012-05-08 at 16:52 +0530, Archit Taneja wrote:
>>> On Tuesday 08 May 2012 04:20 PM, Tomi Valkeinen wrote:
>>
>>>> Checking the validity of all the settings is a bit tricky, but
>>>> currently
>>>> I think, as a rule of thumb, we should accept any settings when things
>>>> are disabled. So, until the interface driver sets the timings before
>>>> enabling the output, all ovl/mgr settings should be allowed. And we
>>>
>>> We have 2 ways to go about this, one is to have an initial set of
>>> 'always valid' values like I have done, the other option is to ignore
>>> manager timing related checks if the manager is disabled, i.e all
>>> configs are okay. To implement the second option, I think our function
>>> dss_check_settings_low() would get more complicated. We would now have
>>> to pass mp->enabled outside of apply, and pass it to dss_mgr_check()
>>> which would further need to pass this to dss_ovl_check(). Hence I used
>>> the first approach.
>>
>> I'm not sure about that. We already do it for overlay. In ovl.c we have
>> dss_ovl_simple_check() and dss_ovl_check(). The simple check sees if the
>> settings pass basic sanity check. The check sees if all the ovl& mgr
>> settings are compatible with each other.
>>
>> Simple check is done when setting the config, called from
>> dss_ovl_set_info(). The proper check is done later when the actual
>> config is about to be taken into use.
>>
>> If mp->enabled = false, can't we just skip dss_check_settings_low()
>> totally for that manager? We skip the check for ovls the same way.
>
> Okay, this would work better I guess. If someone tries to enable the
> manager without setting the timings, that check should fail, and in my
> approach, it would have passed, and would have led to bad stuff later.

One change in behaviour with the patch series I see is with the 
following use case when connected to Pandaboad's DVI interface:

- Bootup with 1080p resolution
- Try to set 640x480 timings with sysfs.

The older behaviour was that the timings were taken, and a skewed image 
was seen(because the overlay size is larger in deimension)

The behaviour after this series is that the mgr_set_timings fails with 
the error:

omapdss OVERLAY error: overlay 0 horizontally not inside the display 
area (0 + 1920 >= 640)

I guess this is better in a way, a user of DSS is supposed to reallocate 
the framebuffer with the desired size and then change the timings.

But with DVI, the problem is that dpi_set_timings already does a ton of 
stuff before dss_mgr_set_timings() is called, so we change the dividers 
to get the new clock, and then realise that the overlay can't fit 
inside, and we are left nowhere. I guess this is a separate problem and 
we could tackle it later.

Archit

>
>>
>>>> shouldn't even write any shadow registers until the moment the
>>>> output is
>>>> enabled.
>>>
>>> That's being done correctly even now I guess, with the checks for
>>> mp->enabled in wrtie_regs() and set_go_bits().
>>
>> Yes, for timings. I was thinking more about the other settings done in
>> dpi.c currently, like dispc_mgr_set_pol_freq(). That writes directly to
>> registers, so we need runtime_get for that.
>
> Ok.
>
> Archit
>
>>
>> Tomi
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3927 bytes --]

On Wed, 2012-05-09 at 15:23 +0530, Archit Taneja wrote:
> On Tuesday 08 May 2012 06:05 PM, Archit Taneja wrote:
> > On Tuesday 08 May 2012 05:25 PM, Tomi Valkeinen wrote:
> >> On Tue, 2012-05-08 at 16:52 +0530, Archit Taneja wrote:
> >>> On Tuesday 08 May 2012 04:20 PM, Tomi Valkeinen wrote:
> >>
> >>>> Checking the validity of all the settings is a bit tricky, but
> >>>> currently
> >>>> I think, as a rule of thumb, we should accept any settings when things
> >>>> are disabled. So, until the interface driver sets the timings before
> >>>> enabling the output, all ovl/mgr settings should be allowed. And we
> >>>
> >>> We have 2 ways to go about this, one is to have an initial set of
> >>> 'always valid' values like I have done, the other option is to ignore
> >>> manager timing related checks if the manager is disabled, i.e all
> >>> configs are okay. To implement the second option, I think our function
> >>> dss_check_settings_low() would get more complicated. We would now have
> >>> to pass mp->enabled outside of apply, and pass it to dss_mgr_check()
> >>> which would further need to pass this to dss_ovl_check(). Hence I used
> >>> the first approach.
> >>
> >> I'm not sure about that. We already do it for overlay. In ovl.c we have
> >> dss_ovl_simple_check() and dss_ovl_check(). The simple check sees if the
> >> settings pass basic sanity check. The check sees if all the ovl& mgr
> >> settings are compatible with each other.
> >>
> >> Simple check is done when setting the config, called from
> >> dss_ovl_set_info(). The proper check is done later when the actual
> >> config is about to be taken into use.
> >>
> >> If mp->enabled == false, can't we just skip dss_check_settings_low()
> >> totally for that manager? We skip the check for ovls the same way.
> >
> > Okay, this would work better I guess. If someone tries to enable the
> > manager without setting the timings, that check should fail, and in my
> > approach, it would have passed, and would have led to bad stuff later.
> 
> One change in behaviour with the patch series I see is with the 
> following use case when connected to Pandaboad's DVI interface:
> 
> - Bootup with 1080p resolution
> - Try to set 640x480 timings with sysfs.
> 
> The older behaviour was that the timings were taken, and a skewed image 
> was seen(because the overlay size is larger in deimension)
> 
> The behaviour after this series is that the mgr_set_timings fails with 
> the error:
> 
> omapdss OVERLAY error: overlay 0 horizontally not inside the display 
> area (0 + 1920 >= 640)
> 
> I guess this is better in a way, a user of DSS is supposed to reallocate 
> the framebuffer with the desired size and then change the timings.

Yes, I think it's better. I think it's undefined what happens if the
overlay is larger than the output. In theory the DSS HW could lock-up,
or something. So we have to prevent it.

omapdss could change the size of the overlay automatically, but I
dislike things like that. Low level drivers adjusting the configuration
from the user almost always brings problems. So yes, I also think that
the user of DSS is supposed to handle it correctly.

> But with DVI, the problem is that dpi_set_timings already does a ton of 
> stuff before dss_mgr_set_timings() is called, so we change the dividers 
> to get the new clock, and then realise that the overlay can't fit 
> inside, and we are left nowhere. I guess this is a separate problem and 
> we could tackle it later.

Ah, ok. Yes, I guess we need to roll back the settings in that case. Or,
I wonder, would a check function work, that would take the new timings
and check those but also check if they match the overlays.

Hmm, no, that doesn't solve it totally, as the overlay could be changed
after the check. Again a problem with dss locking, we can't do the
operation atomically...

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v3 5/5] OMAPDSS: DPI/HDMI: Apply manager timings even if panel is disabled

[Archit Taneja]

The DPI and HDMI interfaces use their 'set_timing' functions to take in a new
set of timings. If the panel is disabled, they do not disable and re-enable
the interface. Currently, the manager timings are applied in hdmi_power_on()
and dpi_set_mode() respectively, these are not called by set_timings if the
panel is disabled.

When checking overlay and manager data, the DSS driver uses the last applied
manager timings, and not the timings held by omap_dss_device struct. Hence,
there is a need to apply the new manager timings even if the panel is disabled.

Apply the manager timings if the panel is disabled. Eventually, there should be
one common place where the timings are applied independent of the state of the
panel.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dpi.c  |    2 ++
 drivers/video/omap2/dss/hdmi.c |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index 8127f46..1650050 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -298,6 +298,8 @@ void dpi_set_timings(struct omap_dss_device *dssdev,
 
 		dispc_runtime_put();
 		dss_runtime_put();
+	} else {
+		dss_mgr_set_timings(dssdev->manager, timings);
 	}
 }
 EXPORT_SYMBOL(dpi_set_timings);
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 8d4ff8c..32ad712 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -435,6 +435,8 @@ void omapdss_hdmi_display_set_timing(struct omap_dss_device *dssdev)
 		r = hdmi_power_on(dssdev);
 		if (r)
 			DSSERR("failed to power on device\n");
+	} else {
+		dss_mgr_set_timings(dssdev->manager, &dssdev->panel.timings);
 	}
 }
 
-- 
1.7.5.4

[PATCH v4 0/9] OMAPDSS: APPLY: Treat overlay manager timings as shadow registers

[Archit Taneja]

An overlay manager's timings (the manager size, and blanking parameters if an
LCD manager) are DISPC shadow registers, and they should hence follow the
correct programming model.

This set makes the timings an extra_info parameter in manager's private data .
The interface drivers now apply the timings in instead of directly writing to
registers.

This change also prevents the need to use display resolution for overlay
checks, hence making some of the APPLY functions less dependent on the display.
Some DISPC functions that needed display width can also use these privately
stored timings.

Changes since v3:

- Remove direct setting of go bit in dpi_set_timings()
- Take some of the patches in "OMAPDSS: Misc fixes and cleanups" and make them a
  part of this series as they are more related.
- Don't have an initial set of manager timings in private data, only check
  manager and overlay parameters once the manager is enabled.

These patches apply over:

git://gitorious.org/linux-omap-dss2/linux.git dev

Reference tree containing this series:

git://gitorious.org/~boddob/linux-omap-dss2/archit-dss2-clone.git mgr_timing_and_fixes_2

Tested on OMAP4 SDP and Panda.

Archit Taneja (9):
  OMAPDSS: APPLY: Add manager timings as extra_info in private data
  OMAPDSS: Apply manager timings instead of direct DISPC writes
  OMAPDSS: MANAGER: Create a function to check manager timings
  OMAPDSS: APPLY: Don't check manager settings if it is disabled
  OMAPDSS: APPLY: Remove display dependency from overlay and manager
    checks
  OMAPDSS: DPI/HDMI: Apply manager timings even if panel is disabled
  OMAPDSS: APPLY: Remove an unnecessary omap_dss_device pointer
  OMAPDSS: DISPC: Remove omap_dss_device pointer usage from
    dispc_mgr_pclk_rate()
  OMAPDSS: DISPC: Remove usage of dispc_mgr_get_device()

 drivers/video/omap2/dss/apply.c   |  132 ++++++++++++++++++++++++++++---------
 drivers/video/omap2/dss/dispc.c   |   64 ++++++++----------
 drivers/video/omap2/dss/dpi.c     |    7 +-
 drivers/video/omap2/dss/dsi.c     |    5 +-
 drivers/video/omap2/dss/dss.h     |   16 +++--
 drivers/video/omap2/dss/hdmi.c    |    4 +-
 drivers/video/omap2/dss/manager.c |   19 +++++-
 drivers/video/omap2/dss/overlay.c |   10 +--
 drivers/video/omap2/dss/rfbi.c    |    4 +-
 drivers/video/omap2/dss/sdi.c     |    2 +-
 drivers/video/omap2/dss/venc.c    |    2 +-
 11 files changed, 175 insertions(+), 90 deletions(-)

-- 
1.7.5.4

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

[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 <archit@ti.com>
---
 drivers/video/omap2/dss/apply.c |   96 +++++++++++++++++++++++++++++++++-----
 drivers/video/omap2/dss/dss.h   |    2 +
 2 files changed, 85 insertions(+), 13 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index b10b3bc..57686f6 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)
 		}
 
 		dss_mgr_write_regs(mgr);
+		dss_mgr_write_regs_extra(mgr);
 	}
 }
 
@@ -693,6 +732,7 @@ static void mgr_clear_shadow_dirty(struct omap_overlay_manager *mgr)
 
 	mp = get_mgr_priv(mgr);
 	mp->shadow_info_dirty = false;
+	mp->shadow_extra_info_dirty = false;
 
 	list_for_each_entry(ovl, &mgr->overlays, list) {
 		op = get_ovl_priv(ovl);
@@ -719,6 +759,7 @@ void dss_mgr_start_update(struct omap_overlay_manager *mgr)
 	}
 
 	dss_mgr_write_regs(mgr);
+	dss_mgr_write_regs_extra(mgr);
 
 	dss_write_regs_common();
 
@@ -1225,6 +1266,35 @@ err:
 	return r;
 }
 
+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);
+
+	dss_write_regs();
+	dss_set_go_bits();
+
+	spin_unlock_irqrestore(&data_lock, flags);
+
+	wait_pending_extra_info_updates();
+
+	mutex_unlock(&apply_lock);
+}
 
 int dss_ovl_set_info(struct omap_overlay *ovl,
 		struct omap_overlay_info *info)
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 94d8234..b17a212 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -177,6 +177,8 @@ void dss_mgr_get_info(struct omap_overlay_manager *mgr,
 int dss_mgr_set_device(struct omap_overlay_manager *mgr,
 		struct omap_dss_device *dssdev);
 int dss_mgr_unset_device(struct omap_overlay_manager *mgr);
+void dss_mgr_set_timings(struct omap_overlay_manager *mgr,
+		struct omap_video_timings *timings);
 
 bool dss_ovl_is_enabled(struct omap_overlay *ovl);
 int dss_ovl_enable(struct omap_overlay *ovl);
-- 
1.7.5.4

[PATCH v4 2/9] OMAPDSS: Apply manager timings instead of direct DISPC writes

[Archit Taneja]

Replace the function dispc_mgr_set_timings() with dss_mgr_set_timings() in the
interface drivers. The latter function ensures that the timing related DISPC
registers are configured according to the shadow register programming model.

Remove the call to dispc_mgr_go() in dpi_set_timings() as the manager's go bit
is now set by dss_mgr_set_timings().

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dpi.c  |    3 +--
 drivers/video/omap2/dss/dsi.c  |    5 ++---
 drivers/video/omap2/dss/hdmi.c |    2 +-
 drivers/video/omap2/dss/rfbi.c |    4 ++--
 drivers/video/omap2/dss/sdi.c  |    2 +-
 drivers/video/omap2/dss/venc.c |    2 +-
 6 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index cec1166..e65cf1f 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -156,7 +156,7 @@ static int dpi_set_mode(struct omap_dss_device *dssdev)
 		t->pixel_clock = pck;
 	}
 
-	dispc_mgr_set_timings(dssdev->manager->id, t);
+	dss_mgr_set_timings(dssdev->manager, t);
 
 	return 0;
 }
@@ -294,7 +294,6 @@ void dpi_set_timings(struct omap_dss_device *dssdev,
 		}
 
 		dpi_set_mode(dssdev);
-		dispc_mgr_go(dssdev->manager->id);
 
 		dispc_runtime_put();
 		dss_runtime_put();
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 6c4b034..95bc996 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -4215,13 +4215,12 @@ static int dsi_display_init_dispc(struct omap_dss_device *dssdev)
 		dispc_mgr_enable_stallmode(dssdev->manager->id, true);
 		dispc_mgr_enable_fifohandcheck(dssdev->manager->id, 1);
 
-		dispc_mgr_set_timings(dssdev->manager->id, &timings);
+		dss_mgr_set_timings(dssdev->manager, &timings);
 	} else {
 		dispc_mgr_enable_stallmode(dssdev->manager->id, false);
 		dispc_mgr_enable_fifohandcheck(dssdev->manager->id, 0);
 
-		dispc_mgr_set_timings(dssdev->manager->id,
-			&dssdev->panel.timings);
+		dss_mgr_set_timings(dssdev->manager, &dssdev->panel.timings);
 	}
 
 		dispc_mgr_set_lcd_display_type(dssdev->manager->id,
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 56f6e9c..8d4ff8c 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -376,7 +376,7 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)
 	dispc_enable_gamma_table(0);
 
 	/* tv size */
-	dispc_mgr_set_timings(dssdev->manager->id, &dssdev->panel.timings);
+	dss_mgr_set_timings(dssdev->manager, &dssdev->panel.timings);
 
 	hdmi.ip_data.ops->video_enable(&hdmi.ip_data, 1);
 
diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
index a81ffcb..feadfab 100644
--- a/drivers/video/omap2/dss/rfbi.c
+++ b/drivers/video/omap2/dss/rfbi.c
@@ -320,7 +320,7 @@ static void rfbi_transfer_area(struct omap_dss_device *dssdev, u16 width,
 
 	DSSDBG("rfbi_transfer_area %dx%d\n", width, height);
 
-	dispc_mgr_set_timings(dssdev->manager->id, &timings);
+	dss_mgr_set_timings(dssdev->manager, &timings);
 
 	dispc_mgr_enable(dssdev->manager->id, true);
 
@@ -804,7 +804,7 @@ int omap_rfbi_prepare_update(struct omap_dss_device *dssdev,
 	if (*w = 0 || *h = 0)
 		return -EINVAL;
 
-	dispc_mgr_set_timings(dssdev->manager->id, &timings);
+	dss_mgr_set_timings(dssdev->manager, &timings);
 
 	return 0;
 }
diff --git a/drivers/video/omap2/dss/sdi.c b/drivers/video/omap2/dss/sdi.c
index 741b834..67fbe7c 100644
--- a/drivers/video/omap2/dss/sdi.c
+++ b/drivers/video/omap2/dss/sdi.c
@@ -107,7 +107,7 @@ int omapdss_sdi_display_enable(struct omap_dss_device *dssdev)
 	}
 
 
-	dispc_mgr_set_timings(dssdev->manager->id, t);
+	dss_mgr_set_timings(dssdev->manager, t);
 
 	r = dss_set_clock_div(&dss_cinfo);
 	if (r)
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 6947f5b..9475e6e 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -444,7 +444,7 @@ static int venc_power_on(struct omap_dss_device *dssdev)
 	timings = dssdev->panel.timings;
 	timings.y_res /= 2;
 
-	dispc_mgr_set_timings(dssdev->manager->id, &timings);
+	dss_mgr_set_timings(dssdev->manager, &timings);
 
 	r = regulator_enable(venc.vdda_dac_reg);
 	if (r)
-- 
1.7.5.4

[PATCH v4 3/9] OMAPDSS: MANAGER: Create a function to check manager timings

[Archit Taneja]

Create a function dss_mgr_check_timings() which wraps around the function
dispc_mgr_timings_ok(). This is mainly a clean up to hide dispc functions
from interface drivers.

dss_mgr_check_timings() is added in the function dss_mgr_check(), it currently
takes the timings maintained in the omap_dss_device struct. This would be later
replaced by the timings stored in the manager's private data.

Make dss_mgr_check_timings() and dispc_mgr_timings_ok() take a const
omap_video_timings pointer since these functions just check the timings.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dispc.c   |    2 +-
 drivers/video/omap2/dss/dpi.c     |    2 +-
 drivers/video/omap2/dss/dss.h     |    4 +++-
 drivers/video/omap2/dss/manager.c |   15 +++++++++++++++
 4 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 82012d1..6eec084 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -2479,7 +2479,7 @@ static bool _dispc_lcd_timings_ok(int hsw, int hfp, int hbp,
 }
 
 bool dispc_mgr_timings_ok(enum omap_channel channel,
-		struct omap_video_timings *timings)
+		const struct omap_video_timings *timings)
 {
 	bool timings_ok;
 
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index e65cf1f..e01ab98 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -311,7 +311,7 @@ int dpi_check_timings(struct omap_dss_device *dssdev,
 	unsigned long pck;
 	struct dispc_clock_info dispc_cinfo;
 
-	if (!dispc_mgr_timings_ok(dssdev->manager->id, timings))
+	if (dss_mgr_check_timings(dssdev->manager, timings))
 		return -EINVAL;
 
 	if (timings->pixel_clock = 0)
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index b17a212..a13b305 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -208,6 +208,8 @@ int dss_init_overlay_managers(struct platform_device *pdev);
 void dss_uninit_overlay_managers(struct platform_device *pdev);
 int dss_mgr_simple_check(struct omap_overlay_manager *mgr,
 		const struct omap_overlay_manager_info *info);
+int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
+		const struct omap_video_timings *timings);
 int dss_mgr_check(struct omap_overlay_manager *mgr,
 		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
@@ -413,7 +415,7 @@ void dispc_enable_gamma_table(bool enable);
 void dispc_set_loadmode(enum omap_dss_load_mode mode);
 
 bool dispc_mgr_timings_ok(enum omap_channel channel,
-		struct omap_video_timings *timings);
+		const struct omap_video_timings *timings);
 unsigned long dispc_fclk_rate(void);
 void dispc_find_clk_divs(bool is_tft, unsigned long req_pck, unsigned long fck,
 		struct dispc_clock_info *cinfo);
diff --git a/drivers/video/omap2/dss/manager.c b/drivers/video/omap2/dss/manager.c
index e736460..566fbba 100644
--- a/drivers/video/omap2/dss/manager.c
+++ b/drivers/video/omap2/dss/manager.c
@@ -654,6 +654,17 @@ static int dss_mgr_check_zorder(struct omap_overlay_manager *mgr,
 	return 0;
 }
 
+int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
+		const struct omap_video_timings *timings)
+{
+	if (!dispc_mgr_timings_ok(mgr->id, timings)) {
+		DSSERR("check_manager: invalid timings\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int dss_mgr_check(struct omap_overlay_manager *mgr,
 		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
@@ -668,6 +679,10 @@ int dss_mgr_check(struct omap_overlay_manager *mgr,
 			return r;
 	}
 
+	r = dss_mgr_check_timings(mgr, &dssdev->panel.timings);
+	if (r)
+		return r;
+
 	list_for_each_entry(ovl, &mgr->overlays, list) {
 		struct omap_overlay_info *oi;
 		int r;
-- 
1.7.5.4

[PATCH v4 4/9] OMAPDSS: APPLY: Don't check manager settings if it is disabled

[Archit Taneja]

If a manager is disabled, there is no guarantee at any point in time that all
it's parameters are configured. There is always a chance that some more
parameters are yet to be configured by a user of DSS, or by DSS itself.

However, when the manager is enabled, we can be certain that all the parameters
have been configured, as we can't enable a manager with an incomplete
configuration. Therefore, if a manager is disabled, don't check for the validity
of it's parameters or the parameters of the overlays connected to it. Only check
once it is enabled. Add a check in dss_check_settings_low() to achieve the same.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/apply.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 57686f6..ad349b3 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -192,6 +192,9 @@ static int dss_check_settings_low(struct omap_overlay_manager *mgr,
 
 	mp = get_mgr_priv(mgr);
 
+	if (!mp->enabled)
+		return 0;
+
 	if (applying && mp->user_info_dirty)
 		mi = &mp->user_info;
 	else
-- 
1.7.5.4

[PATCH v4 5/9] OMAPDSS: APPLY: Remove display dependency from overlay and manager checks

[Archit Taneja]

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.

Pass the manager's timings in private data to dss_mgr_check(). Remove the need
to pass omap_dss_device structs in the functions which check for the validity
of overlay and manager parameters.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/apply.c   |   24 +++++++++++-------------
 drivers/video/omap2/dss/dss.h     |    7 ++++---
 drivers/video/omap2/dss/manager.c |    6 +++---
 drivers/video/omap2/dss/overlay.c |   10 ++++------
 4 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index ad349b3..0ffe4e1 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -181,7 +181,7 @@ static bool mgr_manual_update(struct omap_overlay_manager *mgr)
 }
 
 static int dss_check_settings_low(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev, bool applying)
+		bool applying)
 {
 	struct omap_overlay_info *oi;
 	struct omap_overlay_manager_info *mi;
@@ -214,26 +214,24 @@ static int dss_check_settings_low(struct omap_overlay_manager *mgr,
 		ois[ovl->id] = oi;
 	}
 
-	return dss_mgr_check(mgr, dssdev, mi, ois);
+	return dss_mgr_check(mgr, mi, &mp->timings, ois);
 }
 
 /*
  * check manager and overlay settings using overlay_info from data->info
  */
-static int dss_check_settings(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev)
+static int dss_check_settings(struct omap_overlay_manager *mgr)
 {
-	return dss_check_settings_low(mgr, dssdev, false);
+	return dss_check_settings_low(mgr, false);
 }
 
 /*
  * check manager and overlay settings using overlay_info from ovl->info if
  * dirty and from data->info otherwise
  */
-static int dss_check_settings_apply(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev)
+static int dss_check_settings_apply(struct omap_overlay_manager *mgr)
 {
-	return dss_check_settings_low(mgr, dssdev, true);
+	return dss_check_settings_low(mgr, true);
 }
 
 static bool need_isr(void)
@@ -687,7 +685,7 @@ static void dss_write_regs(void)
 		if (!mp->enabled || mgr_manual_update(mgr) || mp->busy)
 			continue;
 
-		r = dss_check_settings(mgr, mgr->device);
+		r = dss_check_settings(mgr);
 		if (r) {
 			DSSERR("cannot write registers for manager %s: "
 					"illegal configuration\n", mgr->name);
@@ -754,7 +752,7 @@ void dss_mgr_start_update(struct omap_overlay_manager *mgr)
 
 	WARN_ON(mp->updating);
 
-	r = dss_check_settings(mgr, mgr->device);
+	r = dss_check_settings(mgr);
 	if (r) {
 		DSSERR("cannot start manual update: illegal configuration\n");
 		spin_unlock_irqrestore(&data_lock, flags);
@@ -901,7 +899,7 @@ int omap_dss_mgr_apply(struct omap_overlay_manager *mgr)
 
 	spin_lock_irqsave(&data_lock, flags);
 
-	r = dss_check_settings_apply(mgr, mgr->device);
+	r = dss_check_settings_apply(mgr);
 	if (r) {
 		spin_unlock_irqrestore(&data_lock, flags);
 		DSSERR("failed to apply settings: illegal configuration.\n");
@@ -1094,7 +1092,7 @@ int dss_mgr_enable(struct omap_overlay_manager *mgr)
 
 	mp->enabled = true;
 
-	r = dss_check_settings(mgr, mgr->device);
+	r = dss_check_settings(mgr);
 	if (r) {
 		DSSERR("failed to enable manager %d: check_settings failed\n",
 				mgr->id);
@@ -1466,7 +1464,7 @@ int dss_ovl_enable(struct omap_overlay *ovl)
 
 	op->enabling = true;
 
-	r = dss_check_settings(ovl->manager, ovl->manager->device);
+	r = dss_check_settings(ovl->manager);
 	if (r) {
 		DSSERR("failed to enable overlay %d: check_settings failed\n",
 				ovl->id);
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index a13b305..f3fa445 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -179,6 +179,7 @@ int dss_mgr_set_device(struct omap_overlay_manager *mgr,
 int dss_mgr_unset_device(struct omap_overlay_manager *mgr);
 void dss_mgr_set_timings(struct omap_overlay_manager *mgr,
 		struct omap_video_timings *timings);
+const struct omap_video_timings *dss_mgr_get_timings(struct omap_overlay_manager *mgr);
 
 bool dss_ovl_is_enabled(struct omap_overlay *ovl);
 int dss_ovl_enable(struct omap_overlay *ovl);
@@ -211,8 +212,8 @@ int dss_mgr_simple_check(struct omap_overlay_manager *mgr,
 int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
 		const struct omap_video_timings *timings);
 int dss_mgr_check(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
+		const struct omap_video_timings *mgr_timings,
 		struct omap_overlay_info **overlay_infos);
 
 /* overlay */
@@ -222,8 +223,8 @@ void dss_overlay_setup_dispc_manager(struct omap_overlay_manager *mgr);
 void dss_recheck_connections(struct omap_dss_device *dssdev, bool force);
 int dss_ovl_simple_check(struct omap_overlay *ovl,
 		const struct omap_overlay_info *info);
-int dss_ovl_check(struct omap_overlay *ovl,
-		struct omap_overlay_info *info, struct omap_dss_device *dssdev);
+int dss_ovl_check(struct omap_overlay *ovl, struct omap_overlay_info *info,
+		const struct omap_video_timings *mgr_timings);
 
 /* DSS */
 int dss_init_platform_driver(void);
diff --git a/drivers/video/omap2/dss/manager.c b/drivers/video/omap2/dss/manager.c
index 566fbba..0cbcde4 100644
--- a/drivers/video/omap2/dss/manager.c
+++ b/drivers/video/omap2/dss/manager.c
@@ -666,8 +666,8 @@ int dss_mgr_check_timings(struct omap_overlay_manager *mgr,
 }
 
 int dss_mgr_check(struct omap_overlay_manager *mgr,
-		struct omap_dss_device *dssdev,
 		struct omap_overlay_manager_info *info,
+		const struct omap_video_timings *mgr_timings,
 		struct omap_overlay_info **overlay_infos)
 {
 	struct omap_overlay *ovl;
@@ -679,7 +679,7 @@ int dss_mgr_check(struct omap_overlay_manager *mgr,
 			return r;
 	}
 
-	r = dss_mgr_check_timings(mgr, &dssdev->panel.timings);
+	r = dss_mgr_check_timings(mgr, mgr_timings);
 	if (r)
 		return r;
 
@@ -692,7 +692,7 @@ int dss_mgr_check(struct omap_overlay_manager *mgr,
 		if (oi = NULL)
 			continue;
 
-		r = dss_ovl_check(ovl, oi, dssdev);
+		r = dss_ovl_check(ovl, oi, mgr_timings);
 		if (r)
 			return r;
 	}
diff --git a/drivers/video/omap2/dss/overlay.c b/drivers/video/omap2/dss/overlay.c
index 6e82181..0da5eb6 100644
--- a/drivers/video/omap2/dss/overlay.c
+++ b/drivers/video/omap2/dss/overlay.c
@@ -631,16 +631,14 @@ int dss_ovl_simple_check(struct omap_overlay *ovl,
 	return 0;
 }
 
-int dss_ovl_check(struct omap_overlay *ovl,
-		struct omap_overlay_info *info, struct omap_dss_device *dssdev)
+int dss_ovl_check(struct omap_overlay *ovl, struct omap_overlay_info *info,
+		const struct omap_video_timings *mgr_timings)
 {
 	u16 outw, outh;
 	u16 dw, dh;
 
-	if (dssdev = NULL)
-		return 0;
-
-	dssdev->driver->get_resolution(dssdev, &dw, &dh);
+	dw = mgr_timings->x_res;
+	dh = mgr_timings->y_res;
 
 	if ((ovl->caps & OMAP_DSS_OVL_CAP_SCALE) = 0) {
 		outw = info->width;
-- 
1.7.5.4

[PATCH v4 6/9] OMAPDSS: DPI/HDMI: Apply manager timings even if panel is disabled

[Archit Taneja]

The DPI and HDMI interfaces use their 'set_timing' functions to take in a new
set of timings. If the panel is disabled, they do not disable and re-enable
the interface. Currently, the manager timings are applied in hdmi_power_on()
and dpi_set_mode() respectively, these are not called by set_timings if the
panel is disabled.

When checking overlay and manager data, the DSS driver uses the last applied
manager timings, and not the timings held by omap_dss_device struct. Hence,
there is a need to apply the new manager timings even if the panel is disabled.

Apply the manager timings if the panel is disabled. Eventually, there should be
one common place where the timings are applied independent of the state of the
panel.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dpi.c  |    2 ++
 drivers/video/omap2/dss/hdmi.c |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index e01ab98..d6e8fe7 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -297,6 +297,8 @@ void dpi_set_timings(struct omap_dss_device *dssdev,
 
 		dispc_runtime_put();
 		dss_runtime_put();
+	} else {
+		dss_mgr_set_timings(dssdev->manager, timings);
 	}
 }
 EXPORT_SYMBOL(dpi_set_timings);
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 8d4ff8c..32ad712 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -435,6 +435,8 @@ void omapdss_hdmi_display_set_timing(struct omap_dss_device *dssdev)
 		r = hdmi_power_on(dssdev);
 		if (r)
 			DSSERR("failed to power on device\n");
+	} else {
+		dss_mgr_set_timings(dssdev->manager, &dssdev->panel.timings);
 	}
 }
 
-- 
1.7.5.4

[PATCH v4 7/9] OMAPDSS: APPLY: Remove an unnecessary omap_dss_device pointer

[Archit Taneja]

The omap_dss_device pointer declared in dss_ovl_setup_fifo() isn't used. Remove
the pointer variable declaration and it's assignment.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/apply.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 0ffe4e1..15c070c 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -960,14 +960,11 @@ static void dss_ovl_setup_fifo(struct omap_overlay *ovl,
 		bool use_fifo_merge)
 {
 	struct ovl_priv_data *op = get_ovl_priv(ovl);
-	struct omap_dss_device *dssdev;
 	u32 fifo_low, fifo_high;
 
 	if (!op->enabled && !op->enabling)
 		return;
 
-	dssdev = ovl->manager->device;
-
 	dispc_ovl_compute_fifo_thresholds(ovl->id, &fifo_low, &fifo_high,
 			use_fifo_merge);
 
-- 
1.7.5.4

[PATCH v4 8/9] OMAPDSS: DISPC: Remove omap_dss_device pointer usage from dispc_mgr_pclk_rate()

[Archit Taneja]

The pixel clock rate for the TV manager is calculated by checking the device
type connected to the manager, and then requesting the VENC/HDMI interface for
the pixel clock rate.

Remove the use of omap_dss_device pointer from here by checking which interface
generates the pixel clock by reading the DSS_CTRL.VENC_HDMI_SWITCH bit.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dispc.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 6eec084..cd0a397 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -2643,13 +2643,14 @@ unsigned long dispc_mgr_pclk_rate(enum omap_channel channel)
 
 		return r / pcd;
 	} else {
-		struct omap_dss_device *dssdev -			dispc_mgr_get_device(channel);
+		enum dss_hdmi_venc_clk_source_select source;
 
-		switch (dssdev->type) {
-		case OMAP_DISPLAY_TYPE_VENC:
+		source = dss_get_hdmi_venc_clk_source();
+
+		switch (source) {
+		case DSS_VENC_TV_CLK:
 			return venc_get_pixel_clock();
-		case OMAP_DISPLAY_TYPE_HDMI:
+		case DSS_HDMI_M_PCLK:
 			return hdmi_get_pixel_clock();
 		default:
 			BUG();
-- 
1.7.5.4

[PATCH v4 9/9] OMAPDSS: DISPC: Remove usage of dispc_mgr_get_device()

[Archit Taneja]

The functions calc_fclk_five_taps() and check_horiz_timing_omap3() use the
function dispc_mgr_get_device() to get the omap_dss_device pointer to which
the manager is connected, the width of the panel is derived from it.

The manager's timing is stored in it's private data in APPLY. This contains
the latest timings applied to the manager. Pass these timings to
dispc_ovl_setup() and use them in the above functions. Remove the function
dispc_mgr_get_device() as it isn't used any more.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/apply.c |    6 ++--
 drivers/video/omap2/dss/dispc.c |   51 +++++++++++++++++---------------------
 drivers/video/omap2/dss/dss.h   |    3 +-
 3 files changed, 28 insertions(+), 32 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 15c070c..dd88b8f 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -548,11 +548,13 @@ static void dss_ovl_write_regs(struct omap_overlay *ovl)
 
 	oi = &op->info;
 
+	mp = get_mgr_priv(ovl->manager);
+
 	replication = dss_use_replication(ovl->manager->device, oi->color_mode);
 
 	ilace = ovl->manager->device->type = OMAP_DISPLAY_TYPE_VENC;
 
-	r = dispc_ovl_setup(ovl->id, oi, ilace, replication);
+	r = dispc_ovl_setup(ovl->id, oi, ilace, replication, &mp->timings);
 	if (r) {
 		/*
 		 * We can't do much here, as this function can be called from
@@ -566,8 +568,6 @@ static void dss_ovl_write_regs(struct omap_overlay *ovl)
 		return;
 	}
 
-	mp = get_mgr_priv(ovl->manager);
-
 	op->info_dirty = false;
 	if (mp->updating)
 		op->shadow_info_dirty = true;
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index cd0a397..727e15b 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -413,14 +413,6 @@ static inline bool dispc_mgr_is_lcd(enum omap_channel channel)
 		return false;
 }
 
-static struct omap_dss_device *dispc_mgr_get_device(enum omap_channel channel)
-{
-	struct omap_overlay_manager *mgr -		omap_dss_get_overlay_manager(channel);
-
-	return mgr ? mgr->device : NULL;
-}
-
 u32 dispc_mgr_get_vsync_irq(enum omap_channel channel)
 {
 	switch (channel) {
@@ -1661,18 +1653,17 @@ static void calc_dma_rotation_offset(u8 rotation, bool mirror,
  * This function is used to avoid synclosts in OMAP3, because of some
  * undocumented horizontal position and timing related limitations.
  */
-static int check_horiz_timing_omap3(enum omap_channel channel, u16 pos_x,
+static int check_horiz_timing_omap3(enum omap_channel channel,
+		const struct omap_video_timings *t, u16 pos_x,
 		u16 width, u16 height, u16 out_width, u16 out_height)
 {
 	int DS = DIV_ROUND_UP(height, out_height);
-	struct omap_dss_device *dssdev = dispc_mgr_get_device(channel);
-	struct omap_video_timings t = dssdev->panel.timings;
 	unsigned long nonactive, lclk, pclk;
 	static const u8 limits[3] = { 8, 10, 20 };
 	u64 val, blank;
 	int i;
 
-	nonactive = t.x_res + t.hfp + t.hsw + t.hbp - out_width;
+	nonactive = t->x_res + t->hfp + t->hsw + t->hbp - out_width;
 	pclk = dispc_mgr_pclk_rate(channel);
 	if (dispc_mgr_is_lcd(channel))
 		lclk = dispc_mgr_lclk_rate(channel);
@@ -1684,7 +1675,7 @@ static int check_horiz_timing_omap3(enum omap_channel channel, u16 pos_x,
 		i++;
 	if (out_width < width)
 		i++;
-	blank = div_u64((u64)(t.hbp + t.hsw + t.hfp) * lclk, pclk);
+	blank = div_u64((u64)(t->hbp + t->hsw + t->hfp) * lclk, pclk);
 	DSSDBG("blanking period + ppl = %llu (limit = %u)\n", blank, limits[i]);
 	if (blank <= limits[i])
 		return -EINVAL;
@@ -1715,7 +1706,8 @@ static int check_horiz_timing_omap3(enum omap_channel channel, u16 pos_x,
 }
 
 static unsigned long calc_core_clk_five_taps(enum omap_channel channel,
-		u16 width, u16 height, u16 out_width, u16 out_height,
+		const struct omap_video_timings *mgr_timings, u16 width,
+		u16 height, u16 out_width, u16 out_height,
 		enum omap_color_mode color_mode)
 {
 	u32 core_clk = 0;
@@ -1725,8 +1717,7 @@ static unsigned long calc_core_clk_five_taps(enum omap_channel channel,
 		return (unsigned long) pclk;
 
 	if (height > out_height) {
-		struct omap_dss_device *dssdev = dispc_mgr_get_device(channel);
-		unsigned int ppl = dssdev->panel.timings.x_res;
+		unsigned int ppl = mgr_timings->x_res;
 
 		tmp = pclk * height * out_width;
 		do_div(tmp, 2 * out_height * ppl);
@@ -1795,8 +1786,9 @@ static unsigned long calc_core_clk(enum omap_channel channel, u16 width,
 }
 
 static int dispc_ovl_calc_scaling(enum omap_plane plane,
-		enum omap_channel channel, u16 width, u16 height,
-		u16 out_width, u16 out_height,
+		enum omap_channel channel,
+		const struct omap_video_timings *mgr_timings,
+		u16 width, u16 height, u16 out_width, u16 out_height,
 		enum omap_color_mode color_mode, bool *five_taps,
 		int *x_predecim, int *y_predecim, u16 pos_x)
 {
@@ -1871,11 +1863,13 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
 		do {
 			in_height = DIV_ROUND_UP(height, decim_y);
 			in_width = DIV_ROUND_UP(width, decim_x);
-			core_clk = calc_core_clk_five_taps(channel, in_width,
-				in_height, out_width, out_height, color_mode);
+			core_clk = calc_core_clk_five_taps(channel, mgr_timings,
+				in_width, in_height, out_width, out_height,
+				color_mode);
 
-			error = check_horiz_timing_omap3(channel, pos_x,
-				in_width, in_height, out_width, out_height);
+			error = check_horiz_timing_omap3(channel, mgr_timings,
+				pos_x, in_width, in_height, out_width,
+				out_height);
 
 			if (in_width > maxsinglelinewidth)
 				if (in_height > out_height &&
@@ -1900,8 +1894,8 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
 		} while (decim_x <= *x_predecim && decim_y <= *y_predecim
 			&& error);
 
-		if (check_horiz_timing_omap3(channel, pos_x, width, height,
-			out_width, out_height)){
+		if (check_horiz_timing_omap3(channel, mgr_timings, pos_x, width,
+			height, out_width, out_height)){
 				DSSERR("horizontal timing too tight\n");
 				return -EINVAL;
 		}
@@ -1959,7 +1953,8 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
 }
 
 int dispc_ovl_setup(enum omap_plane plane, struct omap_overlay_info *oi,
-		bool ilace, bool replication)
+		bool ilace, bool replication,
+		const struct omap_video_timings *mgr_timings)
 {
 	struct omap_overlay *ovl = omap_dss_get_overlay(plane);
 	bool five_taps = true;
@@ -2008,9 +2003,9 @@ int dispc_ovl_setup(enum omap_plane plane, struct omap_overlay_info *oi,
 	if (!dss_feat_color_mode_supported(plane, oi->color_mode))
 		return -EINVAL;
 
-	r = dispc_ovl_calc_scaling(plane, channel, in_width, in_height,
-			out_width, out_height, oi->color_mode, &five_taps,
-			&x_predecim, &y_predecim, oi->pos_x);
+	r = dispc_ovl_calc_scaling(plane, channel, mgr_timings, in_width,
+			in_height, out_width, out_height, oi->color_mode,
+			&five_taps, &x_predecim, &y_predecim, oi->pos_x);
 	if (r)
 		return r;
 
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index f3fa445..8e9e9a5 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -428,7 +428,8 @@ void dispc_ovl_set_fifo_threshold(enum omap_plane plane, u32 low, u32 high);
 void dispc_ovl_compute_fifo_thresholds(enum omap_plane plane,
 		u32 *fifo_low, u32 *fifo_high, bool use_fifomerge);
 int dispc_ovl_setup(enum omap_plane plane, struct omap_overlay_info *oi,
-		bool ilace, bool replication);
+		bool ilace, bool replication,
+		const struct omap_video_timings *mgr_timings);
 int dispc_ovl_enable(enum omap_plane plane, bool enable);
 void dispc_ovl_set_channel_out(enum omap_plane plane,
 		enum omap_channel channel);
-- 
1.7.5.4

Re: [PATCH v4 0/9] OMAPDSS: APPLY: Treat overlay manager timings as shadow registers

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1422 bytes --]

On Wed, 2012-05-09 at 15:40 +0530, Archit Taneja wrote:
> An overlay manager's timings (the manager size, and blanking parameters if an
> LCD manager) are DISPC shadow registers, and they should hence follow the
> correct programming model.
> 
> This set makes the timings an extra_info parameter in manager's private data .
> The interface drivers now apply the timings in instead of directly writing to
> registers.
> 
> This change also prevents the need to use display resolution for overlay
> checks, hence making some of the APPLY functions less dependent on the display.
> Some DISPC functions that needed display width can also use these privately
> stored timings.
> 
> Changes since v3:
> 
> - Remove direct setting of go bit in dpi_set_timings()
> - Take some of the patches in "OMAPDSS: Misc fixes and cleanups" and make them a
>   part of this series as they are more related.
> - Don't have an initial set of manager timings in private data, only check
>   manager and overlay parameters once the manager is enabled.
> 
> These patches apply over:
> 
> git://gitorious.org/linux-omap-dss2/linux.git dev
> 
> Reference tree containing this series:
> 
> git://gitorious.org/~boddob/linux-omap-dss2/archit-dss2-clone.git mgr_timing_and_fixes_2
> 
> Tested on OMAP4 SDP and Panda.

I did some quick tests on omap3 overo, worked fine. I'll merge to dss
master branch.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v4 0/9] OMAPDSS: APPLY: Treat overlay manager timings as shadow registers

[Archit Taneja]

On Wednesday 09 May 2012 04:43 PM, Tomi Valkeinen wrote:
> On Wed, 2012-05-09 at 15:40 +0530, Archit Taneja wrote:
>> An overlay manager's timings (the manager size, and blanking parameters if an
>> LCD manager) are DISPC shadow registers, and they should hence follow the
>> correct programming model.
>>
>> This set makes the timings an extra_info parameter in manager's private data .
>> The interface drivers now apply the timings in instead of directly writing to
>> registers.
>>
>> This change also prevents the need to use display resolution for overlay
>> checks, hence making some of the APPLY functions less dependent on the display.
>> Some DISPC functions that needed display width can also use these privately
>> stored timings.
>>
>> Changes since v3:
>>
>> - Remove direct setting of go bit in dpi_set_timings()
>> - Take some of the patches in "OMAPDSS: Misc fixes and cleanups" and make them a
>>    part of this series as they are more related.
>> - Don't have an initial set of manager timings in private data, only check
>>    manager and overlay parameters once the manager is enabled.
>>
>> These patches apply over:
>>
>> git://gitorious.org/linux-omap-dss2/linux.git dev
>>
>> Reference tree containing this series:
>>
>> git://gitorious.org/~boddob/linux-omap-dss2/archit-dss2-clone.git mgr_timing_and_fixes_2
>>
>> Tested on OMAP4 SDP and Panda.
>
> I did some quick tests on omap3 overo, worked fine. I'll merge to dss
> master branch.

Russ did some tests on more omap3 boards, the tree which he used has 
some slight changes comapred to what I have posted, but it's more or 
less the same.

Could you add 'Tested-by: Russ.Dill@ti.com' if not too difficult, I 
missed out doing that.

Thanks,
Archit


>
>   Tomi
>

Re: [PATCH v4 0/9] OMAPDSS: APPLY: Treat overlay manager timings as shadow registers

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2001 bytes --]

On Wed, 2012-05-09 at 16:54 +0530, Archit Taneja wrote:
> On Wednesday 09 May 2012 04:43 PM, Tomi Valkeinen wrote:
> > On Wed, 2012-05-09 at 15:40 +0530, Archit Taneja wrote:
> >> An overlay manager's timings (the manager size, and blanking parameters if an
> >> LCD manager) are DISPC shadow registers, and they should hence follow the
> >> correct programming model.
> >>
> >> This set makes the timings an extra_info parameter in manager's private data .
> >> The interface drivers now apply the timings in instead of directly writing to
> >> registers.
> >>
> >> This change also prevents the need to use display resolution for overlay
> >> checks, hence making some of the APPLY functions less dependent on the display.
> >> Some DISPC functions that needed display width can also use these privately
> >> stored timings.
> >>
> >> Changes since v3:
> >>
> >> - Remove direct setting of go bit in dpi_set_timings()
> >> - Take some of the patches in "OMAPDSS: Misc fixes and cleanups" and make them a
> >>    part of this series as they are more related.
> >> - Don't have an initial set of manager timings in private data, only check
> >>    manager and overlay parameters once the manager is enabled.
> >>
> >> These patches apply over:
> >>
> >> git://gitorious.org/linux-omap-dss2/linux.git dev
> >>
> >> Reference tree containing this series:
> >>
> >> git://gitorious.org/~boddob/linux-omap-dss2/archit-dss2-clone.git mgr_timing_and_fixes_2
> >>
> >> Tested on OMAP4 SDP and Panda.
> >
> > I did some quick tests on omap3 overo, worked fine. I'll merge to dss
> > master branch.
> 
> Russ did some tests on more omap3 boards, the tree which he used has 
> some slight changes comapred to what I have posted, but it's more or 
> less the same.
> 
> Could you add 'Tested-by: Russ.Dill@ti.com' if not too difficult, I 
> missed out doing that.

Sorry, I could, but I already merged and pushed the series, so I can't
change it anymore.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]