Patchwork [v2,15/28] drm/i2c: tda998x: use the tda998x video format when cea mode

login
register
mail settings
Submitter Jean-Francois Moine
Date Jan. 9, 2014, 11:04 a.m.
Message ID <20140109120448.24d89eba@armhf>
Download mbox | patch
Permalink /patch/308605/
State New
Headers show

Comments

Jean-Francois Moine - Jan. 9, 2014, 11:04 a.m.
This patch uses the tda998x video format tied to the CEA mode.
This reduces the number of i2c exchanges.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c   | 143 +++++++++++++++++----
 1 file changed, 118 insertions(+), 25 deletions(-)
Russell King - ARM Linux - Jan. 11, 2014, 6:18 p.m.
On Thu, Jan 09, 2014 at 12:04:48PM +0100, Jean-Francois Moine wrote:
> This patch uses the tda998x video format tied to the CEA mode.
> This reduces the number of i2c exchanges.

It is my understanding that one of the major design goals of this driver
is to avoid the use of such a table, and therefore this change is
undesirable by the original driver authors.  This is more a comment for
Rob to pick up on.
Jean-Francois Moine - Jan. 13, 2014, 4:07 p.m.
On Sat, 11 Jan 2014 18:18:32 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Thu, Jan 09, 2014 at 12:04:48PM +0100, Jean-Francois Moine wrote:
> > This patch uses the tda998x video format tied to the CEA mode.
> > This reduces the number of i2c exchanges.
> 
> It is my understanding that one of the major design goals of this driver
> is to avoid the use of such a table, and therefore this change is
> undesirable by the original driver authors.  This is more a comment for
> Rob to pick up on.

I retrieved a message from Rabeeh who got this description from NXP
(http://www.solid-run.com/phpbb/viewtopic.php?f=9&t=1205&start=10
 Thu Mar 28, 2013 10:59 am):

1. write register FF of CEC device to &h87.
                (68ff = 87h)           -> activate all clock tree inc HDMI ones so you can then use HDMI device
2. select page 00 of HDMI device :  write 00 in FF
                (e0ff =00h)
3. write register A0 of HDMI device to select predefined format :
                (e0a0 = 06h)       -> select a predefined 1080p60 format
4. write registers A5 and A6 with total number of pixels per line (or greater value)
                (e0a5 = 08h)
                (e0a6 = 97h) for example
5. write register TBG_CTRL1 of HDMI device to select resync method
                (e0cb = 7ch)        -> select
6. write registers VIP_CTRL to configure RGB input ports :
                (e020 = 45h)       -> select VP port configuration
                (e021 = 01h)
                (e022 = 23h)
7. write register VIP_CTRL3 to 20h vto select external SP sync method
                (e023 = 20h)
Russell King - ARM Linux - Jan. 13, 2014, 4:13 p.m.
On Mon, Jan 13, 2014 at 05:07:07PM +0100, Jean-Francois Moine wrote:
> On Sat, 11 Jan 2014 18:18:32 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > On Thu, Jan 09, 2014 at 12:04:48PM +0100, Jean-Francois Moine wrote:
> > > This patch uses the tda998x video format tied to the CEA mode.
> > > This reduces the number of i2c exchanges.
> > 
> > It is my understanding that one of the major design goals of this driver
> > is to avoid the use of such a table, and therefore this change is
> > undesirable by the original driver authors.  This is more a comment for
> > Rob to pick up on.
> 
> I retrieved a message from Rabeeh who got this description from NXP
> (http://www.solid-run.com/phpbb/viewtopic.php?f=9&t=1205&start=10
>  Thu Mar 28, 2013 10:59 am):
> 
> 1. write register FF of CEC device to &h87.
>                 (68ff = 87h)           -> activate all clock tree inc HDMI ones so you can then use HDMI device
> 2. select page 00 of HDMI device :  write 00 in FF
>                 (e0ff =00h)
> 3. write register A0 of HDMI device to select predefined format :
>                 (e0a0 = 06h)       -> select a predefined 1080p60 format
> 4. write registers A5 and A6 with total number of pixels per line (or greater value)
>                 (e0a5 = 08h)
>                 (e0a6 = 97h) for example
> 5. write register TBG_CTRL1 of HDMI device to select resync method
>                 (e0cb = 7ch)        -> select
> 6. write registers VIP_CTRL to configure RGB input ports :
>                 (e020 = 45h)       -> select VP port configuration
>                 (e021 = 01h)
>                 (e022 = 23h)
> 7. write register VIP_CTRL3 to 20h vto select external SP sync method
>                 (e023 = 20h)

Sorry, I don't see what bearing your response to my reply has.
Jean-Francois Moine - Jan. 13, 2014, 4:22 p.m.
On Mon, 13 Jan 2014 16:13:34 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> Sorry, I don't see what bearing your response to my reply has.

Simply that NXP people better like to use the cea mode.
Russell King - ARM Linux - Jan. 13, 2014, 4:35 p.m.
On Mon, Jan 13, 2014 at 05:22:20PM +0100, Jean-Francois Moine wrote:
> On Mon, 13 Jan 2014 16:13:34 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > Sorry, I don't see what bearing your response to my reply has.
> 
> Simply that NXP people better like to use the cea mode.

Ah, I see.  It's a thread about your DRM driver, and about the lack of
audio on 1080 resolutions.  Taking it in context, the reported issue
appears to be lack of video when 1080p is selected, but the display
doesn't support 1080p.  That's hardly surprising.

Then rabeeh posts his message about lack of audio at 1920x1080.

Well, I can say that with the tda998x as it stands, the 1080* modes
work with audio with armada-drm, so there's no need to change this.
Jean-Francois Moine - Jan. 13, 2014, 5:24 p.m.
On Mon, 13 Jan 2014 16:35:01 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> Ah, I see.  It's a thread about your DRM driver, and about the lack of
> audio on 1080 resolutions.  Taking it in context, the reported issue
> appears to be lack of video when 1080p is selected, but the display
> doesn't support 1080p.  That's hardly surprising.
> 
> Then rabeeh posts his message about lack of audio at 1920x1080.
> 
> Well, I can say that with the tda998x as it stands, the 1080* modes
> work with audio with armada-drm, so there's no need to change this.

My DRM driver also has audio with and without the cea mode for all
resolutions. At the time of Rabeeh's message, I just had forgot to
switch the audio pins...

So, TDA998x CEA mode or not?
Russell King - ARM Linux - Jan. 13, 2014, 5:46 p.m.
On Mon, Jan 13, 2014 at 06:24:55PM +0100, Jean-Francois Moine wrote:
> On Mon, 13 Jan 2014 16:35:01 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > Ah, I see.  It's a thread about your DRM driver, and about the lack of
> > audio on 1080 resolutions.  Taking it in context, the reported issue
> > appears to be lack of video when 1080p is selected, but the display
> > doesn't support 1080p.  That's hardly surprising.
> > 
> > Then rabeeh posts his message about lack of audio at 1920x1080.
> > 
> > Well, I can say that with the tda998x as it stands, the 1080* modes
> > work with audio with armada-drm, so there's no need to change this.
> 
> My DRM driver also has audio with and without the cea mode for all
> resolutions. At the time of Rabeeh's message, I just had forgot to
> switch the audio pins...
> 
> So, TDA998x CEA mode or not?

Firstly, why change something which isn't broken?

Secondly, you need to justify this change by explaining why it is
beneficial to use the internally programmed modes rather than programming
the timings to match the adjusted settings from DRM.

Thirdly, one of your previous patches changed the programming to use the
adjusted mode settings, which may be the CEA mode with slight tweaks.
Have you considered the implication of the hardware generating the video
being programmed with those tweaks vs the encoder using a fixed set of
unadjusted timings?

I'm not convinced that we need this patch at the present time, and I'm
not convinced that we need to introduce a load of tables for no
measurable gain.

Patch

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 0cae820..91bd4e8 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -344,6 +344,85 @@  struct tda998x_priv {
 #define TDA19989N2                0x0202
 #define TDA19988                  0x0301
 
+/* REG_VIDFORMAT values */
+enum e_vfmt {				/* cea mode */
+	E_VFMT_INVALID = 0xff,
+	E_VFMT_640x480p_60Hz = 0,	/* 1 */
+	E_VFMT_720x480p_60Hz,		/* 2/3 */
+	E_VFMT_1280x720p_60Hz,		/* 4 */
+	E_VFMT_1920x1080i_60Hz,		/* 5 */
+	E_VFMT_720x480i_60Hz,		/* 6/7 */
+	E_VFMT_720x240p_60Hz,		/*NT 8/9 */
+	E_VFMT_1920x1080p_60Hz,		/* 16 */
+	E_VFMT_720x576p_50Hz,		/* 17/18 */
+	E_VFMT_1280x720p_50Hz,		/* 19 */
+	E_VFMT_1920x1080i_50Hz,		/* 20 */
+	E_VFMT_720x576i_50Hz,		/* 21/22 */
+	E_VFMT_720x288p_50Hz,		/* 23/24 */
+	E_VFMT_1920x1080p_50Hz,		/* 31 */
+	E_VFMT_1920x1080p_24Hz,		/* 32 */
+	E_VFMT_1440x576p_50Hz,		/* 29/30 */
+	E_VFMT_1440x480p_60Hz,		/* 14/15 */
+	E_VFMT_2880x480p_60Hz,		/* 35/36 */
+	E_VFMT_2880x576p_50Hz,		/* 37/38 */
+	E_VFMT_2880x480i_60Hz,		/* 10/11*/
+	E_VFMT_2880x480i_60Hz_PR2,	/* 10/11*/
+	E_VFMT_2880x480i_60Hz_PR4,	/* 10/11*/
+	E_VFMT_2880x576i_50Hz,		/* 25/26 */
+	E_VFMT_2880x576i_50Hz_PR2,	/* 25/26 */
+	E_VFMT_720x480p_60Hz_FP,	/* 2/3 FP */
+	E_VFMT_1280x720p_60Hz_FP,	/* 4 FP */
+	E_VFMT_720x576p_50Hz_FP,	/* 17/18 FP */
+	E_VFMT_1280x720p_50Hz_FP,	/* 19 FP */
+	E_VFMT_1920x1080p_24Hz_FP,	/* 32 FP */
+	E_VFMT_1920x1080p_25Hz_FP,	/* 33 FP */
+	E_VFMT_1920x1080p_30Hz_FP,	/* 34 FP */
+	E_VFMT_1920x1080i_60Hz_FP,	/* 5 FP */
+	E_VFMT_1920x1080i_50Hz_FP,	/* 20 FP */
+};
+/* cea mode to VIDFORMAT register */
+static u8 cea2vid[] = {
+	E_VFMT_INVALID,		/* 00 */
+	E_VFMT_640x480p_60Hz,	/* 01_640x480p_60Hz */
+	E_VFMT_720x480p_60Hz,	/* 02_720x480p_60Hz */
+	E_VFMT_720x480p_60Hz,	/* 03_720x480p_60Hz */
+	E_VFMT_1280x720p_60Hz,	/* 04_1280x720p_60Hz */
+	E_VFMT_1920x1080i_60Hz,	/* 05_1920x1080i_60Hz */
+	E_VFMT_720x480i_60Hz,	/* 06_720x480i_60Hz */
+	E_VFMT_720x480i_60Hz,	/* 07_720x480i_60Hz */
+	E_VFMT_720x240p_60Hz,	/* 08_720x240p_60Hz */
+	E_VFMT_720x240p_60Hz,	/* 09_720x240p_60Hz */
+	E_VFMT_2880x480i_60Hz_PR4, /* 10_720x480i_60Hz */
+	E_VFMT_2880x480i_60Hz_PR4, /* 11_720x480i_60Hz */
+	E_VFMT_INVALID,		/* 12 */
+	E_VFMT_INVALID,		/* 13 */
+	E_VFMT_1440x480p_60Hz,	/* 14_1440x480p_60Hz */
+	E_VFMT_1440x480p_60Hz,	/* 15_1440x480p_60Hz */
+	E_VFMT_1920x1080p_60Hz,	/* 16_1920x1080p_60Hz */
+	E_VFMT_720x576p_50Hz,	/* 17_720x576p_50Hz */
+	E_VFMT_720x576p_50Hz,	/* 18_720x576p_50Hz */
+	E_VFMT_1280x720p_50Hz,	/* 19_1280x720p_50Hz */
+	E_VFMT_1920x1080i_50Hz,	/* 20_1920x1080i_50Hz */
+	E_VFMT_720x576i_50Hz,	/* 21_720x576i_50Hz */
+	E_VFMT_720x576i_50Hz,	/* 22_720x576i_50Hz */
+	E_VFMT_720x288p_50Hz,	/* 23_720x288p_50Hz */
+	E_VFMT_720x288p_50Hz,	/* 24_720x288p_50Hz */
+	E_VFMT_2880x576i_50Hz,	/* 25_720x576i_50Hz */
+	E_VFMT_2880x576i_50Hz,	/* 26_720x576i_50Hz */
+	E_VFMT_INVALID,		/* 27 */
+	E_VFMT_INVALID,		/* 28 */
+	E_VFMT_1440x576p_50Hz,	/* 29_1440x576p_50Hz */
+	E_VFMT_1440x576p_50Hz,	/* 30_1440x576p_50Hz */
+	E_VFMT_1920x1080p_50Hz,	/* 31_1920x1080p_50Hz */
+	E_VFMT_1920x1080p_24Hz,	/* 32_1920x1080p_24Hz */
+	E_VFMT_INVALID,		/* 33_1920x1080p_25Hz */
+	E_VFMT_INVALID,		/* 34_1920x1080p_30Hz */
+	E_VFMT_2880x480p_60Hz,	/* 35_2880x480p_60Hz */
+	E_VFMT_2880x480p_60Hz,	/* 36_2880x480p_60Hz */
+	E_VFMT_720x576p_50Hz,	/* 37_2880x576p_50Hz */
+	E_VFMT_720x576p_50Hz,	/* 38_2880x576p_50Hz */
+};
+
 static void
 cec_write(struct tda998x_priv *priv, uint16_t addr, uint8_t val)
 {
@@ -723,6 +802,7 @@  tda998x_configure_audio(struct tda998x_priv *priv,
 
 /* DRM encoder functions */
 
+/* this function is not called when no info->platform (DT support) */
 static void
 tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
 {
@@ -826,6 +906,7 @@  tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	uint16_t vwin2_line_s, vwin2_line_e;
 	uint16_t de_pix_s, de_pix_e;
 	uint8_t reg, div, rep;
+	u8 cea_mode;
 
 	mode = adjusted_mode;
 
@@ -946,31 +1027,43 @@  tda998x_encoder_mode_set(struct drm_encoder *encoder,
 		reg |= VIP_CNTRL_3_V_TGL;
 	reg_write(priv, REG_VIP_CNTRL_3, reg);
 
-	reg_write(priv, REG_VIDFORMAT, 0x00);
-	reg_write16(priv, REG_REFPIX_MSB, ref_pix);
-	reg_write16(priv, REG_REFLINE_MSB, ref_line);
-	reg_write16(priv, REG_NPIX_MSB, n_pix);
-	reg_write16(priv, REG_NLINE_MSB, n_line);
-	reg_write16(priv, REG_VS_LINE_STRT_1_MSB, vs1_line_s);
-	reg_write16(priv, REG_VS_PIX_STRT_1_MSB, vs1_pix_s);
-	reg_write16(priv, REG_VS_LINE_END_1_MSB, vs1_line_e);
-	reg_write16(priv, REG_VS_PIX_END_1_MSB, vs1_pix_e);
-	reg_write16(priv, REG_VS_LINE_STRT_2_MSB, vs2_line_s);
-	reg_write16(priv, REG_VS_PIX_STRT_2_MSB, vs2_pix_s);
-	reg_write16(priv, REG_VS_LINE_END_2_MSB, vs2_line_e);
-	reg_write16(priv, REG_VS_PIX_END_2_MSB, vs2_pix_e);
-	reg_write16(priv, REG_HS_PIX_START_MSB, hs_pix_s);
-	reg_write16(priv, REG_HS_PIX_STOP_MSB, hs_pix_e);
-	reg_write16(priv, REG_VWIN_START_1_MSB, vwin1_line_s);
-	reg_write16(priv, REG_VWIN_END_1_MSB, vwin1_line_e);
-	reg_write16(priv, REG_VWIN_START_2_MSB, vwin2_line_s);
-	reg_write16(priv, REG_VWIN_END_2_MSB, vwin2_line_e);
-	reg_write16(priv, REG_DE_START_MSB, de_pix_s);
-	reg_write16(priv, REG_DE_STOP_MSB, de_pix_e);
-
-	if (priv->rev == TDA19988) {
-		/* let incoming pixels fill the active space (if any) */
-		reg_write(priv, REG_ENABLE_SPACE, 0x01);
+	/*
+	 * if one of the cea modes, set the video format
+	 * otherwise, set all values
+	 */
+	cea_mode = drm_match_cea_mode(mode);
+	if (cea_mode >= ARRAY_SIZE(cea2vid))
+		cea_mode = 0;
+	if (cea2vid[cea_mode] != E_VFMT_INVALID) {
+		reg_write(priv, REG_VIDFORMAT, cea2vid[cea_mode]);
+	} else {
+		reg_write(priv, REG_VIDFORMAT, 0x00);
+		reg_write16(priv, REG_REFPIX_MSB, ref_pix);
+		reg_write16(priv, REG_REFLINE_MSB, ref_line);
+		reg_write16(priv, REG_NPIX_MSB, n_pix);
+		reg_write16(priv, REG_NLINE_MSB, n_line);
+		reg_write16(priv, REG_VS_LINE_STRT_1_MSB, vs1_line_s);
+		reg_write16(priv, REG_VS_PIX_STRT_1_MSB, vs1_pix_s);
+		reg_write16(priv, REG_VS_LINE_END_1_MSB, vs1_line_e);
+		reg_write16(priv, REG_VS_PIX_END_1_MSB, vs1_pix_e);
+		reg_write16(priv, REG_VS_LINE_STRT_2_MSB, vs2_line_s);
+		reg_write16(priv, REG_VS_PIX_STRT_2_MSB, vs2_pix_s);
+		reg_write16(priv, REG_VS_LINE_END_2_MSB, vs2_line_e);
+		reg_write16(priv, REG_VS_PIX_END_2_MSB, vs2_pix_e);
+		reg_write16(priv, REG_HS_PIX_START_MSB, hs_pix_s);
+		reg_write16(priv, REG_HS_PIX_STOP_MSB, hs_pix_e);
+		reg_write16(priv, REG_VWIN_START_1_MSB, vwin1_line_s);
+		reg_write16(priv, REG_VWIN_END_1_MSB, vwin1_line_e);
+		reg_write16(priv, REG_VWIN_START_2_MSB, vwin2_line_s);
+		reg_write16(priv, REG_VWIN_END_2_MSB, vwin2_line_e);
+		reg_write16(priv, REG_DE_START_MSB, de_pix_s);
+		reg_write16(priv, REG_DE_STOP_MSB, de_pix_e);
+
+
+		if (priv->rev == TDA19988) {
+			/* let incoming pixels fill the active space (if any) */
+			reg_write(priv, REG_ENABLE_SPACE, 0x01);
+		}
 	}
 
 	/* must be last register set: */