[PATCH 0/4] drm/omapdrm: gamma table support

[PATCH 0/4] drm/omapdrm: gamma table support

[Jyri Sarha]

Implements gamma tables for OMAP4, OMAP5, and dra7xx SoCs and adds a
work-a-round for errata that may break LCD1 channel if gamma tables
are in use.

The first patch ("drm/omapdrm: omap_modeset_init: Separate crtc id and
plane id indexing") is not really a part of the series and can be
applied or dropped independently. I just needed that in order to
reproduce errata i734 conditions.

Jyri Sarha (4):
  drm/omapdrm: omap_modeset_init: Separate crtc id and plane id indexing
  drm/omapdrm: Add gamma table support to DSS dispc
  drm/omapdrm: Work-a-round for errata i734 (LCD1 Gamma) in DSS dispc
  drm/omapdrm: Implement gamma_lut atomic crtc property

 drivers/gpu/drm/omapdrm/dss/dispc.c        | 355 +++++++++++++++++++++++++++--
 drivers/gpu/drm/omapdrm/dss/dispc.h        |   5 +
 drivers/gpu/drm/omapdrm/dss/dss_features.c |   4 +
 drivers/gpu/drm/omapdrm/dss/dss_features.h |   2 +
 drivers/gpu/drm/omapdrm/dss/hdmi4.c        |   3 -
 drivers/gpu/drm/omapdrm/dss/hdmi5.c        |   3 -
 drivers/gpu/drm/omapdrm/dss/omapdss.h      |   5 +
 drivers/gpu/drm/omapdrm/omap_crtc.c        |  20 ++
 drivers/gpu/drm/omapdrm/omap_drv.c         |  31 +--
 9 files changed, 392 insertions(+), 36 deletions(-)

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] drm/omapdrm: omap_modeset_init: Separate crtc id and plane id indexing

[Jyri Sarha]

Separate crtc id and plane id indexing in omap_modeset_init(). The
coupling of crtc- and plane-id is hard to follow.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index fe79498..8d0fe45 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -264,24 +264,24 @@ cleanup:
 	return r;
 }
 
-static int omap_modeset_create_crtc(struct drm_device *dev, int id,
-				    enum omap_channel channel)
+static int omap_modeset_create_crtc(struct drm_device *dev, int crtc_id,
+				    int plane_id, enum omap_channel channel)
 {
 	struct omap_drm_private *priv = dev->dev_private;
 	struct drm_plane *plane;
 	struct drm_crtc *crtc;
 
-	plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_PRIMARY);
+	plane = omap_plane_init(dev, plane_id, DRM_PLANE_TYPE_PRIMARY);
 	if (IS_ERR(plane))
 		return PTR_ERR(plane);
 
-	crtc = omap_crtc_init(dev, plane, channel, id);
+	crtc = omap_crtc_init(dev, plane, channel, crtc_id);
 
 	BUG_ON(priv->num_crtcs >= ARRAY_SIZE(priv->crtcs));
-	priv->crtcs[id] = crtc;
+	priv->crtcs[crtc_id] = crtc;
 	priv->num_crtcs++;
 
-	priv->planes[id] = plane;
+	priv->planes[plane_id] = plane;
 	priv->num_planes++;
 
 	return 0;
@@ -315,7 +315,7 @@ static int omap_modeset_init(struct drm_device *dev)
 	int num_ovls = dss_feat_get_num_ovls();
 	int num_mgrs = dss_feat_get_num_mgrs();
 	int num_crtcs;
-	int i, id = 0;
+	int i, crtc_id = 0, plane_id = 0;
 	int ret;
 
 	drm_mode_config_init(dev);
@@ -377,7 +377,7 @@ static int omap_modeset_init(struct drm_device *dev)
 		 * the possible_crtcs field for all the encoders with the final
 		 * set of crtcs we create
 		 */
-		if (id == num_crtcs)
+		if (crtc_id == num_crtcs)
 			continue;
 
 		/*
@@ -396,7 +396,8 @@ static int omap_modeset_init(struct drm_device *dev)
 		 * allocated crtc, we create a new crtc for it
 		 */
 		if (!channel_used(dev, channel)) {
-			ret = omap_modeset_create_crtc(dev, id, channel);
+			ret = omap_modeset_create_crtc(dev, crtc_id, plane_id,
+						       channel);
 			if (ret < 0) {
 				dev_err(dev->dev,
 					"could not create CRTC (channel %u)\n",
@@ -404,7 +405,7 @@ static int omap_modeset_init(struct drm_device *dev)
 				return ret;
 			}
 
-			id++;
+			crtc_id++; plane_id++;
 		}
 	}
 
@@ -412,7 +413,7 @@ static int omap_modeset_init(struct drm_device *dev)
 	 * we have allocated crtcs according to the need of the panels/encoders,
 	 * adding more crtcs here if needed
 	 */
-	for (; id < num_crtcs; id++) {
+	for (; crtc_id < num_crtcs; crtc_id++) {
 
 		/* find a free manager for this crtc */
 		for (i = 0; i < num_mgrs; i++) {
@@ -426,21 +427,22 @@ static int omap_modeset_init(struct drm_device *dev)
 			return -ENOMEM;
 		}
 
-		ret = omap_modeset_create_crtc(dev, id, i);
+		ret = omap_modeset_create_crtc(dev, crtc_id, plane_id, i);
 		if (ret < 0) {
 			dev_err(dev->dev,
 				"could not create CRTC (channel %u)\n", i);
 			return ret;
 		}
+		plane_id++;
 	}
 
 	/*
 	 * Create normal planes for the remaining overlays:
 	 */
-	for (; id < num_ovls; id++) {
+	for (; plane_id < num_ovls; plane_id++) {
 		struct drm_plane *plane;
 
-		plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_OVERLAY);
+		plane = omap_plane_init(dev, plane_id, DRM_PLANE_TYPE_OVERLAY);
 		if (IS_ERR(plane))
 			return PTR_ERR(plane);
 
@@ -453,6 +455,7 @@ static int omap_modeset_init(struct drm_device *dev)
 		struct omap_dss_device *dssdev =
 					omap_encoder_get_dssdev(encoder);
 		struct omap_dss_device *output;
+		int id;
 
 		output = omapdss_find_output_from_display(dssdev);
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/omapdrm: Add gamma table support to DSS dispc

[Jyri Sarha]

Add gamma table support to DSS dispc.

DSS driver initializes the default gamma table at component bind time
and holds a copy of all gamma tables in it's internal data structure.

Each call to dispc_mgr_set_gamma() updates the internal table and
triggers write the HW, if it is enabled. The tables are restored to HW
in PM resume callback. The tables at DSS side matches the HW tables in
size and in number of significant bits per color component. The
dispc_mgr_set_gamma() converts the size of any given table to match
the table size in HW.

dispc_mgr_gamma_size() gives HW gamma table size for the channel
and returns 0 if gamma table is not supported by HW or DSS driver.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c        | 189 ++++++++++++++++++++++++++---
 drivers/gpu/drm/omapdrm/dss/dispc.h        |   5 +
 drivers/gpu/drm/omapdrm/dss/dss_features.c |   2 +
 drivers/gpu/drm/omapdrm/dss/dss_features.h |   1 +
 drivers/gpu/drm/omapdrm/dss/hdmi4.c        |   3 -
 drivers/gpu/drm/omapdrm/dss/hdmi5.c        |   3 -
 drivers/gpu/drm/omapdrm/dss/omapdss.h      |   5 +
 7 files changed, 186 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index f83608b..87dde05 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -116,6 +116,7 @@ struct dispc_features {
 };
 
 #define DISPC_MAX_NR_FIFOS 5
+#define DISPC_MAX_CHANNEL_GAMMA 4
 
 static struct {
 	struct platform_device *pdev;
@@ -135,6 +136,8 @@ static struct {
 	bool		ctx_valid;
 	u32		ctx[DISPC_SZ_REGS / sizeof(u32)];
 
+	u32 *gamma_table[DISPC_MAX_CHANNEL_GAMMA];
+
 	const struct dispc_features *feat;
 
 	bool is_enabled;
@@ -178,11 +181,19 @@ struct dispc_reg_field {
 	u8 low;
 };
 
+struct dispc_gamma_desc {
+	u32 len;
+	u32 bits;
+	u16 reg;
+	bool has_index;
+};
+
 static const struct {
 	const char *name;
 	u32 vsync_irq;
 	u32 framedone_irq;
 	u32 sync_lost_irq;
+	struct dispc_gamma_desc gamma;
 	struct dispc_reg_field reg_desc[DISPC_MGR_FLD_NUM];
 } mgr_desc[] = {
 	[OMAP_DSS_CHANNEL_LCD] = {
@@ -190,6 +201,12 @@ static const struct {
 		.vsync_irq	= DISPC_IRQ_VSYNC,
 		.framedone_irq	= DISPC_IRQ_FRAMEDONE,
 		.sync_lost_irq	= DISPC_IRQ_SYNC_LOST,
+		.gamma		= {
+			.len	= 256,
+			.bits	= 8,
+			.reg	= DISPC_GAMMA_TABLE0,
+			.has_index = true,
+		},
 		.reg_desc	= {
 			[DISPC_MGR_FLD_ENABLE]		= { DISPC_CONTROL,  0,  0 },
 			[DISPC_MGR_FLD_STNTFT]		= { DISPC_CONTROL,  3,  3 },
@@ -207,6 +224,12 @@ static const struct {
 		.vsync_irq	= DISPC_IRQ_EVSYNC_ODD | DISPC_IRQ_EVSYNC_EVEN,
 		.framedone_irq	= DISPC_IRQ_FRAMEDONETV,
 		.sync_lost_irq	= DISPC_IRQ_SYNC_LOST_DIGIT,
+		.gamma		= {
+			.len	= 1024,
+			.bits	= 10,
+			.reg	= DISPC_GAMMA_TABLE2,
+			.has_index = false,
+		},
 		.reg_desc	= {
 			[DISPC_MGR_FLD_ENABLE]		= { DISPC_CONTROL,  1,  1 },
 			[DISPC_MGR_FLD_STNTFT]		= { },
@@ -224,6 +247,12 @@ static const struct {
 		.vsync_irq	= DISPC_IRQ_VSYNC2,
 		.framedone_irq	= DISPC_IRQ_FRAMEDONE2,
 		.sync_lost_irq	= DISPC_IRQ_SYNC_LOST2,
+		.gamma		= {
+			.len	= 256,
+			.bits	= 8,
+			.reg	= DISPC_GAMMA_TABLE1,
+			.has_index = true,
+		},
 		.reg_desc	= {
 			[DISPC_MGR_FLD_ENABLE]		= { DISPC_CONTROL2,  0,  0 },
 			[DISPC_MGR_FLD_STNTFT]		= { DISPC_CONTROL2,  3,  3 },
@@ -241,6 +270,12 @@ static const struct {
 		.vsync_irq	= DISPC_IRQ_VSYNC3,
 		.framedone_irq	= DISPC_IRQ_FRAMEDONE3,
 		.sync_lost_irq	= DISPC_IRQ_SYNC_LOST3,
+		.gamma		= {
+			.len	= 256,
+			.bits	= 8,
+			.reg	= DISPC_GAMMA_TABLE3,
+			.has_index = true,
+		},
 		.reg_desc	= {
 			[DISPC_MGR_FLD_ENABLE]		= { DISPC_CONTROL3,  0,  0 },
 			[DISPC_MGR_FLD_STNTFT]		= { DISPC_CONTROL3,  3,  3 },
@@ -1084,20 +1119,6 @@ static u32 dispc_ovl_get_burst_size(enum omap_plane plane)
 	return unit * 8;
 }
 
-void dispc_enable_gamma_table(bool enable)
-{
-	/*
-	 * This is partially implemented to support only disabling of
-	 * the gamma table.
-	 */
-	if (enable) {
-		DSSWARN("Gamma table enabling for TV not yet supported");
-		return;
-	}
-
-	REG_FLD_MOD(DISPC_CONFIG, enable, 9, 9);
-}
-
 static void dispc_mgr_enable_cpr(enum omap_channel channel, bool enable)
 {
 	if (channel == OMAP_DSS_CHANNEL_DIGIT)
@@ -3814,6 +3835,128 @@ void dispc_disable_sidle(void)
 	REG_FLD_MOD(DISPC_SYSCONFIG, 1, 4, 3);	/* SIDLEMODE: no idle */
 }
 
+u32 dispc_mgr_gamma_size(enum omap_channel channel)
+{
+	const struct dispc_gamma_desc *gdesc = &mgr_desc[channel].gamma;
+
+	if (!dss_has_feature(FEAT_GAMMA_TABLE))
+		return 0;
+
+	return gdesc->len;
+}
+EXPORT_SYMBOL(dispc_mgr_gamma_size);
+
+static void dispc_mgr_write_gamma_table(enum omap_channel channel)
+{
+	const struct dispc_gamma_desc *gdesc = &mgr_desc[channel].gamma;
+	u32 *table = dispc.gamma_table[channel];
+	unsigned int i;
+
+	DSSDBG("%s: channel %d\n", __func__, channel);
+
+	for (i = 0; i < gdesc->len; ++i) {
+		u32 v = table[i];
+
+		if (gdesc->has_index)
+			v |= i << 24;
+		else if (i == 0)
+			v |= 1 << 31;
+
+		dispc_write_reg(gdesc->reg, v);
+	}
+}
+
+static void dispc_restore_gamma_tables(void)
+{
+	DSSDBG("%s()\n", __func__);
+
+	if (!dss_has_feature(FEAT_GAMMA_TABLE))
+		return;
+
+	dispc_mgr_write_gamma_table(OMAP_DSS_CHANNEL_LCD);
+
+	dispc_mgr_write_gamma_table(OMAP_DSS_CHANNEL_DIGIT);
+
+	if (dss_has_feature(FEAT_MGR_LCD2))
+		dispc_mgr_write_gamma_table(OMAP_DSS_CHANNEL_LCD2);
+
+	if (dss_has_feature(FEAT_MGR_LCD3))
+		dispc_mgr_write_gamma_table(OMAP_DSS_CHANNEL_LCD3);
+}
+
+void dispc_mgr_set_gamma(enum omap_channel channel,
+			 const struct drm_color_lut *lut,
+			 unsigned int length)
+{
+	const struct dispc_gamma_desc *gdesc = &mgr_desc[channel].gamma;
+	u32 *table = dispc.gamma_table[channel];
+	int i;
+
+	DSSDBG("%s: channel %d, lut len %u, hw len %u\n", __func__,
+	       channel, length, gdesc->len);
+
+	if (!dss_has_feature(FEAT_GAMMA_TABLE))
+		return;
+
+	for (i = 0; i < length; ++i) {
+		int first = i * (gdesc->len - 1) / (length - 1);
+		int last = (i + 1) * (gdesc->len - 1) / (length - 1);
+		int w = last - first;
+		u16 r, g, b;
+		int j;
+
+		for (j = 0; j <= w; j++) {
+			r = (lut[i].red * (w - j) + lut[i+1].red * j) / w;
+			g = (lut[i].green * (w - j) + lut[i+1].green * j) / w;
+			b = (lut[i].blue * (w - j) + lut[i+1].blue * j) / w;
+
+			r >>= (16 - gdesc->bits);
+			g >>= (16 - gdesc->bits);
+			b >>= (16 - gdesc->bits);
+
+			table[first + j] = (r << (gdesc->bits * 2)) |
+				(g << gdesc->bits) | b;
+		}
+	}
+
+	if (dispc.is_enabled)
+		dispc_mgr_write_gamma_table(channel);
+}
+EXPORT_SYMBOL(dispc_mgr_set_gamma);
+
+static int dispc_init_gamma_tables(void)
+{
+	int channel;
+
+	if (!dss_has_feature(FEAT_GAMMA_TABLE))
+		return;
+
+	for (channel = 0; channel < ARRAY_SIZE(dispc.gamma_table); channel++) {
+		const struct dispc_gamma_desc *gdesc = &mgr_desc[channel].gamma;
+		u32 *table;
+		int i;
+
+		if (channel == OMAP_DSS_CHANNEL_LCD2 &&
+		    !dss_has_feature(FEAT_MGR_LCD2))
+			continue;
+
+		if (channel == OMAP_DSS_CHANNEL_LCD3 &&
+		    !dss_has_feature(FEAT_MGR_LCD3))
+			continue;
+
+		table = devm_kmalloc_array(&dispc.pdev->dev, gdesc->len,
+					   sizeof(table[i]), GFP_KERNEL);
+		if (!table)
+			return -ENOMEM;
+
+		for (i = 0; i < gdesc->len; ++i)
+			table[i] =
+				(i << 2*gdesc->bits) | (i << gdesc->bits) | i;
+		dispc.gamma_table[channel] = table;
+	}
+	return 0;
+}
+
 static void _omap_dispc_initial_config(void)
 {
 	u32 l;
@@ -3829,8 +3972,16 @@ static void _omap_dispc_initial_config(void)
 		dispc.core_clk_rate = dispc_fclk_rate();
 	}
 
-	/* FUNCGATED */
-	if (dss_has_feature(FEAT_FUNCGATED))
+	/* Use gamma table */
+	if (dss_has_feature(FEAT_GAMMA_TABLE))
+		REG_FLD_MOD(DISPC_CONFIG, 1, 3, 3);
+
+	/* For older DSS versions (FEAT_FUNCGATED) this enables
+	 * func-clock auto-gating. For newer versions
+	 * (FEAT_GAMMA_TABLE) this enables tv-out gamma tables.
+	 */
+	if (dss_has_feature(FEAT_FUNCGATED) ||
+	    dss_has_feature(FEAT_GAMMA_TABLE))
 		REG_FLD_MOD(DISPC_CONFIG, 1, 9, 9);
 
 	dispc_setup_color_conv_coef();
@@ -4100,6 +4251,10 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
 		}
 	}
 
+	r = dispc_init_gamma_tables();
+	if (r)
+		return r;
+
 	pm_runtime_enable(&pdev->dev);
 
 	r = dispc_runtime_get();
@@ -4170,6 +4325,8 @@ static int dispc_runtime_resume(struct device *dev)
 		_omap_dispc_initial_config();
 
 		dispc_restore_context();
+
+		dispc_restore_gamma_tables();
 	}
 
 	dispc.is_enabled = true;
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.h b/drivers/gpu/drm/omapdrm/dss/dispc.h
index 4837442..bc1d812 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.h
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.h
@@ -42,6 +42,11 @@
 #define DISPC_MSTANDBY_CTRL		0x0858
 #define DISPC_GLOBAL_MFLAG_ATTRIBUTE	0x085C
 
+#define DISPC_GAMMA_TABLE0		0x0630
+#define DISPC_GAMMA_TABLE1		0x0634
+#define DISPC_GAMMA_TABLE2		0x0638
+#define DISPC_GAMMA_TABLE3		0x0850
+
 /* DISPC overlay registers */
 #define DISPC_OVL_BA0(n)		(DISPC_OVL_BASE(n) + \
 					DISPC_BA0_OFFSET(n))
diff --git a/drivers/gpu/drm/omapdrm/dss/dss_features.c b/drivers/gpu/drm/omapdrm/dss/dss_features.c
index c886a29..f77b1c5 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss_features.c
+++ b/drivers/gpu/drm/omapdrm/dss/dss_features.c
@@ -593,6 +593,7 @@ static const enum dss_feat_id omap4_dss_feat_list[] = {
 	FEAT_ALPHA_FREE_ZORDER,
 	FEAT_FIFO_MERGE,
 	FEAT_BURST_2D,
+	FEAT_GAMMA_TABLE,
 };
 
 static const enum dss_feat_id omap5_dss_feat_list[] = {
@@ -615,6 +616,7 @@ static const enum dss_feat_id omap5_dss_feat_list[] = {
 	FEAT_BURST_2D,
 	FEAT_DSI_PHY_DCC,
 	FEAT_MFLAG,
+	FEAT_GAMMA_TABLE,
 };
 
 /* OMAP2 DSS Features */
diff --git a/drivers/gpu/drm/omapdrm/dss/dss_features.h b/drivers/gpu/drm/omapdrm/dss/dss_features.h
index 3d67d39..d4d14db 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss_features.h
+++ b/drivers/gpu/drm/omapdrm/dss/dss_features.h
@@ -62,6 +62,7 @@ enum dss_feat_id {
 	FEAT_BURST_2D,
 	FEAT_DSI_PHY_DCC,
 	FEAT_MFLAG,
+	FEAT_GAMMA_TABLE,
 };
 
 /* DSS register field id */
diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
index f892ae15..12bef2d 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
@@ -213,9 +213,6 @@ static int hdmi_power_on_full(struct omap_dss_device *dssdev)
 
 	hdmi4_configure(&hdmi.core, &hdmi.wp, &hdmi.cfg);
 
-	/* bypass TV gamma table */
-	dispc_enable_gamma_table(0);
-
 	/* tv size */
 	dss_mgr_set_timings(channel, p);
 
diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
index a43f7b1..10530c5 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
@@ -230,9 +230,6 @@ static int hdmi_power_on_full(struct omap_dss_device *dssdev)
 
 	hdmi5_configure(&hdmi.core, &hdmi.wp, &hdmi.cfg);
 
-	/* bypass TV gamma table */
-	dispc_enable_gamma_table(0);
-
 	/* tv size */
 	dss_mgr_set_timings(channel, p);
 
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index d7e7c90..adcd315 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -19,6 +19,7 @@
 #define __OMAP_DRM_DSS_H
 
 #include <video/omapdss.h>
+#include <uapi/drm/drm_mode.h>
 
 u32 dispc_read_irqstatus(void);
 void dispc_clear_irqstatus(u32 mask);
@@ -44,6 +45,10 @@ void dispc_mgr_set_timings(enum omap_channel channel,
 		const struct omap_video_timings *timings);
 void dispc_mgr_setup(enum omap_channel channel,
 		const struct omap_overlay_manager_info *info);
+u32 dispc_mgr_gamma_size(enum omap_channel channel);
+void dispc_mgr_set_gamma(enum omap_channel channel,
+			 const struct drm_color_lut *lut,
+			 unsigned int length);
 
 int dispc_ovl_enable(enum omap_plane plane, bool enable);
 bool dispc_ovl_enabled(enum omap_plane plane);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/omapdrm: Work-a-round for errata i734 (LCD1 Gamma) in DSS dispc

[Jyri Sarha]

Work-a-round for errata i734 in DSS dispc
 - LCD1 Gamma Correction Is Not Working When GFX Pipe Is Disabled

For gamma tables to work on LCD1 the GFX plane has to be used at least
once after DSS HW has come out of reset. The work-a-round sets up a
minimal LCD setup with GFX plane and waits for one vertical sync irq
before disabling the setup and continuing with the context
restore. The physical outputs are gated during the operation. This
work-a-round requires that gamma table's LOADMODE is set to 0x2 in
DISPC_CONTROL1 register.

For details see:
OMAP543x Multimedia Device Silicon Revision 2.0 Silicon Errata
Literature Number: SWPZ037E
Or some other relevant errata document for the DSS IP version.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c        | 166 +++++++++++++++++++++++++++++
 drivers/gpu/drm/omapdrm/dss/dss_features.c |   2 +
 drivers/gpu/drm/omapdrm/dss/dss_features.h |   1 +
 3 files changed, 169 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 87dde05..3608cf2 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -4201,6 +4201,164 @@ void dispc_free_irq(void *dev_id)
 }
 EXPORT_SYMBOL(dispc_free_irq);
 
+/*
+ * Work-a-round for errata i734 in DSS dispc
+ *  - LCD1 Gamma Correction Is Not Working When GFX Pipe Is Disabled
+ *
+ * For gamma tables to work on LCD1 the GFX plane has to be used at
+ * least once after DSS HW has come out of reset. The work-a-round
+ * sets up a minimal LCD setup with GFX plane and waits for one
+ * vertical sync irq before disabling the setup and continuing with
+ * the context restore. The physical outputs are gated during the
+ * operation. This work-a-round requires that gamma table's LOADMODE
+ * is set to 0x2 in DISPC_CONTROL1 register.
+ *
+ * For details see:
+ * OMAP543x Multimedia Device Silicon Revision 2.0 Silicon Errata
+ * Literature Number: SWPZ037E
+ * Or some other relevant errata document for the DSS IP version.
+ */
+
+static struct dispc_errata_i734_data {
+	struct omap_video_timings timings;
+	struct omap_overlay_info ovli;
+	struct omap_overlay_manager_info mgri;
+	struct dss_lcd_mgr_config lcd_conf;
+	struct i734_buf {
+		dma_addr_t paddr;
+		void *vaddr;
+	} buf;
+} i734 = {
+	.timings = {
+		.x_res = 8, .y_res = 1,
+		.pixelclock = 16000000,
+		.hsw = 8, .hfp = 4, .hbp = 4,
+		.vsw = 1, .vfp = 1, .vbp = 1,
+		.vsync_level = OMAPDSS_SIG_ACTIVE_LOW,
+		.hsync_level = OMAPDSS_SIG_ACTIVE_LOW,
+		.interlace = false,
+		.data_pclk_edge = OMAPDSS_DRIVE_SIG_RISING_EDGE,
+		.de_level = OMAPDSS_SIG_ACTIVE_HIGH,
+		.sync_pclk_edge = OMAPDSS_DRIVE_SIG_RISING_EDGE,
+		.double_pixel = false,
+	},
+	.ovli = {
+		.screen_width = 1,
+		.width = 1, .height = 1,
+		.color_mode = OMAP_DSS_COLOR_RGB24U,
+		.rotation = OMAP_DSS_ROT_0,
+		.rotation_type = OMAP_DSS_ROT_DMA,
+		.mirror = 0,
+		.pos_x = 0, .pos_y = 0,
+		.out_width = 0, .out_height = 0,
+		.global_alpha = 0xff,
+		.pre_mult_alpha = 0,
+		.zorder = 0,
+	},
+	.mgri = {
+		.default_color = 0,
+		.trans_enabled = false,
+		.partial_alpha_enabled = false,
+		.cpr_enable = false,
+	},
+	.lcd_conf = {
+		.io_pad_mode = DSS_IO_PAD_MODE_BYPASS,
+		.stallmode = false,
+		.fifohandcheck = false,
+		.clock_info = {
+			.lck_div = 1,
+			.pck_div = 2,
+		},
+		.video_port_width = 24,
+		.lcden_sig_polarity = 0,
+	},
+};
+
+static int dispc_errata_i734_wa_init(void)
+{
+	size_t buff_size = i734.ovli.width * i734.ovli.height *
+		color_mode_to_bpp(i734.ovli.color_mode) / 8;
+
+	if (!dss_has_feature(FEAT_GAMMA_I734_BUG))
+		return 0;
+
+	i734.buf.vaddr = dma_alloc_writecombine(&dispc.pdev->dev, buff_size,
+						&i734.buf.paddr, GFP_KERNEL);
+	if (WARN_ON(IS_ERR(i734.buf.vaddr)))
+		return PTR_ERR(i734.buf.vaddr);
+
+	i734.ovli.paddr = i734.buf.paddr;
+
+	return 0;
+}
+
+static void dispc_errata_i734_wa_fini(void)
+{
+	size_t buff_size = i734.ovli.width * i734.ovli.height *
+		color_mode_to_bpp(i734.ovli.color_mode) / 8;
+
+	if (!dss_has_feature(FEAT_GAMMA_I734_BUG))
+		return;
+
+	dma_free_writecombine(&dispc.pdev->dev, buff_size, i734.buf.vaddr,
+			      i734.buf.paddr);
+}
+
+static void dispc_errata_i734_wa(void)
+{
+	u32 vsync_irq = dispc_mgr_get_vsync_irq(OMAP_DSS_CHANNEL_LCD);
+	u32 framedone_irq = dispc_mgr_get_framedone_irq(OMAP_DSS_CHANNEL_LCD);
+	u32 gatestate = REG_GET(DISPC_CONTROL, 8, 4);
+	unsigned int count;
+
+	if (!dss_has_feature(FEAT_GAMMA_I734_BUG))
+		return;
+
+	/* Gate all LCD1 outputs */
+	REG_FLD_MOD(DISPC_CONTROL, 0, 8, 4);
+
+	/* Setup and enable GFX plane */
+	dispc_ovl_set_channel_out(OMAP_DSS_GFX, OMAP_DSS_CHANNEL_LCD);
+	dispc_ovl_setup(OMAP_DSS_GFX, &i734.ovli, false, &i734.timings, false);
+	dispc_ovl_enable(OMAP_DSS_GFX, true);
+
+	/* Set up and enable display manager for LCD1 */
+	dispc_mgr_setup(OMAP_DSS_CHANNEL_LCD, &i734.mgri);
+	dispc_calc_clock_rates(dss_get_dispc_clk_rate(),
+			       &i734.lcd_conf.clock_info);
+	dispc_mgr_set_lcd_config(OMAP_DSS_CHANNEL_LCD, &i734.lcd_conf);
+	dispc_mgr_set_timings(OMAP_DSS_CHANNEL_LCD, &i734.timings);
+	dispc_mgr_enable(OMAP_DSS_CHANNEL_LCD, true);
+
+	dispc_clear_irqstatus(vsync_irq | framedone_irq);
+
+	/* Busy wait for vsync (we can't fiddle with irq handlers in resume) */
+	count = 0;
+	while (!(dispc_read_irqstatus() & vsync_irq))
+		if (count++ > 10000) {
+			dev_err(&dispc.pdev->dev, "%s: vsync timeout\n",
+				__func__);
+			break;
+		}
+
+	/* Shut down the WA setup */
+	dispc_mgr_enable(OMAP_DSS_CHANNEL_LCD, false);
+	dispc_ovl_enable(OMAP_DSS_GFX, false);
+	count = 0;
+	while (!(dispc_read_irqstatus() & framedone_irq))
+		if (count++ > 10000) {
+			dev_err(&dispc.pdev->dev, "%s: framedone timeout\n",
+				__func__);
+			break;
+		}
+
+	/* Clear all irq bits before continuing */
+	dispc_clear_irqstatus(0xffffffff);
+
+	/* Restore the original state to LCD1 output gates */
+	REG_FLD_MOD(DISPC_CONTROL, gatestate, 8, 4);
+}
+
 /* DISPC HW IP initialisation */
 static int dispc_bind(struct device *dev, struct device *master, void *data)
 {
@@ -4214,6 +4372,10 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
 
 	spin_lock_init(&dispc.control_lock);
 
+	r = dispc_errata_i734_wa_init();
+	if (r)
+		return r;
+
 	r = dispc_init_features(dispc.pdev);
 	if (r)
 		return r;
@@ -4282,6 +4444,8 @@ static void dispc_unbind(struct device *dev, struct device *master,
 			       void *data)
 {
 	pm_runtime_disable(dev);
+
+	dispc_errata_i734_wa_fini();
 }
 
 static const struct component_ops dispc_component_ops = {
@@ -4324,6 +4488,8 @@ static int dispc_runtime_resume(struct device *dev)
 	if (REG_GET(DISPC_CONFIG, 2, 1) != OMAP_DSS_LOAD_FRAME_ONLY) {
 		_omap_dispc_initial_config();
 
+		dispc_errata_i734_wa();
+
 		dispc_restore_context();
 
 		dispc_restore_gamma_tables();
diff --git a/drivers/gpu/drm/omapdrm/dss/dss_features.c b/drivers/gpu/drm/omapdrm/dss/dss_features.c
index f77b1c5..3127bd6 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss_features.c
+++ b/drivers/gpu/drm/omapdrm/dss/dss_features.c
@@ -594,6 +594,7 @@ static const enum dss_feat_id omap4_dss_feat_list[] = {
 	FEAT_FIFO_MERGE,
 	FEAT_BURST_2D,
 	FEAT_GAMMA_TABLE,
+	FEAT_GAMMA_I734_BUG,
 };
 
 static const enum dss_feat_id omap5_dss_feat_list[] = {
@@ -617,6 +618,7 @@ static const enum dss_feat_id omap5_dss_feat_list[] = {
 	FEAT_DSI_PHY_DCC,
 	FEAT_MFLAG,
 	FEAT_GAMMA_TABLE,
+	FEAT_GAMMA_I734_BUG,
 };
 
 /* OMAP2 DSS Features */
diff --git a/drivers/gpu/drm/omapdrm/dss/dss_features.h b/drivers/gpu/drm/omapdrm/dss/dss_features.h
index d4d14db..0f74083 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss_features.h
+++ b/drivers/gpu/drm/omapdrm/dss/dss_features.h
@@ -63,6 +63,7 @@ enum dss_feat_id {
 	FEAT_DSI_PHY_DCC,
 	FEAT_MFLAG,
 	FEAT_GAMMA_TABLE,
+	FEAT_GAMMA_I734_BUG,
 };
 
 /* DSS register field id */
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/omapdrm: Implement gamma_lut atomic crtc property

[Jyri Sarha]

Implement gamma_lut atomic crtc property, set crtc gamma size to 256
for all crtcs and use drm_atomic_helper_legacy_gamma_set() as
gamma_set func. The tv-out crtc has 1024 element gamma table (with
10bit precision) in HW, but current Xorg server does not accept
anything else but 256 elements so that is used for all CRTCs. The dss
dispc API converts table of any length for HW and uses linear
interpolation in the process.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 075f2bb..d5210fe 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -384,6 +384,15 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
 
 	WARN_ON(omap_crtc->vblank_irq.registered);
 
+	if (crtc->state->color_mgmt_changed) {
+		struct drm_color_lut *lut = (struct drm_color_lut *)
+			crtc->state->gamma_lut->data;
+		unsigned int length = crtc->state->gamma_lut->length /
+			sizeof(*lut);
+
+		dispc_mgr_set_gamma(omap_crtc->channel, lut, length);
+	}
+
 	if (dispc_mgr_is_enabled(omap_crtc->channel)) {
 
 		DBG("%s: GO", omap_crtc->name);
@@ -460,6 +469,7 @@ static const struct drm_crtc_funcs omap_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = omap_crtc_destroy,
 	.page_flip = drm_atomic_helper_page_flip,
+	.gamma_set = drm_atomic_helper_legacy_gamma_set,
 	.set_property = drm_atomic_helper_crtc_set_property,
 	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
@@ -534,6 +544,16 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 
 	drm_crtc_helper_add(crtc, &omap_crtc_helper_funcs);
 
+	/* The dispc API adapts to what ever size, but the HW supports
+	 * 256 element gamma table for LCDs and 1024 element table for
+	 * OMAP_DSS_CHANNEL_DIGIT. X server assumes 256 element gamma
+	 * tables so lets use that. Size of HW gamma table can be
+	 * extracted with dispc_mgr_gamma_size(). If it returns 0
+	 * gamma table is not supprted.
+	 */
+	if (dispc_mgr_gamma_size(channel))
+		drm_mode_crtc_set_gamma_size(crtc, 256);
+
 	omap_plane_install_properties(crtc->primary, &crtc->base);
 
 	omap_crtcs[channel] = omap_crtc;
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/omapdrm: Implement gamma_lut atomic crtc property

[Daniel Vetter]

On Fri, May 20, 2016 at 09:35:56AM +0300, Jyri Sarha wrote:
> Implement gamma_lut atomic crtc property, set crtc gamma size to 256
> for all crtcs and use drm_atomic_helper_legacy_gamma_set() as
> gamma_set func. The tv-out crtc has 1024 element gamma table (with
> 10bit precision) in HW, but current Xorg server does not accept
> anything else but 256 elements so that is used for all CRTCs. The dss
> dispc API converts table of any length for HW and uses linear
> interpolation in the process.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>

I think you also want to wire up drm_atomic_helper_legacy_gamma_set so
that legacy clients using the gamma ioctl will work with this support.
-Daniel

> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 075f2bb..d5210fe 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -384,6 +384,15 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
>  
>  	WARN_ON(omap_crtc->vblank_irq.registered);
>  
> +	if (crtc->state->color_mgmt_changed) {
> +		struct drm_color_lut *lut = (struct drm_color_lut *)
> +			crtc->state->gamma_lut->data;
> +		unsigned int length = crtc->state->gamma_lut->length /
> +			sizeof(*lut);
> +
> +		dispc_mgr_set_gamma(omap_crtc->channel, lut, length);
> +	}
> +
>  	if (dispc_mgr_is_enabled(omap_crtc->channel)) {
>  
>  		DBG("%s: GO", omap_crtc->name);
> @@ -460,6 +469,7 @@ static const struct drm_crtc_funcs omap_crtc_funcs = {
>  	.set_config = drm_atomic_helper_set_config,
>  	.destroy = omap_crtc_destroy,
>  	.page_flip = drm_atomic_helper_page_flip,
> +	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>  	.set_property = drm_atomic_helper_crtc_set_property,
>  	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> @@ -534,6 +544,16 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>  
>  	drm_crtc_helper_add(crtc, &omap_crtc_helper_funcs);
>  
> +	/* The dispc API adapts to what ever size, but the HW supports
> +	 * 256 element gamma table for LCDs and 1024 element table for
> +	 * OMAP_DSS_CHANNEL_DIGIT. X server assumes 256 element gamma
> +	 * tables so lets use that. Size of HW gamma table can be
> +	 * extracted with dispc_mgr_gamma_size(). If it returns 0
> +	 * gamma table is not supprted.
> +	 */
> +	if (dispc_mgr_gamma_size(channel))
> +		drm_mode_crtc_set_gamma_size(crtc, 256);
> +
>  	omap_plane_install_properties(crtc->primary, &crtc->base);
>  
>  	omap_crtcs[channel] = omap_crtc;
> -- 
> 1.9.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/omapdrm: Implement gamma_lut atomic crtc property

[Jyri Sarha]

On 05/20/16 10:05, Daniel Vetter wrote:
> On Fri, May 20, 2016 at 09:35:56AM +0300, Jyri Sarha wrote:
>> > Implement gamma_lut atomic crtc property, set crtc gamma size to 256
>> > for all crtcs and use drm_atomic_helper_legacy_gamma_set() as
>> > gamma_set func. The tv-out crtc has 1024 element gamma table (with
>> > 10bit precision) in HW, but current Xorg server does not accept
>> > anything else but 256 elements so that is used for all CRTCs. The dss
>> > dispc API converts table of any length for HW and uses linear
>> > interpolation in the process.
>> > 
>> > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> I think you also want to wire up drm_atomic_helper_legacy_gamma_set so
> that legacy clients using the gamma ioctl will work with this support.

But it is there couple of lines down, or am I missing something?

BR,
Jyri

> 
>> > ---
>> >  drivers/gpu/drm/omapdrm/omap_crtc.c | 20 ++++++++++++++++++++
>> >  1 file changed, 20 insertions(+)
...
>> > @@ -460,6 +469,7 @@ static const struct drm_crtc_funcs omap_crtc_funcs = {
>> >  	.set_config = drm_atomic_helper_set_config,
>> >  	.destroy = omap_crtc_destroy,
>> >  	.page_flip = drm_atomic_helper_page_flip,
>> > +	.gamma_set = drm_atomic_helper_legacy_gamma_set,
Here ^^^
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/omapdrm: Implement gamma_lut atomic crtc property

[Daniel Vetter]

On Fri, May 20, 2016 at 9:35 AM, Jyri Sarha <jsarha@ti.com> wrote:
> On 05/20/16 10:05, Daniel Vetter wrote:
>> On Fri, May 20, 2016 at 09:35:56AM +0300, Jyri Sarha wrote:
>>> > Implement gamma_lut atomic crtc property, set crtc gamma size to 256
>>> > for all crtcs and use drm_atomic_helper_legacy_gamma_set() as
>>> > gamma_set func. The tv-out crtc has 1024 element gamma table (with
>>> > 10bit precision) in HW, but current Xorg server does not accept
>>> > anything else but 256 elements so that is used for all CRTCs. The dss
>>> > dispc API converts table of any length for HW and uses linear
>>> > interpolation in the process.
>>> >
>>> > Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> I think you also want to wire up drm_atomic_helper_legacy_gamma_set so
>> that legacy clients using the gamma ioctl will work with this support.
>
> But it is there couple of lines down, or am I missing something?

Coffee didn't work yet over here - I looked for it and was blind ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/omapdrm: omap_modeset_init: Separate crtc id and plane id indexing

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 651 bytes --]

On 20/05/16 09:35, Jyri Sarha wrote:
> Separate crtc id and plane id indexing in omap_modeset_init(). The
> coupling of crtc- and plane-id is hard to follow.

The id stuff does need cleanup, but I don't think this patch does that
well enough. A few quick ones:

omap_crtc_init() doesn't use the id parameter, so it could just be dropped.

"crtc_id" and "plane_id" are misleading. Usually those mean the id
numbere of the DRM object. Here plane_id is really the 'enum omap_plane'
from omapdss, if I'm not mistaken.

Probably the first step with cleaning this up would be to rename that
enum so that it can be used in omapdrm.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/omapdrm: Add gamma table support to DSS dispc

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 11385 bytes --]

Hi,

On 20/05/16 09:35, Jyri Sarha wrote:
> Add gamma table support to DSS dispc.
> 
> DSS driver initializes the default gamma table at component bind time
> and holds a copy of all gamma tables in it's internal data structure.

"its"

> Each call to dispc_mgr_set_gamma() updates the internal table and
> triggers write the HW, if it is enabled. The tables are restored to HW

"... to the HW"?

> in PM resume callback. The tables at DSS side matches the HW tables in
> size and in number of significant bits per color component. The

Maybe "the tables in RAM match the tables in HW".

> dispc_mgr_set_gamma() converts the size of any given table to match
> the table size in HW.
> 
> dispc_mgr_gamma_size() gives HW gamma table size for the channel
> and returns 0 if gamma table is not supported by HW or DSS driver.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c        | 189 ++++++++++++++++++++++++++---
>  drivers/gpu/drm/omapdrm/dss/dispc.h        |   5 +
>  drivers/gpu/drm/omapdrm/dss/dss_features.c |   2 +
>  drivers/gpu/drm/omapdrm/dss/dss_features.h |   1 +
>  drivers/gpu/drm/omapdrm/dss/hdmi4.c        |   3 -
>  drivers/gpu/drm/omapdrm/dss/hdmi5.c        |   3 -
>  drivers/gpu/drm/omapdrm/dss/omapdss.h      |   5 +
>  7 files changed, 186 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index f83608b..87dde05 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -116,6 +116,7 @@ struct dispc_features {
>  };
>  
>  #define DISPC_MAX_NR_FIFOS 5
> +#define DISPC_MAX_CHANNEL_GAMMA 4
>  
>  static struct {
>  	struct platform_device *pdev;
> @@ -135,6 +136,8 @@ static struct {
>  	bool		ctx_valid;
>  	u32		ctx[DISPC_SZ_REGS / sizeof(u32)];
>  
> +	u32 *gamma_table[DISPC_MAX_CHANNEL_GAMMA];
> +
>  	const struct dispc_features *feat;
>  
>  	bool is_enabled;
> @@ -178,11 +181,19 @@ struct dispc_reg_field {
>  	u8 low;
>  };
>  
> +struct dispc_gamma_desc {
> +	u32 len;
> +	u32 bits;
> +	u16 reg;
> +	bool has_index;
> +};
> +
>  static const struct {
>  	const char *name;
>  	u32 vsync_irq;
>  	u32 framedone_irq;
>  	u32 sync_lost_irq;
> +	struct dispc_gamma_desc gamma;
>  	struct dispc_reg_field reg_desc[DISPC_MGR_FLD_NUM];
>  } mgr_desc[] = {
>  	[OMAP_DSS_CHANNEL_LCD] = {
> @@ -190,6 +201,12 @@ static const struct {
>  		.vsync_irq	= DISPC_IRQ_VSYNC,
>  		.framedone_irq	= DISPC_IRQ_FRAMEDONE,
>  		.sync_lost_irq	= DISPC_IRQ_SYNC_LOST,
> +		.gamma		= {
> +			.len	= 256,
> +			.bits	= 8,
> +			.reg	= DISPC_GAMMA_TABLE0,
> +			.has_index = true,
> +		},
>  		.reg_desc	= {
>  			[DISPC_MGR_FLD_ENABLE]		= { DISPC_CONTROL,  0,  0 },
>  			[DISPC_MGR_FLD_STNTFT]		= { DISPC_CONTROL,  3,  3 },
> @@ -207,6 +224,12 @@ static const struct {
>  		.vsync_irq	= DISPC_IRQ_EVSYNC_ODD | DISPC_IRQ_EVSYNC_EVEN,
>  		.framedone_irq	= DISPC_IRQ_FRAMEDONETV,
>  		.sync_lost_irq	= DISPC_IRQ_SYNC_LOST_DIGIT,
> +		.gamma		= {
> +			.len	= 1024,
> +			.bits	= 10,
> +			.reg	= DISPC_GAMMA_TABLE2,
> +			.has_index = false,
> +		},
>  		.reg_desc	= {
>  			[DISPC_MGR_FLD_ENABLE]		= { DISPC_CONTROL,  1,  1 },
>  			[DISPC_MGR_FLD_STNTFT]		= { },
> @@ -224,6 +247,12 @@ static const struct {
>  		.vsync_irq	= DISPC_IRQ_VSYNC2,
>  		.framedone_irq	= DISPC_IRQ_FRAMEDONE2,
>  		.sync_lost_irq	= DISPC_IRQ_SYNC_LOST2,
> +		.gamma		= {
> +			.len	= 256,
> +			.bits	= 8,
> +			.reg	= DISPC_GAMMA_TABLE1,
> +			.has_index = true,
> +		},
>  		.reg_desc	= {
>  			[DISPC_MGR_FLD_ENABLE]		= { DISPC_CONTROL2,  0,  0 },
>  			[DISPC_MGR_FLD_STNTFT]		= { DISPC_CONTROL2,  3,  3 },
> @@ -241,6 +270,12 @@ static const struct {
>  		.vsync_irq	= DISPC_IRQ_VSYNC3,
>  		.framedone_irq	= DISPC_IRQ_FRAMEDONE3,
>  		.sync_lost_irq	= DISPC_IRQ_SYNC_LOST3,
> +		.gamma		= {
> +			.len	= 256,
> +			.bits	= 8,
> +			.reg	= DISPC_GAMMA_TABLE3,
> +			.has_index = true,
> +		},
>  		.reg_desc	= {
>  			[DISPC_MGR_FLD_ENABLE]		= { DISPC_CONTROL3,  0,  0 },
>  			[DISPC_MGR_FLD_STNTFT]		= { DISPC_CONTROL3,  3,  3 },
> @@ -1084,20 +1119,6 @@ static u32 dispc_ovl_get_burst_size(enum omap_plane plane)
>  	return unit * 8;
>  }
>  
> -void dispc_enable_gamma_table(bool enable)
> -{
> -	/*
> -	 * This is partially implemented to support only disabling of
> -	 * the gamma table.
> -	 */
> -	if (enable) {
> -		DSSWARN("Gamma table enabling for TV not yet supported");
> -		return;
> -	}
> -
> -	REG_FLD_MOD(DISPC_CONFIG, enable, 9, 9);
> -}
> -
>  static void dispc_mgr_enable_cpr(enum omap_channel channel, bool enable)
>  {
>  	if (channel == OMAP_DSS_CHANNEL_DIGIT)
> @@ -3814,6 +3835,128 @@ void dispc_disable_sidle(void)
>  	REG_FLD_MOD(DISPC_SYSCONFIG, 1, 4, 3);	/* SIDLEMODE: no idle */
>  }
>  
> +u32 dispc_mgr_gamma_size(enum omap_channel channel)
> +{
> +	const struct dispc_gamma_desc *gdesc = &mgr_desc[channel].gamma;
> +
> +	if (!dss_has_feature(FEAT_GAMMA_TABLE))
> +		return 0;
> +

This should probably check the availability of LCD2/LCD3?

> +	return gdesc->len;
> +}
> +EXPORT_SYMBOL(dispc_mgr_gamma_size);
> +
> +static void dispc_mgr_write_gamma_table(enum omap_channel channel)
> +{
> +	const struct dispc_gamma_desc *gdesc = &mgr_desc[channel].gamma;
> +	u32 *table = dispc.gamma_table[channel];
> +	unsigned int i;
> +
> +	DSSDBG("%s: channel %d\n", __func__, channel);
> +
> +	for (i = 0; i < gdesc->len; ++i) {
> +		u32 v = table[i];
> +
> +		if (gdesc->has_index)
> +			v |= i << 24;
> +		else if (i == 0)
> +			v |= 1 << 31;
> +
> +		dispc_write_reg(gdesc->reg, v);
> +	}
> +}
> +
> +static void dispc_restore_gamma_tables(void)
> +{
> +	DSSDBG("%s()\n", __func__);
> +
> +	if (!dss_has_feature(FEAT_GAMMA_TABLE))
> +		return;
> +
> +	dispc_mgr_write_gamma_table(OMAP_DSS_CHANNEL_LCD);
> +
> +	dispc_mgr_write_gamma_table(OMAP_DSS_CHANNEL_DIGIT);
> +
> +	if (dss_has_feature(FEAT_MGR_LCD2))
> +		dispc_mgr_write_gamma_table(OMAP_DSS_CHANNEL_LCD2);
> +
> +	if (dss_has_feature(FEAT_MGR_LCD3))
> +		dispc_mgr_write_gamma_table(OMAP_DSS_CHANNEL_LCD3);
> +}
> +
> +void dispc_mgr_set_gamma(enum omap_channel channel,
> +			 const struct drm_color_lut *lut,
> +			 unsigned int length)
> +{
> +	const struct dispc_gamma_desc *gdesc = &mgr_desc[channel].gamma;
> +	u32 *table = dispc.gamma_table[channel];
> +	int i;

unsigned.

> +
> +	DSSDBG("%s: channel %d, lut len %u, hw len %u\n", __func__,
> +	       channel, length, gdesc->len);
> +
> +	if (!dss_has_feature(FEAT_GAMMA_TABLE))
> +		return;
> +
> +	for (i = 0; i < length; ++i) {
> +		int first = i * (gdesc->len - 1) / (length - 1);
> +		int last = (i + 1) * (gdesc->len - 1) / (length - 1);

This might be slightly more readable if you hade src_len and dst_len
local variables. Or maybe it's clear enough, it's a short function.

> +		int w = last - first;

unsigned.

> +		u16 r, g, b;
> +		int j;

unsigned.

> +
> +		for (j = 0; j <= w; j++) {
> +			r = (lut[i].red * (w - j) + lut[i+1].red * j) / w;
> +			g = (lut[i].green * (w - j) + lut[i+1].green * j) / w;
> +			b = (lut[i].blue * (w - j) + lut[i+1].blue * j) / w;
> +
> +			r >>= (16 - gdesc->bits);
> +			g >>= (16 - gdesc->bits);
> +			b >>= (16 - gdesc->bits);

I don't think the parenthesis do anything here.

> +
> +			table[first + j] = (r << (gdesc->bits * 2)) |
> +				(g << gdesc->bits) | b;
> +		}
> +	}
> +
> +	if (dispc.is_enabled)
> +		dispc_mgr_write_gamma_table(channel);
> +}
> +EXPORT_SYMBOL(dispc_mgr_set_gamma);
> +
> +static int dispc_init_gamma_tables(void)
> +{
> +	int channel;
> +
> +	if (!dss_has_feature(FEAT_GAMMA_TABLE))
> +		return;
> +
> +	for (channel = 0; channel < ARRAY_SIZE(dispc.gamma_table); channel++) {
> +		const struct dispc_gamma_desc *gdesc = &mgr_desc[channel].gamma;
> +		u32 *table;
> +		int i;

unsigned.

> +
> +		if (channel == OMAP_DSS_CHANNEL_LCD2 &&
> +		    !dss_has_feature(FEAT_MGR_LCD2))
> +			continue;
> +
> +		if (channel == OMAP_DSS_CHANNEL_LCD3 &&
> +		    !dss_has_feature(FEAT_MGR_LCD3))
> +			continue;
> +
> +		table = devm_kmalloc_array(&dispc.pdev->dev, gdesc->len,
> +					   sizeof(table[i]), GFP_KERNEL);

Wouldn't allocating gdesc->len * sizeof(u32) be much more
understandable? 'i' is even uninitialized at this point, making the code
quite suspicious.

> +		if (!table)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < gdesc->len; ++i)
> +			table[i] =
> +				(i << 2*gdesc->bits) | (i << gdesc->bits) | i;

Add an empty line here.

Add spaces around '*'.

And instead of splitting the line above, perhaps assign the value to u32
local, and assign that to table[i].

> +		dispc.gamma_table[channel] = table;
> +	}
> +	return 0;
> +}
> +
>  static void _omap_dispc_initial_config(void)
>  {
>  	u32 l;
> @@ -3829,8 +3972,16 @@ static void _omap_dispc_initial_config(void)
>  		dispc.core_clk_rate = dispc_fclk_rate();
>  	}
>  
> -	/* FUNCGATED */
> -	if (dss_has_feature(FEAT_FUNCGATED))
> +	/* Use gamma table */

Use gamma table for what, instead of what?

> +	if (dss_has_feature(FEAT_GAMMA_TABLE))
> +		REG_FLD_MOD(DISPC_CONFIG, 1, 3, 3);
> +
> +	/* For older DSS versions (FEAT_FUNCGATED) this enables
> +	 * func-clock auto-gating. For newer versions
> +	 * (FEAT_GAMMA_TABLE) this enables tv-out gamma tables.
> +	 */
> +	if (dss_has_feature(FEAT_FUNCGATED) ||
> +	    dss_has_feature(FEAT_GAMMA_TABLE))
>  		REG_FLD_MOD(DISPC_CONFIG, 1, 9, 9);
>  
>  	dispc_setup_color_conv_coef();
> @@ -4100,6 +4251,10 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
>  		}
>  	}
>  
> +	r = dispc_init_gamma_tables();
> +	if (r)
> +		return r;
> +
>  	pm_runtime_enable(&pdev->dev);
>  
>  	r = dispc_runtime_get();
> @@ -4170,6 +4325,8 @@ static int dispc_runtime_resume(struct device *dev)
>  		_omap_dispc_initial_config();
>  
>  		dispc_restore_context();
> +
> +		dispc_restore_gamma_tables();
>  	}
>  
>  	dispc.is_enabled = true;
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.h b/drivers/gpu/drm/omapdrm/dss/dispc.h
> index 4837442..bc1d812 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.h
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.h
> @@ -42,6 +42,11 @@
>  #define DISPC_MSTANDBY_CTRL		0x0858
>  #define DISPC_GLOBAL_MFLAG_ATTRIBUTE	0x085C
>  
> +#define DISPC_GAMMA_TABLE0		0x0630
> +#define DISPC_GAMMA_TABLE1		0x0634
> +#define DISPC_GAMMA_TABLE2		0x0638
> +#define DISPC_GAMMA_TABLE3		0x0850
> +
>  /* DISPC overlay registers */
>  #define DISPC_OVL_BA0(n)		(DISPC_OVL_BASE(n) + \
>  					DISPC_BA0_OFFSET(n))
> diff --git a/drivers/gpu/drm/omapdrm/dss/dss_features.c b/drivers/gpu/drm/omapdrm/dss/dss_features.c
> index c886a29..f77b1c5 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dss_features.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dss_features.c
> @@ -593,6 +593,7 @@ static const enum dss_feat_id omap4_dss_feat_list[] = {
>  	FEAT_ALPHA_FREE_ZORDER,
>  	FEAT_FIFO_MERGE,
>  	FEAT_BURST_2D,
> +	FEAT_GAMMA_TABLE,
>  };

The dss_features is deprecated. New features should be added to the
respective driver. In this case, dispc.c. See struct dispc_features.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/omapdrm: Add gamma table support to DSS dispc

[Jyri Sarha]

[-- Attachment #1.1.1: Type: text/plain, Size: 462 bytes --]

On 05/20/16 15:51, Tomi Valkeinen wrote:
>> +u32 dispc_mgr_gamma_size(enum omap_channel channel)
>> > +{
>> > +	const struct dispc_gamma_desc *gdesc = &mgr_desc[channel].gamma;
>> > +
>> > +	if (!dss_has_feature(FEAT_GAMMA_TABLE))
>> > +		return 0;
>> > +
> This should probably check the availability of LCD2/LCD3?
> 

That would be a double check (the function should never be called with
channels that are not there), but I am happy to add it.


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/omapdrm: Work-a-round for errata i734 (LCD1 Gamma) in DSS dispc

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 9421 bytes --]

Hi,

On 20/05/16 09:35, Jyri Sarha wrote:
> Work-a-round for errata i734 in DSS dispc
>  - LCD1 Gamma Correction Is Not Working When GFX Pipe Is Disabled
> 
> For gamma tables to work on LCD1 the GFX plane has to be used at least
> once after DSS HW has come out of reset. The work-a-round sets up a
> minimal LCD setup with GFX plane and waits for one vertical sync irq
> before disabling the setup and continuing with the context
> restore. The physical outputs are gated during the operation. This
> work-a-round requires that gamma table's LOADMODE is set to 0x2 in
> DISPC_CONTROL1 register.
> 
> For details see:
> OMAP543x Multimedia Device Silicon Revision 2.0 Silicon Errata
> Literature Number: SWPZ037E
> Or some other relevant errata document for the DSS IP version.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c        | 166 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/omapdrm/dss/dss_features.c |   2 +
>  drivers/gpu/drm/omapdrm/dss/dss_features.h |   1 +
>  3 files changed, 169 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index 87dde05..3608cf2 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -4201,6 +4201,164 @@ void dispc_free_irq(void *dev_id)
>  }
>  EXPORT_SYMBOL(dispc_free_irq);
>  
> +/*
> + * Work-a-round for errata i734 in DSS dispc
> + *  - LCD1 Gamma Correction Is Not Working When GFX Pipe Is Disabled
> + *
> + * For gamma tables to work on LCD1 the GFX plane has to be used at
> + * least once after DSS HW has come out of reset. The work-a-round
> + * sets up a minimal LCD setup with GFX plane and waits for one
> + * vertical sync irq before disabling the setup and continuing with
> + * the context restore. The physical outputs are gated during the
> + * operation. This work-a-round requires that gamma table's LOADMODE
> + * is set to 0x2 in DISPC_CONTROL1 register.
> + *
> + * For details see:
> + * OMAP543x Multimedia Device Silicon Revision 2.0 Silicon Errata
> + * Literature Number: SWPZ037E
> + * Or some other relevant errata document for the DSS IP version.
> + */
> +
> +static struct dispc_errata_i734_data {
> +	struct omap_video_timings timings;
> +	struct omap_overlay_info ovli;
> +	struct omap_overlay_manager_info mgri;
> +	struct dss_lcd_mgr_config lcd_conf;
> +	struct i734_buf {
> +		dma_addr_t paddr;
> +		void *vaddr;
> +	} buf;

It might be good to separate the setup data to a const struct, and the
buffer data to a mutable one.

You need to set the ovl info base address, but you could just create a
local copy of the const ovl_info when doing the setup.

> +} i734 = {
> +	.timings = {
> +		.x_res = 8, .y_res = 1,
> +		.pixelclock = 16000000,
> +		.hsw = 8, .hfp = 4, .hbp = 4,
> +		.vsw = 1, .vfp = 1, .vbp = 1,
> +		.vsync_level = OMAPDSS_SIG_ACTIVE_LOW,
> +		.hsync_level = OMAPDSS_SIG_ACTIVE_LOW,
> +		.interlace = false,
> +		.data_pclk_edge = OMAPDSS_DRIVE_SIG_RISING_EDGE,
> +		.de_level = OMAPDSS_SIG_ACTIVE_HIGH,
> +		.sync_pclk_edge = OMAPDSS_DRIVE_SIG_RISING_EDGE,
> +		.double_pixel = false,
> +	},
> +	.ovli = {
> +		.screen_width = 1,
> +		.width = 1, .height = 1,
> +		.color_mode = OMAP_DSS_COLOR_RGB24U,
> +		.rotation = OMAP_DSS_ROT_0,
> +		.rotation_type = OMAP_DSS_ROT_DMA,
> +		.mirror = 0,
> +		.pos_x = 0, .pos_y = 0,
> +		.out_width = 0, .out_height = 0,
> +		.global_alpha = 0xff,
> +		.pre_mult_alpha = 0,
> +		.zorder = 0,
> +	},
> +	.mgri = {
> +		.default_color = 0,
> +		.trans_enabled = false,
> +		.partial_alpha_enabled = false,
> +		.cpr_enable = false,
> +	},
> +	.lcd_conf = {
> +		.io_pad_mode = DSS_IO_PAD_MODE_BYPASS,
> +		.stallmode = false,
> +		.fifohandcheck = false,
> +		.clock_info = {
> +			.lck_div = 1,
> +			.pck_div = 2,
> +		},
> +		.video_port_width = 24,
> +		.lcden_sig_polarity = 0,
> +	},
> +};
> +
> +static int dispc_errata_i734_wa_init(void)
> +{
> +	size_t buff_size = i734.ovli.width * i734.ovli.height *

"buf_size" would match the other names =).

Usually I try to do only simple initializations when declaring the
locals. This is a bit on the complex side, and also there's a check
below that can make all this init calculation not needed.

> +		color_mode_to_bpp(i734.ovli.color_mode) / 8;
> +
> +	if (!dss_has_feature(FEAT_GAMMA_I734_BUG))
> +		return 0;
> +
> +	i734.buf.vaddr = dma_alloc_writecombine(&dispc.pdev->dev, buff_size,
> +						&i734.buf.paddr, GFP_KERNEL);
> +	if (WARN_ON(IS_ERR(i734.buf.vaddr)))
> +		return PTR_ERR(i734.buf.vaddr);

I think a plain dev_err() would be enough here, not WARN.

> +
> +	i734.ovli.paddr = i734.buf.paddr;
> +
> +	return 0;
> +}
> +
> +static void dispc_errata_i734_wa_fini(void)
> +{
> +	size_t buff_size = i734.ovli.width * i734.ovli.height *
> +		color_mode_to_bpp(i734.ovli.color_mode) / 8;
> +
> +	if (!dss_has_feature(FEAT_GAMMA_I734_BUG))
> +		return;
> +
> +	dma_free_writecombine(&dispc.pdev->dev, buff_size, i734.buf.vaddr,
> +			      i734.buf.paddr);
> +}
> +
> +static void dispc_errata_i734_wa(void)
> +{
> +	u32 vsync_irq = dispc_mgr_get_vsync_irq(OMAP_DSS_CHANNEL_LCD);
> +	u32 framedone_irq = dispc_mgr_get_framedone_irq(OMAP_DSS_CHANNEL_LCD);
> +	u32 gatestate = REG_GET(DISPC_CONTROL, 8, 4);

Here too, too complex initializations. Especially doing register
reads/writes should be something done in the "real" code. The bits you
read might not even be valid on some platforms which don't have the i734.

> +	unsigned int count;
> +
> +	if (!dss_has_feature(FEAT_GAMMA_I734_BUG))
> +		return;
> +
> +	/* Gate all LCD1 outputs */
> +	REG_FLD_MOD(DISPC_CONTROL, 0, 8, 4);

This ungates the outputs. Or would, if it used the right register, but
it's using wrong register...

> +	/* Setup and enable GFX plane */
> +	dispc_ovl_set_channel_out(OMAP_DSS_GFX, OMAP_DSS_CHANNEL_LCD);
> +	dispc_ovl_setup(OMAP_DSS_GFX, &i734.ovli, false, &i734.timings, false);
> +	dispc_ovl_enable(OMAP_DSS_GFX, true);
> +
> +	/* Set up and enable display manager for LCD1 */
> +	dispc_mgr_setup(OMAP_DSS_CHANNEL_LCD, &i734.mgri);
> +	dispc_calc_clock_rates(dss_get_dispc_clk_rate(),
> +			       &i734.lcd_conf.clock_info);
> +	dispc_mgr_set_lcd_config(OMAP_DSS_CHANNEL_LCD, &i734.lcd_conf);
> +	dispc_mgr_set_timings(OMAP_DSS_CHANNEL_LCD, &i734.timings);
> +	dispc_mgr_enable(OMAP_DSS_CHANNEL_LCD, true);

When you enable the LCD channel, it starts sending the stream. So...

> +	dispc_clear_irqstatus(vsync_irq | framedone_irq);

...clearing the irqstatus should be done before enabling the output.

> +	/* Busy wait for vsync (we can't fiddle with irq handlers in resume) */
> +	count = 0;
> +	while (!(dispc_read_irqstatus() & vsync_irq))
> +		if (count++ > 10000) {
> +			dev_err(&dispc.pdev->dev, "%s: vsync timeout\n",
> +				__func__);
> +			break;
> +		}
> +
> +	/* Shut down the WA setup */
> +	dispc_mgr_enable(OMAP_DSS_CHANNEL_LCD, false);

You can disable the LCD right after enabling it. What happens is that
when you set the enable bit, DISPC starts the output. When you clear the
enable bit, DISPC will stop at the end of the next frame.

In other words, you don't need to wait for vsync at all, just framedone.
That is, presuming it's enough for the WA to work. I think it is, as it
does send a single frame.

> +	dispc_ovl_enable(OMAP_DSS_GFX, false);
> +	count = 0;
> +	while (!(dispc_read_irqstatus() & framedone_irq))
> +		if (count++ > 10000) {
> +			dev_err(&dispc.pdev->dev, "%s: framedone timeout\n",
> +				__func__);
> +			break;
> +		}
> +
> +	/* Clear all irq bits before continuing */
> +	dispc_clear_irqstatus(0xffffffff);
> +
> +	/* Restore the original state to LCD1 output gates */
> +	REG_FLD_MOD(DISPC_CONTROL, gatestate, 8, 4);

Still wrong register =).

> +}
> +
>  /* DISPC HW IP initialisation */
>  static int dispc_bind(struct device *dev, struct device *master, void *data)
>  {
> @@ -4214,6 +4372,10 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
>  
>  	spin_lock_init(&dispc.control_lock);
>  
> +	r = dispc_errata_i734_wa_init();
> +	if (r)
> +		return r;
> +
>  	r = dispc_init_features(dispc.pdev);
>  	if (r)
>  		return r;
> @@ -4282,6 +4444,8 @@ static void dispc_unbind(struct device *dev, struct device *master,
>  			       void *data)
>  {
>  	pm_runtime_disable(dev);
> +
> +	dispc_errata_i734_wa_fini();
>  }
>  
>  static const struct component_ops dispc_component_ops = {
> @@ -4324,6 +4488,8 @@ static int dispc_runtime_resume(struct device *dev)
>  	if (REG_GET(DISPC_CONFIG, 2, 1) != OMAP_DSS_LOAD_FRAME_ONLY) {
>  		_omap_dispc_initial_config();
>  
> +		dispc_errata_i734_wa();
> +
>  		dispc_restore_context();
>  
>  		dispc_restore_gamma_tables();
> diff --git a/drivers/gpu/drm/omapdrm/dss/dss_features.c b/drivers/gpu/drm/omapdrm/dss/dss_features.c
> index f77b1c5..3127bd6 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dss_features.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dss_features.c
> @@ -594,6 +594,7 @@ static const enum dss_feat_id omap4_dss_feat_list[] = {
>  	FEAT_FIFO_MERGE,
>  	FEAT_BURST_2D,
>  	FEAT_GAMMA_TABLE,
> +	FEAT_GAMMA_I734_BUG,

This, too, should be in dispc features.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/omapdrm: Add gamma table support to DSS dispc

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 570 bytes --]

On 20/05/16 16:14, Jyri Sarha wrote:
> On 05/20/16 15:51, Tomi Valkeinen wrote:
>>> +u32 dispc_mgr_gamma_size(enum omap_channel channel)
>>>> +{
>>>> +	const struct dispc_gamma_desc *gdesc = &mgr_desc[channel].gamma;
>>>> +
>>>> +	if (!dss_has_feature(FEAT_GAMMA_TABLE))
>>>> +		return 0;
>>>> +
>> This should probably check the availability of LCD2/LCD3?
>>
> 
> That would be a double check (the function should never be called with
> channels that are not there), but I am happy to add it.
> 

True, I think we can trust omapdrm here =).

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/omapdrm: Work-a-round for errata i734 (LCD1 Gamma) in DSS dispc

[Jyri Sarha]

[-- Attachment #1.1.1: Type: text/plain, Size: 1510 bytes --]

On 05/20/16 16:21, Tomi Valkeinen wrote:
>> > +
>> > +static int dispc_errata_i734_wa_init(void)
>> > +{
>> > +	size_t buff_size = i734.ovli.width * i734.ovli.height *
> "buf_size" would match the other names =).
> 
> Usually I try to do only simple initializations when declaring the
> locals. This is a bit on the complex side, and also there's a check
> below that can make all this init calculation not needed.
> 

Compiler optimizations should delay the initialization to happen only if
it is needed.

I usually try to keep the functions as compact as possible so that they
fit on the screen at once. But I think it is better to move the size
also inside the struct i734_buf so it we only need to calculate the size
once and functions become even smaller :).

...
>> > +
>> > +static void dispc_errata_i734_wa(void)
>> > +{
>> > +	u32 vsync_irq = dispc_mgr_get_vsync_irq(OMAP_DSS_CHANNEL_LCD);
>> > +	u32 framedone_irq = dispc_mgr_get_framedone_irq(OMAP_DSS_CHANNEL_LCD);
>> > +	u32 gatestate = REG_GET(DISPC_CONTROL, 8, 4);
> Here too, too complex initializations. Especially doing register
> reads/writes should be something done in the "real" code. The bits you
> read might not even be valid on some platforms which don't have the i734.
> 

You are right about the register read. Compiler should not optimize the
register access. But - but with your permission - I'll keep the IRQ mask
initializations as they are quite constant in nature anyway.

BR,
Jyri




[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/omapdrm: Work-a-round for errata i734 (LCD1 Gamma) in DSS dispc

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1843 bytes --]


On 23/05/16 12:50, Jyri Sarha wrote:
> On 05/20/16 16:21, Tomi Valkeinen wrote:
>>>> +
>>>> +static int dispc_errata_i734_wa_init(void)
>>>> +{
>>>> +	size_t buff_size = i734.ovli.width * i734.ovli.height *
>> "buf_size" would match the other names =).
>>
>> Usually I try to do only simple initializations when declaring the
>> locals. This is a bit on the complex side, and also there's a check
>> below that can make all this init calculation not needed.
>>
> 
> Compiler optimizations should delay the initialization to happen only if
> it is needed.

That is true.

My worry is more on the functional side: if you do, say, calculations
there and, e.g. do a division, it's easy to get division by zero if all
the variables are not actually valid. Or if you do get_foo(channel), and
get_foo() only works for certain channels, again you could hit a crash.

Those kind of things happen easily when you have code like:

int foo = get_foo(channel);

if (channel != XYZ)
	return;

>>>> +
>>>> +static void dispc_errata_i734_wa(void)
>>>> +{
>>>> +	u32 vsync_irq = dispc_mgr_get_vsync_irq(OMAP_DSS_CHANNEL_LCD);
>>>> +	u32 framedone_irq = dispc_mgr_get_framedone_irq(OMAP_DSS_CHANNEL_LCD);
>>>> +	u32 gatestate = REG_GET(DISPC_CONTROL, 8, 4);
>> Here too, too complex initializations. Especially doing register
>> reads/writes should be something done in the "real" code. The bits you
>> read might not even be valid on some platforms which don't have the i734.
>>
> 
> You are right about the register read. Compiler should not optimize the
> register access. But - but with your permission - I'll keep the IRQ mask
> initializations as they are quite constant in nature anyway.

Yes, those should be fine. Presuming all DSS versions have CHANNEL_LCD,
which happens to be the case =).

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel