[PATCH v3 07/24] drm/i2c: tda998x: set the video mode from the adjusted value

Subsystems: drm drivers, drm drivers and misc gpu patches, the rest

STALE4603d

5 messages, 3 authors, 2014-01-28 · open the first message on its own page

[PATCH v3 07/24] drm/i2c: tda998x: set the video mode from the adjusted value

From: Jean-Francois Moine <hidden>
Date: 2014-01-19 19:00:51

This patch uses always the adjusted video mode instead of a mix of
original and adjusted mode.

Signed-off-by: Jean-Francois Moine <redacted>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 66 +++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index b688801..5d82301 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -773,7 +773,7 @@ tda998x_encoder_mode_valid(struct drm_encoder *encoder,
 static void
 tda998x_encoder_mode_set(struct drm_encoder *encoder,
 			struct drm_display_mode *mode,
-			struct drm_display_mode *adjusted_mode)
+			struct drm_display_mode *adj_mode)
 {
 	struct tda998x_priv *priv = to_tda998x_priv(encoder);
 	uint16_t ref_pix, ref_line, n_pix, n_line;
@@ -802,13 +802,13 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * So we add +1 to all horizontal and vertical register values,
 	 * plus an additional +3 for REFPIX as we are using RGB input only.
 	 */
-	n_pix        = mode->htotal;
-	n_line       = mode->vtotal;
+	n_pix        = adj_mode->htotal;
+	n_line       = adj_mode->vtotal;
 
-	hs_pix_e     = mode->hsync_end - mode->hdisplay;
-	hs_pix_s     = mode->hsync_start - mode->hdisplay;
-	de_pix_e     = mode->htotal;
-	de_pix_s     = mode->htotal - mode->hdisplay;
+	hs_pix_e     = adj_mode->hsync_end - adj_mode->hdisplay;
+	hs_pix_s     = adj_mode->hsync_start - adj_mode->hdisplay;
+	de_pix_e     = adj_mode->htotal;
+	de_pix_s     = adj_mode->htotal - adj_mode->hdisplay;
 	ref_pix      = 3 + hs_pix_s;
 
 	/*
@@ -816,37 +816,38 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * those to adjust the position of the rising VS edge by adding
 	 * HSKEW to ref_pix.
 	 */
-	if (adjusted_mode->flags & DRM_MODE_FLAG_HSKEW)
-		ref_pix += adjusted_mode->hskew;
+	if (adj_mode->flags & DRM_MODE_FLAG_HSKEW)
+		ref_pix += adj_mode->hskew;
 
-	if ((mode->flags & DRM_MODE_FLAG_INTERLACE) == 0) {
-		ref_line     = 1 + mode->vsync_start - mode->vdisplay;
-		vwin1_line_s = mode->vtotal - mode->vdisplay - 1;
-		vwin1_line_e = vwin1_line_s + mode->vdisplay;
+	if ((adj_mode->flags & DRM_MODE_FLAG_INTERLACE) == 0) {
+		ref_line     = 1 + adj_mode->vsync_start - adj_mode->vdisplay;
+		vwin1_line_s = adj_mode->vtotal - adj_mode->vdisplay - 1;
+		vwin1_line_e = vwin1_line_s + adj_mode->vdisplay;
 		vs1_pix_s    = vs1_pix_e = hs_pix_s;
-		vs1_line_s   = mode->vsync_start - mode->vdisplay;
+		vs1_line_s   = adj_mode->vsync_start - adj_mode->vdisplay;
 		vs1_line_e   = vs1_line_s +
-			       mode->vsync_end - mode->vsync_start;
+			       adj_mode->vsync_end - adj_mode->vsync_start;
 		vwin2_line_s = vwin2_line_e = 0;
 		vs2_pix_s    = vs2_pix_e  = 0;
 		vs2_line_s   = vs2_line_e = 0;
 	} else {
-		ref_line     = 1 + (mode->vsync_start - mode->vdisplay)/2;
-		vwin1_line_s = (mode->vtotal - mode->vdisplay)/2;
-		vwin1_line_e = vwin1_line_s + mode->vdisplay/2;
+		ref_line     = 1 + (adj_mode->vsync_start -
+						adj_mode->vdisplay)/2;
+		vwin1_line_s = (adj_mode->vtotal - adj_mode->vdisplay)/2;
+		vwin1_line_e = vwin1_line_s + adj_mode->vdisplay/2;
 		vs1_pix_s    = vs1_pix_e = hs_pix_s;
-		vs1_line_s   = (mode->vsync_start - mode->vdisplay)/2;
+		vs1_line_s   = (adj_mode->vsync_start - adj_mode->vdisplay)/2;
 		vs1_line_e   = vs1_line_s +
-			       (mode->vsync_end - mode->vsync_start)/2;
-		vwin2_line_s = vwin1_line_s + mode->vtotal/2;
-		vwin2_line_e = vwin2_line_s + mode->vdisplay/2;
-		vs2_pix_s    = vs2_pix_e = hs_pix_s + mode->htotal/2;
-		vs2_line_s   = vs1_line_s + mode->vtotal/2 ;
+			       (adj_mode->vsync_end - adj_mode->vsync_start)/2;
+		vwin2_line_s = vwin1_line_s + adj_mode->vtotal/2;
+		vwin2_line_e = vwin2_line_s + adj_mode->vdisplay/2;
+		vs2_pix_s    = vs2_pix_e = hs_pix_s + adj_mode->htotal/2;
+		vs2_line_s   = vs1_line_s + adj_mode->vtotal/2 ;
 		vs2_line_e   = vs2_line_s +
-			       (mode->vsync_end - mode->vsync_start)/2;
+			       (adj_mode->vsync_end - adj_mode->vsync_start)/2;
 	}
 
-	div = 148500 / mode->clock;
+	div = 148500 / adj_mode->clock;
 
 	/* mute the audio FIFO: */
 	reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
@@ -896,9 +897,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * TDA19988 requires high-active sync at input stage,
 	 * so invert low-active sync provided by master encoder here
 	 */
-	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
 		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
-	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
 		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL);
 
 	/*
@@ -906,9 +907,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * revert input stage toggled sync at output stage
 	 */
 	reg = TBG_CNTRL_1_TGL_EN;
-	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
 		reg |= TBG_CNTRL_1_H_TGL;
-	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
 		reg |= TBG_CNTRL_1_V_TGL;
 	reg_write(priv, REG_TBG_CNTRL_1, reg);
 
@@ -949,11 +950,10 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 		reg_write(priv, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(1));
 		reg_set(priv, REG_TX33, TX33_HDMI);
 
-		tda998x_write_avi(priv, adjusted_mode);
+		tda998x_write_avi(priv, adj_mode);
 
 		if (priv->params.audio_cfg)
-			tda998x_configure_audio(priv, adjusted_mode,
-						&priv->params);
+			tda998x_configure_audio(priv, adj_mode, &priv->params);
 	}
 }
 
-- 
1.8.5.3

Re: [PATCH v3 07/24] drm/i2c: tda998x: set the video mode from the adjusted value

From: Russell King - ARM Linux <hidden>
Date: 2014-01-22 21:28:28

On Sun, Jan 19, 2014 at 07:58:40PM +0100, Jean-Francois Moine wrote:
This patch uses always the adjusted video mode instead of a mix of
original and adjusted mode.

Signed-off-by: Jean-Francois Moine <redacted>
Nothing obviously wrong and appears to work, thanks.

Acked-by: Russell King <redacted>
Tested-by: Russell King <redacted>

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [PATCH v3 07/24] drm/i2c: tda998x: set the video mode from the adjusted value

From: Darren Etheridge <hidden>
Date: 2014-01-23 23:25:55

Jean-Francois Moine [off-list ref] wrote on Sun [2014-Jan-19 19:58:40 +0100]:
quoted hunk
This patch uses always the adjusted video mode instead of a mix of
original and adjusted mode.

Signed-off-by: Jean-Francois Moine <redacted>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 66 +++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index b688801..5d82301 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -773,7 +773,7 @@ tda998x_encoder_mode_valid(struct drm_encoder *encoder,
 static void
 tda998x_encoder_mode_set(struct drm_encoder *encoder,
 			struct drm_display_mode *mode,
-			struct drm_display_mode *adjusted_mode)
+			struct drm_display_mode *adj_mode)
 {
 	struct tda998x_priv *priv = to_tda998x_priv(encoder);
 	uint16_t ref_pix, ref_line, n_pix, n_line;
@@ -802,13 +802,13 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * So we add +1 to all horizontal and vertical register values,
 	 * plus an additional +3 for REFPIX as we are using RGB input only.
 	 */
-	n_pix        = mode->htotal;
-	n_line       = mode->vtotal;
+	n_pix        = adj_mode->htotal;
+	n_line       = adj_mode->vtotal;
 
-	hs_pix_e     = mode->hsync_end - mode->hdisplay;
-	hs_pix_s     = mode->hsync_start - mode->hdisplay;
-	de_pix_e     = mode->htotal;
-	de_pix_s     = mode->htotal - mode->hdisplay;
+	hs_pix_e     = adj_mode->hsync_end - adj_mode->hdisplay;
+	hs_pix_s     = adj_mode->hsync_start - adj_mode->hdisplay;
+	de_pix_e     = adj_mode->htotal;
+	de_pix_s     = adj_mode->htotal - adj_mode->hdisplay;
 	ref_pix      = 3 + hs_pix_s;
 
 	/*
@@ -816,37 +816,38 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * those to adjust the position of the rising VS edge by adding
 	 * HSKEW to ref_pix.
 	 */
-	if (adjusted_mode->flags & DRM_MODE_FLAG_HSKEW)
-		ref_pix += adjusted_mode->hskew;
+	if (adj_mode->flags & DRM_MODE_FLAG_HSKEW)
+		ref_pix += adj_mode->hskew;
 
-	if ((mode->flags & DRM_MODE_FLAG_INTERLACE) == 0) {
-		ref_line     = 1 + mode->vsync_start - mode->vdisplay;
-		vwin1_line_s = mode->vtotal - mode->vdisplay - 1;
-		vwin1_line_e = vwin1_line_s + mode->vdisplay;
+	if ((adj_mode->flags & DRM_MODE_FLAG_INTERLACE) == 0) {
+		ref_line     = 1 + adj_mode->vsync_start - adj_mode->vdisplay;
+		vwin1_line_s = adj_mode->vtotal - adj_mode->vdisplay - 1;
+		vwin1_line_e = vwin1_line_s + adj_mode->vdisplay;
 		vs1_pix_s    = vs1_pix_e = hs_pix_s;
-		vs1_line_s   = mode->vsync_start - mode->vdisplay;
+		vs1_line_s   = adj_mode->vsync_start - adj_mode->vdisplay;
 		vs1_line_e   = vs1_line_s +
-			       mode->vsync_end - mode->vsync_start;
+			       adj_mode->vsync_end - adj_mode->vsync_start;
 		vwin2_line_s = vwin2_line_e = 0;
 		vs2_pix_s    = vs2_pix_e  = 0;
 		vs2_line_s   = vs2_line_e = 0;
 	} else {
-		ref_line     = 1 + (mode->vsync_start - mode->vdisplay)/2;
-		vwin1_line_s = (mode->vtotal - mode->vdisplay)/2;
-		vwin1_line_e = vwin1_line_s + mode->vdisplay/2;
+		ref_line     = 1 + (adj_mode->vsync_start -
+						adj_mode->vdisplay)/2;
+		vwin1_line_s = (adj_mode->vtotal - adj_mode->vdisplay)/2;
+		vwin1_line_e = vwin1_line_s + adj_mode->vdisplay/2;
 		vs1_pix_s    = vs1_pix_e = hs_pix_s;
-		vs1_line_s   = (mode->vsync_start - mode->vdisplay)/2;
+		vs1_line_s   = (adj_mode->vsync_start - adj_mode->vdisplay)/2;
 		vs1_line_e   = vs1_line_s +
-			       (mode->vsync_end - mode->vsync_start)/2;
-		vwin2_line_s = vwin1_line_s + mode->vtotal/2;
-		vwin2_line_e = vwin2_line_s + mode->vdisplay/2;
-		vs2_pix_s    = vs2_pix_e = hs_pix_s + mode->htotal/2;
-		vs2_line_s   = vs1_line_s + mode->vtotal/2 ;
+			       (adj_mode->vsync_end - adj_mode->vsync_start)/2;
+		vwin2_line_s = vwin1_line_s + adj_mode->vtotal/2;
+		vwin2_line_e = vwin2_line_s + adj_mode->vdisplay/2;
+		vs2_pix_s    = vs2_pix_e = hs_pix_s + adj_mode->htotal/2;
+		vs2_line_s   = vs1_line_s + adj_mode->vtotal/2 ;
 		vs2_line_e   = vs2_line_s +
-			       (mode->vsync_end - mode->vsync_start)/2;
+			       (adj_mode->vsync_end - adj_mode->vsync_start)/2;
 	}
 
-	div = 148500 / mode->clock;
+	div = 148500 / adj_mode->clock;
 
 	/* mute the audio FIFO: */
 	reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
@@ -896,9 +897,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * TDA19988 requires high-active sync at input stage,
 	 * so invert low-active sync provided by master encoder here
 	 */
-	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
 		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
-	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
 		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL);
 
Using the adj_mode->flags breaks a workaround I had done on BeagleBone Black
(tilcdc + tda998x) to resolve an issue with out of spec syncs from the
tlcdc.  I invert the HSYNC in adj_mode->flags but don't want the tda998x to
really know that I am doing that so I use adj_mode in the tilcdc driver, and
mode here in the tda998x driver.  The theory being adj_mode contains whatever
workarounds I need to do for the driving device and mode has the pristine
values that I want to send to the monitor.  I would need to look if there is a
different way to solve this as I am guessing you are actually using adj_mode in
the manner it was intended.

Otherwise this patch series is working on BeagleBone Black - I have only tried
video so far (not audio).

Darren
quoted hunk
 	/*
@@ -906,9 +907,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * revert input stage toggled sync at output stage
 	 */
 	reg = TBG_CNTRL_1_TGL_EN;
-	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
 		reg |= TBG_CNTRL_1_H_TGL;
-	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
 		reg |= TBG_CNTRL_1_V_TGL;
 	reg_write(priv, REG_TBG_CNTRL_1, reg);
 
@@ -949,11 +950,10 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 		reg_write(priv, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(1));
 		reg_set(priv, REG_TX33, TX33_HDMI);
 
-		tda998x_write_avi(priv, adjusted_mode);
+		tda998x_write_avi(priv, adj_mode);
 
 		if (priv->params.audio_cfg)
-			tda998x_configure_audio(priv, adjusted_mode,
-						&priv->params);
+			tda998x_configure_audio(priv, adj_mode, &priv->params);
 	}
 }
 
-- 
1.8.5.3

_______________________________________________
dri-devel mailing list
dri-devel at lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 07/24] drm/i2c: tda998x: set the video mode from the adjusted value

From: Jean-Francois Moine <hidden>
Date: 2014-01-28 17:12:14

On Thu, 23 Jan 2014 17:29:07 -0600
Darren Etheridge [off-list ref] wrote:
quoted
@@ -896,9 +897,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * TDA19988 requires high-active sync at input stage,
 	 * so invert low-active sync provided by master encoder here
 	 */
-	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
 		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
-	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
 		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL);
   
Using the adj_mode->flags breaks a workaround I had done on BeagleBone Black
(tilcdc + tda998x) to resolve an issue with out of spec syncs from the
tlcdc.  I invert the HSYNC in adj_mode->flags but don't want the tda998x to
really know that I am doing that so I use adj_mode in the tilcdc driver, and
mode here in the tda998x driver.  The theory being adj_mode contains whatever
workarounds I need to do for the driving device and mode has the pristine
values that I want to send to the monitor.  I would need to look if there is a
different way to solve this as I am guessing you are actually using adj_mode in
the manner it was intended.
No. In fact, I just wanted the function to use only one mode.

Looking at the other drivers, it seems that they don't touch the
adjusted_mode, so, for the Cubox, mode and adjusted_mode have same
values.

I will do an other patch so that you will not have to touch the tilcdc
driver.

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v3 07/24] drm/i2c: tda998x: set the video mode from the adjusted value

From: Darren Etheridge <hidden>
Date: 2014-01-28 19:20:41

Jean-Francois Moine [off-list ref] wrote on Tue [2014-Jan-28 18:12:18 +0100]:
On Thu, 23 Jan 2014 17:29:07 -0600
Darren Etheridge [off-list ref] wrote:
quoted
quoted
@@ -896,9 +897,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * TDA19988 requires high-active sync at input stage,
 	 * so invert low-active sync provided by master encoder here
 	 */
-	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
 		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
-	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
 		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL);
   
Using the adj_mode->flags breaks a workaround I had done on BeagleBone Black
(tilcdc + tda998x) to resolve an issue with out of spec syncs from the
tlcdc.  I invert the HSYNC in adj_mode->flags but don't want the tda998x to
really know that I am doing that so I use adj_mode in the tilcdc driver, and
mode here in the tda998x driver.  The theory being adj_mode contains whatever
workarounds I need to do for the driving device and mode has the pristine
values that I want to send to the monitor.  I would need to look if there is a
different way to solve this as I am guessing you are actually using adj_mode in
the manner it was intended.
No. In fact, I just wanted the function to use only one mode.

Looking at the other drivers, it seems that they don't touch the
adjusted_mode, so, for the Cubox, mode and adjusted_mode have same
values.

I will do an other patch so that you will not have to touch the tilcdc
driver.
Thanks, that would certainly be the easiest path to avoid the regression given
the other drivers that use tda998x don't currently use adj_mode.  However I
don't disagree with your reasoning, it would make sense to only use values
from one of the strctures and not a mix and match.  Anybody looking at only
the tda998x driver would be confused :) I'll work on finding a different
solution for this.
-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help