Re: [PATCH 5/5] drm/i2c: tda998x: improve correctness of quantisation range
On Wed, Jan 30, 2019 at 06:18:44PM +, Russell King - ARM Linux admin wrote:
> On Wed, Jan 30, 2019 at 03:53:04PM +, Brian Starkey wrote:
> > Hi Russell,
> >
> > On Fri, Jan 25, 2019 at 09:43:29AM +, 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 5/5] drm/i2c: tda998x: improve correctness of quantisation range
On Wed, Jan 30, 2019 at 03:53:04PM +, Brian Starkey wrote:
> Hi Russell,
>
> On Fri, Jan 25, 2019 at 09:43:29AM +, 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
> > ---
> > 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(,
> > >connector, mode);
> > - frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_FULL;
> > + drm_hdmi_avi_infoframe_quant_range(, mode,
> > + priv->rgb_quant_range,
> > + priv->has_rgb_quant, false);
> >
> > tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, );
> > }
> > @@ -1259,6 +1263,7 @@ static int tda998x_connector_get_modes(struct
> > drm_connector *connector)
> > mutex_lock(>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(>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
Re: [PATCH 5/5] drm/i2c: tda998x: improve correctness of quantisation range
Hi Russell,
On Fri, Jan 25, 2019 at 09:43:29AM +, 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
> ---
> 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(,
>>connector, mode);
> - frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_FULL;
> + drm_hdmi_avi_infoframe_quant_range(, mode,
> +priv->rgb_quant_range,
> +priv->has_rgb_quant, false);
>
> tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, );
> }
> @@ -1259,6 +1263,7 @@ static int tda998x_connector_get_modes(struct
> drm_connector *connector)
> mutex_lock(>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(>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
>
[PATCH 5/5] drm/i2c: tda998x: improve correctness of quantisation range
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
---
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(,
>connector, mode);
- frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_FULL;
+ drm_hdmi_avi_infoframe_quant_range(, mode,
+ priv->rgb_quant_range,
+ priv->has_rgb_quant, false);
tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, );
}
@@ -1259,6 +1263,7 @@ static int tda998x_connector_get_modes(struct
drm_connector *connector)
mutex_lock(>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(>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
