| Message ID | 20140109120448.24d89eba@armhf |
|---|---|
| State | New |
| Headers | show |
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.
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)
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.
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.
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.
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?
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.
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: */
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(-)