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

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

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