[PATCH 0/5] tda998x updates

[PATCH 0/5] tda998x updates

[Russell King - ARM Linux admin]

Hi,

This series adds support for programming the SPD and vendor infoframes.

It also adds support for pixel repeated modes - we were not rejecting
these modes, but we also didn't have the implementation to support
them.  As their implementation is simple, add it rather than rejecting
the modes.

Support is also added for the bridge timing information, that upstream
components may wish to use to adjust their output appropriately.

Lastly, rather than merely passing through the full-range RGB from the
CRTC, adapt the RGB range to the capabilities of the display and the
default range for the mode.  This means that if the display does not
support the Q bit in the video infoframe, and the mode is defined to
have limited range RGB, we will compress the output RGB range to
limited range.

Tested on 4.20 with a Panasonic TV.

      drm/i2c: tda998x: add support for pixel repeated modes
      drm/i2c: tda998x: add bridge timing information
      drm/i2c: tda998x: add support for writing SPD
      drm/i2c: tda998x: add vendor specific infoframe support
      drm/i2c: tda998x: improve correctness of quantisation range

 drivers/gpu/drm/i2c/tda998x_drv.c | 120 +++++++++++++++++++++++++++++++++-----
 1 file changed, 105 insertions(+), 15 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/5] drm/i2c: tda998x: add support for pixel repeated modes

[Russell King]

TDA998x has no support for pixel repeated modes, and the code notes this
as a "TODO" item.  The implementation appears to be relatively simple,
so lets add it.

We need to calculate the serializer clock divisor based on the TMDS
clock rate, set the repeat control, and set the serializer pixel
repeat count.  Since the audio code needs the actual TMDS clock,
record that.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index ecdb8070ed35..a7bc3b7a9bcc 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -241,6 +241,7 @@ struct tda998x_priv {
 # define HVF_CNTRL_1_PAD(x)       (((x) & 3) << 4)
 # define HVF_CNTRL_1_SEMI_PLANAR  (1 << 6)
 #define REG_RPT_CNTRL             REG(0x00, 0xf0)     /* write */
+# define RPT_CNTRL_REPEAT(x)      ((x) & 15)
 #define REG_I2S_FORMAT            REG(0x00, 0xfc)     /* read/write */
 # define I2S_FORMAT(x)            (((x) & 3) << 0)
 #define REG_AIP_CLKSEL            REG(0x00, 0xfd)     /* write */
@@ -1351,7 +1352,7 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
 	u16 vwin1_line_s, vwin1_line_e;
 	u16 vwin2_line_s, vwin2_line_e;
 	u16 de_pix_s, de_pix_e;
-	u8 reg, div, rep;
+	u8 reg, div, rep, sel_clk;
 
 	/*
 	 * Internally TDA998x is using ITU-R BT.656 style sync but
@@ -1414,7 +1415,16 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
 			       (mode->vsync_end - mode->vsync_start)/2;
 	}
 
-	tmds_clock = mode->clock;
+	/*
+	 * Select pixel repeat depending on the double-clock flag
+	 * (which means we have to repeat each pixel once.)
+	 */
+	rep = mode->flags & DRM_MODE_FLAG_DBLCLK ? 1 : 0;
+	sel_clk = SEL_CLK_ENA_SC_CLK | SEL_CLK_SEL_CLK1 |
+		  SEL_CLK_SEL_VRF_CLK(rep ? 2 : 0);
+
+	/* the TMDS clock is scaled up by the pixel repeat */
+	tmds_clock = mode->clock * (1 + rep);
 
 	/*
 	 * The divisor is power-of-2. The TDA9983B datasheet gives
@@ -1430,6 +1440,8 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
 
 	mutex_lock(&priv->audio_mutex);
 
+	priv->tmds_clock = tmds_clock;
+
 	/* mute the audio FIFO: */
 	reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
 
@@ -1452,12 +1464,8 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
 	reg_write(priv, REG_SERIALIZER, 0);
 	reg_write(priv, REG_HVF_CNTRL_1, HVF_CNTRL_1_VQR(0));
 
-	/* TODO enable pixel repeat for pixel rates less than 25Msamp/s */
-	rep = 0;
-	reg_write(priv, REG_RPT_CNTRL, 0);
-	reg_write(priv, REG_SEL_CLK, SEL_CLK_SEL_VRF_CLK(0) |
-			SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK);
-
+	reg_write(priv, REG_RPT_CNTRL, RPT_CNTRL_REPEAT(rep));
+	reg_write(priv, REG_SEL_CLK, sel_clk);
 	reg_write(priv, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(div) |
 			PLL_SERIAL_2_SRL_PR(rep));
 
@@ -1525,8 +1533,6 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
 	/* must be last register set: */
 	reg_write(priv, REG_TBG_CNTRL_0, 0);
 
-	priv->tmds_clock = adjusted_mode->clock;
-
 	/* CEA-861B section 6 says that:
 	 * CEA version 1 (CEA-861) has no support for infoframes.
 	 * CEA version 2 (CEA-861A) supports version 1 AVI infoframes,
-- 
2.7.4

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

[PATCH 2/5] drm/i2c: tda998x: add bridge timing information

[Russell King]

Add bridge timing information so that bridge users can figure out the
timing parameters that are necessary for TDA998x.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a7bc3b7a9bcc..c399a7b73e2b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1572,6 +1572,18 @@ static const struct drm_bridge_funcs tda998x_bridge_funcs = {
 	.enable = tda998x_bridge_enable,
 };
 
+static const struct drm_bridge_timings tda9989_timings = {
+	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+	.setup_time_ps = 1500,
+	.hold_time_ps = 1000,
+};
+
+static const struct drm_bridge_timings tda19988_timings = {
+	.sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+	.setup_time_ps = 1600,
+	.hold_time_ps = 1200,
+};
+
 /* I2C driver functions */
 
 static int tda998x_get_audio_ports(struct tda998x_priv *priv,
@@ -1842,6 +1854,17 @@ static int tda998x_create(struct device *dev)
 	priv->bridge.of_node = dev->of_node;
 #endif
 
+	switch (priv->rev) {
+	case TDA9989N2:
+	case TDA19989:
+	case TDA19989N2:
+		priv->bridge.timings = &tda9989_timings;
+		break;
+	case TDA19988:
+		priv->bridge.timings = &tda19988_timings;
+		break;
+	}
+
 	drm_bridge_add(&priv->bridge);
 
 	return 0;
-- 
2.7.4

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

[PATCH 3/5] drm/i2c: tda998x: add support for writing SPD

[Russell King]

Add support for writing the SPD infoframe to the TDA998x.  Identify us
as "Generic" vendor "PC" product, and as "PC general" source device
information.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index c399a7b73e2b..dad7396ebe2b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -845,6 +845,23 @@ static int tda998x_write_aif(struct tda998x_priv *priv,
 	return 0;
 }
 
+static void tda998x_write_spd(struct tda998x_priv *priv)
+{
+	union hdmi_infoframe frame;
+	int ret;
+
+	ret = hdmi_spd_infoframe_init(&frame.spd, "Generic", "PC");
+	if (ret < 0) {
+		dev_err(&priv->hdmi->dev, "failed to fill SPD infoframe: %d\n",
+			ret);
+		return;
+	}
+
+	frame.spd.sdi = HDMI_SPD_SDI_PC;
+
+	tda998x_write_if(priv, DIP_IF_FLAGS_IF3, REG_IF3_HB0, &frame);
+}
+
 static void
 tda998x_write_avi(struct tda998x_priv *priv, const struct drm_display_mode *mode)
 {
@@ -1554,6 +1571,7 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
 		reg_set(priv, REG_TX33, TX33_HDMI);
 
 		tda998x_write_avi(priv, adjusted_mode);
+		tda998x_write_spd(priv);
 
 		if (priv->audio_params.format != AFMT_UNUSED &&
 		    priv->sink_has_audio)
-- 
2.7.4

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

[PATCH 4/5] drm/i2c: tda998x: add vendor specific infoframe support

[Russell King]

Add support for the vendor specific infoframe.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index dad7396ebe2b..b0ed2ef49c62 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -874,6 +874,19 @@ tda998x_write_avi(struct tda998x_priv *priv, const struct drm_display_mode *mode
 	tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, &frame);
 }
 
+static void tda998x_write_vsi(struct tda998x_priv *priv,
+			      struct drm_display_mode *mode)
+{
+	union hdmi_infoframe frame;
+
+	if (drm_hdmi_vendor_infoframe_from_display_mode(&frame.vendor.hdmi,
+							&priv->connector,
+							mode))
+		reg_clear(priv, REG_DIP_IF_FLAGS, DIP_IF_FLAGS_IF1);
+	else
+		tda998x_write_if(priv, DIP_IF_FLAGS_IF1, REG_IF1_HB0, &frame);
+}
+
 /* Audio support */
 
 static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
@@ -1572,6 +1585,7 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
 
 		tda998x_write_avi(priv, adjusted_mode);
 		tda998x_write_spd(priv);
+		tda998x_write_vsi(priv, adjusted_mode);
 
 		if (priv->audio_params.format != AFMT_UNUSED &&
 		    priv->sink_has_audio)
-- 
2.7.4

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

[PATCH 5/5] drm/i2c: tda998x: improve correctness of quantisation range

[Russell King]

CEA-861 says: "A Source shall not send a non-zero Q value that does
not correspond to the default RGB Quantization Range for the
transmitted Picture unless the Sink indicates support for the Q bit
in a Video Capabilities Data Block."

Make TDA998x compliant by using the helper to set the quantisation
range in the infoframe, and using the TDA998x's colour scaling to
appropriately adjust the RGB values sent to the monitor.

This ensures that monitors that do not support the Q bit are sent
RGB values that are within the expected range.  Monitors with
support for the Q bit will be sent full-range RGB.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index b0ed2ef49c62..7d9aea79aff2 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -50,6 +50,8 @@ struct tda998x_priv {
 	bool is_on;
 	bool supports_infoframes;
 	bool sink_has_audio;
+	bool has_rgb_quant;
+	enum hdmi_quantization_range rgb_quant_range;
 	u8 vip_cntrl_0;
 	u8 vip_cntrl_1;
 	u8 vip_cntrl_2;
@@ -869,7 +871,9 @@ tda998x_write_avi(struct tda998x_priv *priv, const struct drm_display_mode *mode
 
 	drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
 						 &priv->connector, mode);
-	frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_FULL;
+	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
+					   priv->rgb_quant_range,
+					   priv->has_rgb_quant, false);
 
 	tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, &frame);
 }
@@ -1259,6 +1263,7 @@ static int tda998x_connector_get_modes(struct drm_connector *connector)
 	mutex_lock(&priv->audio_mutex);
 	n = drm_add_edid_modes(connector, edid);
 	priv->sink_has_audio = drm_detect_monitor_audio(edid);
+	priv->has_rgb_quant = drm_rgb_quant_range_selectable(edid);
 	mutex_unlock(&priv->audio_mutex);
 
 	kfree(edid);
@@ -1385,6 +1390,15 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
 	u8 reg, div, rep, sel_clk;
 
 	/*
+	 * Since we are "computer" like, our source invariably produces
+	 * full-range RGB.  If the monitor supports full-range, then use
+	 * it, otherwise reduce to limited-range.
+	 */
+	priv->rgb_quant_range = priv->has_rgb_quant ?
+		HDMI_QUANTIZATION_RANGE_FULL :
+		drm_default_rgb_quant_range(adjusted_mode);
+
+	/*
 	 * Internally TDA998x is using ITU-R BT.656 style sync but
 	 * we get VESA style sync. TDA998x is using a reference pixel
 	 * relative to ITU to sync to the input frame and for output
@@ -1499,10 +1513,25 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
 	reg_write(priv, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(div) |
 			PLL_SERIAL_2_SRL_PR(rep));
 
-	/* set color matrix bypass flag: */
-	reg_write(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP |
-				MAT_CONTRL_MAT_SC(1));
-	reg_set(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
+	/* set color matrix according to output rgb quant range */
+	if (priv->rgb_quant_range == HDMI_QUANTIZATION_RANGE_LIMITED) {
+		static u8 tda998x_full_to_limited_range[] = {
+			MAT_CONTRL_MAT_SC(2),
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			0x03, 0x6f, 0x00, 0x00, 0x00, 0x00,
+			0x00, 0x00, 0x03, 0x6f, 0x00, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x03, 0x6f,
+			0x00, 0x40, 0x00, 0x40, 0x00, 0x40
+		};
+		reg_clear(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
+		reg_write_range(priv, REG_MAT_CONTRL,
+				tda998x_full_to_limited_range,
+				sizeof(tda998x_full_to_limited_range));
+	} else {
+		reg_write(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP |
+					MAT_CONTRL_MAT_SC(1));
+		reg_set(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
+	}
 
 	/* set BIAS tmds value: */
 	reg_write(priv, REG_ANA_GENERAL, 0x09);
-- 
2.7.4

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

Re: [PATCH 0/5] tda998x updates

[Brian Starkey]

Hi Russell,

On Fri, Jan 25, 2019 at 09:40:38AM +0000, Russell King - ARM Linux admin wrote:
> Hi,
> 
> This series adds support for programming the SPD and vendor infoframes.
> 
> It also adds support for pixel repeated modes - we were not rejecting
> these modes, but we also didn't have the implementation to support
> them.  As their implementation is simple, add it rather than rejecting
> the modes.
> 
> Support is also added for the bridge timing information, that upstream
> components may wish to use to adjust their output appropriately.
> 
> Lastly, rather than merely passing through the full-range RGB from the
> CRTC, adapt the RGB range to the capabilities of the display and the
> default range for the mode.  This means that if the display does not
> support the Q bit in the video infoframe, and the mode is defined to
> have limited range RGB, we will compress the output RGB range to
> limited range.
> 
> Tested on 4.20 with a Panasonic TV.
> 
>       drm/i2c: tda998x: add support for pixel repeated modes
>       drm/i2c: tda998x: add bridge timing information
>       drm/i2c: tda998x: add support for writing SPD
>       drm/i2c: tda998x: add vendor specific infoframe support
>       drm/i2c: tda998x: improve correctness of quantisation range

Only this cover letter made it to my inbox (and the dri-devel
archives, for what they're worth).

Is there somewhere I can take a look at the patches?

Thanks,
-Brian

> 
>  drivers/gpu/drm/i2c/tda998x_drv.c | 120 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 105 insertions(+), 15 deletions(-)
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] tda998x updates

[Russell King - ARM Linux admin]

(Removed what I assume is a typo on the Cc line - nd@arm.com)

On Fri, Jan 25, 2019 at 11:45:10AM +0000, Brian Starkey wrote:
> Hi Russell,
> 
> On Fri, Jan 25, 2019 at 09:40:38AM +0000, Russell King - ARM Linux admin wrote:
> > Hi,
> > 
> > This series adds support for programming the SPD and vendor infoframes.
> > 
> > It also adds support for pixel repeated modes - we were not rejecting
> > these modes, but we also didn't have the implementation to support
> > them.  As their implementation is simple, add it rather than rejecting
> > the modes.
> > 
> > Support is also added for the bridge timing information, that upstream
> > components may wish to use to adjust their output appropriately.
> > 
> > Lastly, rather than merely passing through the full-range RGB from the
> > CRTC, adapt the RGB range to the capabilities of the display and the
> > default range for the mode.  This means that if the display does not
> > support the Q bit in the video infoframe, and the mode is defined to
> > have limited range RGB, we will compress the output RGB range to
> > limited range.
> > 
> > Tested on 4.20 with a Panasonic TV.
> > 
> >       drm/i2c: tda998x: add support for pixel repeated modes
> >       drm/i2c: tda998x: add bridge timing information
> >       drm/i2c: tda998x: add support for writing SPD
> >       drm/i2c: tda998x: add vendor specific infoframe support
> >       drm/i2c: tda998x: improve correctness of quantisation range
> 
> Only this cover letter made it to my inbox (and the dri-devel
> archives, for what they're worth).

I think leave it a while longer - the series emails were accepted by
gabe.freedesktop.org at 09:51 UTC, maybe it's still thinking about
them.  If not, I don't think there's anything I can do about it.

> Is there somewhere I can take a look at the patches?

http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=drm-tda998x-devel

contains very similar patches, but based on 4.20.  The series I posted
were those patches rebased on the linux-drm drm-next branch - and there
were some minor conflicts.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] tda998x updates

[Brian Starkey]

Hi,

On Fri, Jan 25, 2019 at 11:56:09AM +0000, Russell King - ARM Linux admin wrote:
> (Removed what I assume is a typo on the Cc line - nd@arm.com)

Sadly not. I have to Cc (not Bcc!) nd@arm.com to remove the
confidentiality disclaimer which would otherwise be added.

Ugly, but my only option.

> 
> On Fri, Jan 25, 2019 at 11:45:10AM +0000, Brian Starkey wrote:
> > Hi Russell,
> > 
> > On Fri, Jan 25, 2019 at 09:40:38AM +0000, Russell King - ARM Linux admin wrote:
> > > Hi,
> > > 
> > > This series adds support for programming the SPD and vendor infoframes.
> > > 
> > > It also adds support for pixel repeated modes - we were not rejecting
> > > these modes, but we also didn't have the implementation to support
> > > them.  As their implementation is simple, add it rather than rejecting
> > > the modes.
> > > 
> > > Support is also added for the bridge timing information, that upstream
> > > components may wish to use to adjust their output appropriately.
> > > 
> > > Lastly, rather than merely passing through the full-range RGB from the
> > > CRTC, adapt the RGB range to the capabilities of the display and the
> > > default range for the mode.  This means that if the display does not
> > > support the Q bit in the video infoframe, and the mode is defined to
> > > have limited range RGB, we will compress the output RGB range to
> > > limited range.
> > > 
> > > Tested on 4.20 with a Panasonic TV.
> > > 
> > >       drm/i2c: tda998x: add support for pixel repeated modes
> > >       drm/i2c: tda998x: add bridge timing information
> > >       drm/i2c: tda998x: add support for writing SPD
> > >       drm/i2c: tda998x: add vendor specific infoframe support
> > >       drm/i2c: tda998x: improve correctness of quantisation range
> > 
> > Only this cover letter made it to my inbox (and the dri-devel
> > archives, for what they're worth).
> 
> I think leave it a while longer - the series emails were accepted by
> gabe.freedesktop.org at 09:51 UTC, maybe it's still thinking about
> them.  If not, I don't think there's anything I can do about it.
> 
> > Is there somewhere I can take a look at the patches?
> 
> http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=drm-tda998x-devel
> 
> contains very similar patches, but based on 4.20.  The series I posted
> were those patches rebased on the linux-drm drm-next branch - and there
> were some minor conflicts.

Great, thanks.

-Brian

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/i2c: tda998x: add support for writing SPD

[Brian Starkey]

Hi Russell,

These did eventually reach me on Saturday evening.

On Fri, Jan 25, 2019 at 09:43:19AM +0000, Russell King wrote:
> Add support for writing the SPD infoframe to the TDA998x.  Identify us
> as "Generic" vendor "PC" product, and as "PC general" source device
> information.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---

As this infoframe is optional, and is intended to provide a "useful"
name to the user, I wonder if there's really much value in just
sending "Generic"/"PC"? It seems that it might be better to just not
send the SPD infoframe until we have a way to put something more
useful there (e.g. specified by the host driver).

Thanks,
-Brian

>  drivers/gpu/drm/i2c/tda998x_drv.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index c399a7b73e2b..dad7396ebe2b 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -845,6 +845,23 @@ static int tda998x_write_aif(struct tda998x_priv *priv,
>  	return 0;
>  }
>  
> +static void tda998x_write_spd(struct tda998x_priv *priv)
> +{
> +	union hdmi_infoframe frame;
> +	int ret;
> +
> +	ret = hdmi_spd_infoframe_init(&frame.spd, "Generic", "PC");
> +	if (ret < 0) {
> +		dev_err(&priv->hdmi->dev, "failed to fill SPD infoframe: %d\n",
> +			ret);
> +		return;
> +	}
> +
> +	frame.spd.sdi = HDMI_SPD_SDI_PC;
> +
> +	tda998x_write_if(priv, DIP_IF_FLAGS_IF3, REG_IF3_HB0, &frame);
> +}
> +
>  static void
>  tda998x_write_avi(struct tda998x_priv *priv, const struct drm_display_mode *mode)
>  {
> @@ -1554,6 +1571,7 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
>  		reg_set(priv, REG_TX33, TX33_HDMI);
>  
>  		tda998x_write_avi(priv, adjusted_mode);
> +		tda998x_write_spd(priv);
>  
>  		if (priv->audio_params.format != AFMT_UNUSED &&
>  		    priv->sink_has_audio)
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i2c: tda998x: improve correctness of quantisation range

[Brian Starkey]

Hi Russell,

On Fri, Jan 25, 2019 at 09:43:29AM +0000, Russell King wrote:
> CEA-861 says: "A Source shall not send a non-zero Q value that does
> not correspond to the default RGB Quantization Range for the
> transmitted Picture unless the Sink indicates support for the Q bit
> in a Video Capabilities Data Block."
> 
> Make TDA998x compliant by using the helper to set the quantisation
> range in the infoframe, and using the TDA998x's colour scaling to
> appropriately adjust the RGB values sent to the monitor.
> 
> This ensures that monitors that do not support the Q bit are sent
> RGB values that are within the expected range.  Monitors with
> support for the Q bit will be sent full-range RGB.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index b0ed2ef49c62..7d9aea79aff2 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -50,6 +50,8 @@ struct tda998x_priv {
>  	bool is_on;
>  	bool supports_infoframes;
>  	bool sink_has_audio;
> +	bool has_rgb_quant;
> +	enum hdmi_quantization_range rgb_quant_range;
>  	u8 vip_cntrl_0;
>  	u8 vip_cntrl_1;
>  	u8 vip_cntrl_2;
> @@ -869,7 +871,9 @@ tda998x_write_avi(struct tda998x_priv *priv, const struct drm_display_mode *mode
>  
>  	drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
>  						 &priv->connector, mode);
> -	frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_FULL;
> +	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
> +					   priv->rgb_quant_range,
> +					   priv->has_rgb_quant, false);
>  
>  	tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, &frame);
>  }
> @@ -1259,6 +1263,7 @@ static int tda998x_connector_get_modes(struct drm_connector *connector)
>  	mutex_lock(&priv->audio_mutex);
>  	n = drm_add_edid_modes(connector, edid);
>  	priv->sink_has_audio = drm_detect_monitor_audio(edid);
> +	priv->has_rgb_quant = drm_rgb_quant_range_selectable(edid);
>  	mutex_unlock(&priv->audio_mutex);
>  
>  	kfree(edid);
> @@ -1385,6 +1390,15 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
>  	u8 reg, div, rep, sel_clk;
>  
>  	/*
> +	 * Since we are "computer" like, our source invariably produces
> +	 * full-range RGB.  If the monitor supports full-range, then use
> +	 * it, otherwise reduce to limited-range.
> +	 */
> +	priv->rgb_quant_range = priv->has_rgb_quant ?
> +		HDMI_QUANTIZATION_RANGE_FULL :
> +		drm_default_rgb_quant_range(adjusted_mode);
> +
> +	/*
>  	 * Internally TDA998x is using ITU-R BT.656 style sync but
>  	 * we get VESA style sync. TDA998x is using a reference pixel
>  	 * relative to ITU to sync to the input frame and for output
> @@ -1499,10 +1513,25 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
>  	reg_write(priv, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(div) |
>  			PLL_SERIAL_2_SRL_PR(rep));
>  
> -	/* set color matrix bypass flag: */
> -	reg_write(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP |
> -				MAT_CONTRL_MAT_SC(1));
> -	reg_set(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
> +	/* set color matrix according to output rgb quant range */
> +	if (priv->rgb_quant_range == HDMI_QUANTIZATION_RANGE_LIMITED) {
> +		static u8 tda998x_full_to_limited_range[] = {
> +			MAT_CONTRL_MAT_SC(2),
> +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			0x03, 0x6f, 0x00, 0x00, 0x00, 0x00,
> +			0x00, 0x00, 0x03, 0x6f, 0x00, 0x00,
> +			0x00, 0x00, 0x00, 0x00, 0x03, 0x6f,
> +			0x00, 0x40, 0x00, 0x40, 0x00, 0x40
> +		};

I couldn't figure out from the datasheet I have what the expected data
bit-depth is here, but I assume from this offset that it's 12-bit?

Should you also set HVF_CNTRL_1_VQR to 0b01 when using limited range?

Cheers,
-Brian

> +		reg_clear(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
> +		reg_write_range(priv, REG_MAT_CONTRL,
> +				tda998x_full_to_limited_range,
> +				sizeof(tda998x_full_to_limited_range));
> +	} else {
> +		reg_write(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP |
> +					MAT_CONTRL_MAT_SC(1));
> +		reg_set(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
> +	}
>  
>  	/* set BIAS tmds value: */
>  	reg_write(priv, REG_ANA_GENERAL, 0x09);
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/i2c: tda998x: add vendor specific infoframe support

[Brian Starkey]

On Fri, Jan 25, 2019 at 09:43:24AM +0000, Russell King wrote:
> Add support for the vendor specific infoframe.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

LGTM.

Reviewed-by: Brian Starkey <brian.starkey@arm.com>

> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index dad7396ebe2b..b0ed2ef49c62 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -874,6 +874,19 @@ tda998x_write_avi(struct tda998x_priv *priv, const struct drm_display_mode *mode
>  	tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, &frame);
>  }
>  
> +static void tda998x_write_vsi(struct tda998x_priv *priv,
> +			      struct drm_display_mode *mode)
> +{
> +	union hdmi_infoframe frame;
> +
> +	if (drm_hdmi_vendor_infoframe_from_display_mode(&frame.vendor.hdmi,
> +							&priv->connector,
> +							mode))
> +		reg_clear(priv, REG_DIP_IF_FLAGS, DIP_IF_FLAGS_IF1);
> +	else
> +		tda998x_write_if(priv, DIP_IF_FLAGS_IF1, REG_IF1_HB0, &frame);
> +}
> +
>  /* Audio support */
>  
>  static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
> @@ -1572,6 +1585,7 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
>  
>  		tda998x_write_avi(priv, adjusted_mode);
>  		tda998x_write_spd(priv);
> +		tda998x_write_vsi(priv, adjusted_mode);
>  
>  		if (priv->audio_params.format != AFMT_UNUSED &&
>  		    priv->sink_has_audio)
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/i2c: tda998x: add support for writing SPD

[Russell King - ARM Linux admin]

On Wed, Jan 30, 2019 at 03:41:04PM +0000, Brian Starkey wrote:
> Hi Russell,
> 
> These did eventually reach me on Saturday evening.
> 
> On Fri, Jan 25, 2019 at 09:43:19AM +0000, Russell King wrote:
> > Add support for writing the SPD infoframe to the TDA998x.  Identify us
> > as "Generic" vendor "PC" product, and as "PC general" source device
> > information.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> 
> As this infoframe is optional, and is intended to provide a "useful"
> name to the user, I wonder if there's really much value in just
> sending "Generic"/"PC"? It seems that it might be better to just not
> send the SPD infoframe until we have a way to put something more
> useful there (e.g. specified by the host driver).

It's along the lines of what other drivers do - are you suggesting
that other drivers should not send the SPD infoframe either?

E.g.

static void vc4_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
{
        union hdmi_infoframe frame;
        int ret;

        ret = hdmi_spd_infoframe_init(&frame.spd, "Broadcom", "Videocore");
===
        mtk_hdmi_setup_spd_infoframe(hdmi, "mediatek", "On-chip HDMI");
===
        ret = hdmi_spd_infoframe_init(&frame.spd, "Intel", "Integrated gfx");

None of these convey a "useful" name to the user, unless the user
knows what is inside their device - eg, "it's a mediatek SoC" or
"it's a Broadcom SoC".

I could send instead "Philips" "TDA998x" which would be on-par with
these strings.

Maybe there should be a way to set these from DT and/or userspace?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/i2c: tda998x: add support for writing SPD

[Brian Starkey]

On Wed, Jan 30, 2019 at 05:23:40PM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Jan 30, 2019 at 03:41:04PM +0000, Brian Starkey wrote:
> > Hi Russell,
> > 
> > These did eventually reach me on Saturday evening.
> > 
> > On Fri, Jan 25, 2019 at 09:43:19AM +0000, Russell King wrote:
> > > Add support for writing the SPD infoframe to the TDA998x.  Identify us
> > > as "Generic" vendor "PC" product, and as "PC general" source device
> > > information.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > 
> > As this infoframe is optional, and is intended to provide a "useful"
> > name to the user, I wonder if there's really much value in just
> > sending "Generic"/"PC"? It seems that it might be better to just not
> > send the SPD infoframe until we have a way to put something more
> > useful there (e.g. specified by the host driver).
> 
> It's along the lines of what other drivers do - are you suggesting
> that other drivers should not send the SPD infoframe either?
> 
> E.g.
> 
> static void vc4_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
> {
>         union hdmi_infoframe frame;
>         int ret;
> 
>         ret = hdmi_spd_infoframe_init(&frame.spd, "Broadcom", "Videocore");
> ===
>         mtk_hdmi_setup_spd_infoframe(hdmi, "mediatek", "On-chip HDMI");
> ===
>         ret = hdmi_spd_infoframe_init(&frame.spd, "Intel", "Integrated gfx");
> 
> None of these convey a "useful" name to the user, unless the user
> knows what is inside their device - eg, "it's a mediatek SoC" or
> "it's a Broadcom SoC".

I grepped the same, but I came to a different conclusion. From
"Broadcom", "mediatek" and "Intel" I can identify a device, if I know
it contains one of those components. From "Generic"/"PC" I can't tell
anything at all.

> 
> I could send instead "Philips" "TDA998x" which would be on-par with
> these strings.
> 

I don't agree that it would be "on-par", but IMO it would be an
improvement over "Generic"/"PC". The strings above describe the "host"
driver/device, rather than the transmitter per-se. I think it's fair
to say a user is more likely to know what SoC their device contains
than which HDMI transmitter is connected to that SoC.

> Maybe there should be a way to set these from DT and/or userspace?

DT is a nice idea. Maybe Product from /model and Vendor from
/compatible (though how you turn a DT compatible into a human-readable
vendor could be fun).

That sounds like something which shouldn't be local to tda998x,
though.

Thanks,
-Brian

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i2c: tda998x: improve correctness of quantisation range

[Russell King - ARM Linux admin]

On Wed, Jan 30, 2019 at 03:53:04PM +0000, Brian Starkey wrote:
> Hi Russell,
> 
> On Fri, Jan 25, 2019 at 09:43:29AM +0000, Russell King wrote:
> > CEA-861 says: "A Source shall not send a non-zero Q value that does
> > not correspond to the default RGB Quantization Range for the
> > transmitted Picture unless the Sink indicates support for the Q bit
> > in a Video Capabilities Data Block."
> > 
> > Make TDA998x compliant by using the helper to set the quantisation
> > range in the infoframe, and using the TDA998x's colour scaling to
> > appropriately adjust the RGB values sent to the monitor.
> > 
> > This ensures that monitors that do not support the Q bit are sent
> > RGB values that are within the expected range.  Monitors with
> > support for the Q bit will be sent full-range RGB.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/gpu/drm/i2c/tda998x_drv.c | 39 ++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 34 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> > index b0ed2ef49c62..7d9aea79aff2 100644
> > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > @@ -50,6 +50,8 @@ struct tda998x_priv {
> >  	bool is_on;
> >  	bool supports_infoframes;
> >  	bool sink_has_audio;
> > +	bool has_rgb_quant;
> > +	enum hdmi_quantization_range rgb_quant_range;
> >  	u8 vip_cntrl_0;
> >  	u8 vip_cntrl_1;
> >  	u8 vip_cntrl_2;
> > @@ -869,7 +871,9 @@ tda998x_write_avi(struct tda998x_priv *priv, const struct drm_display_mode *mode
> >  
> >  	drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> >  						 &priv->connector, mode);
> > -	frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_FULL;
> > +	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
> > +					   priv->rgb_quant_range,
> > +					   priv->has_rgb_quant, false);
> >  
> >  	tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, &frame);
> >  }
> > @@ -1259,6 +1263,7 @@ static int tda998x_connector_get_modes(struct drm_connector *connector)
> >  	mutex_lock(&priv->audio_mutex);
> >  	n = drm_add_edid_modes(connector, edid);
> >  	priv->sink_has_audio = drm_detect_monitor_audio(edid);
> > +	priv->has_rgb_quant = drm_rgb_quant_range_selectable(edid);
> >  	mutex_unlock(&priv->audio_mutex);
> >  
> >  	kfree(edid);
> > @@ -1385,6 +1390,15 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
> >  	u8 reg, div, rep, sel_clk;
> >  
> >  	/*
> > +	 * Since we are "computer" like, our source invariably produces
> > +	 * full-range RGB.  If the monitor supports full-range, then use
> > +	 * it, otherwise reduce to limited-range.
> > +	 */
> > +	priv->rgb_quant_range = priv->has_rgb_quant ?
> > +		HDMI_QUANTIZATION_RANGE_FULL :
> > +		drm_default_rgb_quant_range(adjusted_mode);
> > +
> > +	/*
> >  	 * Internally TDA998x is using ITU-R BT.656 style sync but
> >  	 * we get VESA style sync. TDA998x is using a reference pixel
> >  	 * relative to ITU to sync to the input frame and for output
> > @@ -1499,10 +1513,25 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
> >  	reg_write(priv, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(div) |
> >  			PLL_SERIAL_2_SRL_PR(rep));
> >  
> > -	/* set color matrix bypass flag: */
> > -	reg_write(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP |
> > -				MAT_CONTRL_MAT_SC(1));
> > -	reg_set(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
> > +	/* set color matrix according to output rgb quant range */
> > +	if (priv->rgb_quant_range == HDMI_QUANTIZATION_RANGE_LIMITED) {
> > +		static u8 tda998x_full_to_limited_range[] = {
> > +			MAT_CONTRL_MAT_SC(2),
> > +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +			0x03, 0x6f, 0x00, 0x00, 0x00, 0x00,
> > +			0x00, 0x00, 0x03, 0x6f, 0x00, 0x00,
> > +			0x00, 0x00, 0x00, 0x00, 0x03, 0x6f,
> > +			0x00, 0x40, 0x00, 0x40, 0x00, 0x40
> > +		};
> 
> I couldn't figure out from the datasheet I have what the expected data
> bit-depth is here, but I assume from this offset that it's 12-bit?

No, the data sheet makes it clear that (eg) MAT_OI1_MSB contains
OFFSET_IN1[10:8] and MAT_OI1_LSB contains OFFSET_IN1[7:0] - so that's
11 bits of offset - which looks like from the information I have that
it's twos complement.  Similar applies to the output offsets.

The above is the list of register values starting at MAT_CONTRL (0x80),
with the input offsets in the first line, then the G/Y output
coefficients, R/CR coefficients, B/CB coefficients and finally the
output offsets on the last line.

Each line is a pair of MSB, LSB values, starting at G/Y input, R/CR
input, B/CB input.

The above is equivalent to:

GY_OUT = (GY_IN + 0) * 0x36f / 0x400 + 0x040

repeated for R/CR and B/CB channels.

This works if we assume that each channel is 10-bit, but as the
TDA998x supports 12-bit (and we operate it in 12-bit mode internally),
I suspect the registers do not allow the least two LSBs to be specified
in either the scaling or offset registers.

Note that when the TDA998x is configured for less bits in the data
path, it merely sets the LSBs to zero.

> Should you also set HVF_CNTRL_1_VQR to 0b01 when using limited range?

That register is part of the "HDMI Video Formatter".  It's not clear
what these bits describe - whether it's the input signal or the output
signal.  It's also not clear from the data sheet where the video
formatter resides in the chain of processing.  Given that using the
color matrix has been tested to have the desired effect, I'd rather
not mess with the HDMI video formatter unless someone identifies that
there is a real issue that it solves.

Note that I've tested this by forcing the driver to configure the
output to both limited and full range (via extra patches that have
been rejected by upstream) and switching the TV between expecting
limited or full range input with a test output that shows up the
difference very nicely.

> 
> Cheers,
> -Brian
> 
> > +		reg_clear(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
> > +		reg_write_range(priv, REG_MAT_CONTRL,
> > +				tda998x_full_to_limited_range,
> > +				sizeof(tda998x_full_to_limited_range));
> > +	} else {
> > +		reg_write(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP |
> > +					MAT_CONTRL_MAT_SC(1));
> > +		reg_set(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
> > +	}
> >  
> >  	/* set BIAS tmds value: */
> >  	reg_write(priv, REG_ANA_GENERAL, 0x09);
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i2c: tda998x: improve correctness of quantisation range

[Brian Starkey]

On Wed, Jan 30, 2019 at 06:18:44PM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Jan 30, 2019 at 03:53:04PM +0000, Brian Starkey wrote:
> > Hi Russell,
> > 
> > On Fri, Jan 25, 2019 at 09:43:29AM +0000, Russell King wrote:
> > >  
> > > -	/* set color matrix bypass flag: */
> > > -	reg_write(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP |
> > > -				MAT_CONTRL_MAT_SC(1));
> > > -	reg_set(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
> > > +	/* set color matrix according to output rgb quant range */
> > > +	if (priv->rgb_quant_range == HDMI_QUANTIZATION_RANGE_LIMITED) {
> > > +		static u8 tda998x_full_to_limited_range[] = {
> > > +			MAT_CONTRL_MAT_SC(2),
> > > +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > +			0x03, 0x6f, 0x00, 0x00, 0x00, 0x00,
> > > +			0x00, 0x00, 0x03, 0x6f, 0x00, 0x00,
> > > +			0x00, 0x00, 0x00, 0x00, 0x03, 0x6f,
> > > +			0x00, 0x40, 0x00, 0x40, 0x00, 0x40
> > > +		};
> > 
> > I couldn't figure out from the datasheet I have what the expected data
> > bit-depth is here, but I assume from this offset that it's 12-bit?
> 
> No, the data sheet makes it clear that (eg) MAT_OI1_MSB contains
> OFFSET_IN1[10:8] and MAT_OI1_LSB contains OFFSET_IN1[7:0] - so that's
> 11 bits of offset - which looks like from the information I have that
> it's twos complement.  Similar applies to the output offsets.
> 
> The above is the list of register values starting at MAT_CONTRL (0x80),
> with the input offsets in the first line, then the G/Y output
> coefficients, R/CR coefficients, B/CB coefficients and finally the
> output offsets on the last line.
> 
> Each line is a pair of MSB, LSB values, starting at G/Y input, R/CR
> input, B/CB input.
> 
> The above is equivalent to:
> 
> GY_OUT = (GY_IN + 0) * 0x36f / 0x400 + 0x040
> 
> repeated for R/CR and B/CB channels.

Right you are - I need a new calculator and/or brain. I did 256 * 4
and somehow got 4096.

Offset of 64 makes sense for 10-bit.

> 
> This works if we assume that each channel is 10-bit, but as the
> TDA998x supports 12-bit (and we operate it in 12-bit mode internally),
> I suspect the registers do not allow the least two LSBs to be specified
> in either the scaling or offset registers.
> 
> Note that when the TDA998x is configured for less bits in the data
> path, it merely sets the LSBs to zero.
> 
> > Should you also set HVF_CNTRL_1_VQR to 0b01 when using limited range?
> 
> That register is part of the "HDMI Video Formatter".  It's not clear
> what these bits describe - whether it's the input signal or the output
> signal.  It's also not clear from the data sheet where the video
> formatter resides in the chain of processing.  Given that using the
> color matrix has been tested to have the desired effect, I'd rather
> not mess with the HDMI video formatter unless someone identifies that
> there is a real issue that it solves.
> 

I figured "Video input processing registers" were related to the input
signal, and "HDMI video formatter control registers" were related to
the HDMI output encoding.

I agree that the (TDA9983B, I assume?) datasheet isn't really clear in
this regard. If it works fine, and we don't have any better
information, then OK.

Feel free to add my r-b.

Thanks,
-Brian

> Note that I've tested this by forcing the driver to configure the
> output to both limited and full range (via extra patches that have
> been rejected by upstream) and switching the TV between expecting
> limited or full range input with a test output that shows up the
> difference very nicely.
> 
> > 
> > Cheers,
> > -Brian
> > 
> > > +		reg_clear(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
> > > +		reg_write_range(priv, REG_MAT_CONTRL,
> > > +				tda998x_full_to_limited_range,
> > > +				sizeof(tda998x_full_to_limited_range));
> > > +	} else {
> > > +		reg_write(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP |
> > > +					MAT_CONTRL_MAT_SC(1));
> > > +		reg_set(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
> > > +	}
> > >  
> > >  	/* set BIAS tmds value: */
> > >  	reg_write(priv, REG_ANA_GENERAL, 0x09);
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] tda998x updates

[Russell King - ARM Linux admin]

On Fri, Jan 25, 2019 at 09:40:38AM +0000, Russell King - ARM Linux admin wrote:
> Hi,
> 
> This series adds support for programming the SPD and vendor infoframes.
> 
> It also adds support for pixel repeated modes - we were not rejecting
> these modes, but we also didn't have the implementation to support
> them.  As their implementation is simple, add it rather than rejecting
> the modes.
> 
> Support is also added for the bridge timing information, that upstream
> components may wish to use to adjust their output appropriately.
> 
> Lastly, rather than merely passing through the full-range RGB from the
> CRTC, adapt the RGB range to the capabilities of the display and the
> default range for the mode.  This means that if the display does not
> support the Q bit in the video infoframe, and the mode is defined to
> have limited range RGB, we will compress the output RGB range to
> limited range.
> 
> Tested on 4.20 with a Panasonic TV.
> 
>       drm/i2c: tda998x: add support for pixel repeated modes
>       drm/i2c: tda998x: add bridge timing information
>       drm/i2c: tda998x: add support for writing SPD
>       drm/i2c: tda998x: add vendor specific infoframe support
>       drm/i2c: tda998x: improve correctness of quantisation range
> 
>  drivers/gpu/drm/i2c/tda998x_drv.c | 120 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 105 insertions(+), 15 deletions(-)

Hi,

From what I can tell, patches 1 and 2 have not received any comments.
Should I assume that these are fine to go to David?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel