[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 at 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