[PATCH 0/4] STV0367/DDB DVBv5 signal statistics

[PATCH 0/4] STV0367/DDB DVBv5 signal statistics

[Daniel Scheller]

From: Daniel Scheller <d.scheller@gmx.net>

This series adds DVBv5 statistics support to the new DDB codepath of the
stv0367 demodulator driver.

The changes utilise already existing functionality (in form of register
readouts), but wraps the reads in separate functions so the existing
relative scale reporting can be kept as-is, while adding the v5 stats
in dB scale where appropriate.

>From my own testing: Reported values look approx. the same as those
reported by the cxd2841er driver for both -C and -T.

Daniel Scheller (4):
  [media] dvb-frontends/stv0367: initial DDB DVBv5 stats, implement
    ucblocks
  [media] dvb-frontends/stv0367: split SNR determination into functions
  [media] dvb-frontends/stv0367: SNR DVBv5 statistics for DVB-C and T
  [media] dvb-frontends/stv0367: DVB-C signal strength statistics

 drivers/media/dvb-frontends/stv0367.c | 180 ++++++++++++++++++++++++++++------
 1 file changed, 150 insertions(+), 30 deletions(-)

-- 
2.13.0

[PATCH 1/4] [media] dvb-frontends/stv0367: initial DDB DVBv5 stats, implement ucblocks

[Daniel Scheller]

From: Daniel Scheller <d.scheller@gmx.net>

This adds the basics to stv0367ddb_get_frontend() to be able to properly
provide signal statistics in DVBv5 format. Also adds UCB readout and
provides those values.

Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
---
 drivers/media/dvb-frontends/stv0367.c | 59 ++++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb-frontends/stv0367.c b/drivers/media/dvb-frontends/stv0367.c
index e726c2e00460..5374d4eaabd6 100644
--- a/drivers/media/dvb-frontends/stv0367.c
+++ b/drivers/media/dvb-frontends/stv0367.c
@@ -2997,21 +2997,64 @@ static int stv0367ddb_read_status(struct dvb_frontend *fe,
 	return -EINVAL;
 }
 
+static void stv0367ddb_read_ucblocks(struct dvb_frontend *fe)
+{
+	struct stv0367_state *state = fe->demodulator_priv;
+	struct dtv_frontend_properties *p = &fe->dtv_property_cache;
+	u32 ucblocks = 0;
+
+	switch (state->activedemod) {
+	case demod_ter:
+		stv0367ter_read_ucblocks(fe, &ucblocks);
+		break;
+	case demod_cab:
+		stv0367cab_read_ucblcks(fe, &ucblocks);
+		break;
+	default:
+		p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+		return;
+	}
+
+	p->block_error.stat[0].scale = FE_SCALE_COUNTER;
+	p->block_error.stat[0].uvalue = ucblocks;
+}
+
 static int stv0367ddb_get_frontend(struct dvb_frontend *fe,
 				   struct dtv_frontend_properties *p)
 {
 	struct stv0367_state *state = fe->demodulator_priv;
+	int ret = -EINVAL;
+	enum fe_status status = 0;
 
 	switch (state->activedemod) {
 	case demod_ter:
-		return stv0367ter_get_frontend(fe, p);
+		ret = stv0367ter_get_frontend(fe, p);
+		break;
 	case demod_cab:
-		return stv0367cab_get_frontend(fe, p);
-	default:
+		ret = stv0367cab_get_frontend(fe, p);
 		break;
+	default:
+		p->strength.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+		p->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+		p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+		return ret;
 	}
 
-	return -EINVAL;
+	/* read fe lock status */
+	if (!ret)
+		ret = stv0367ddb_read_status(fe, &status);
+
+	/* stop if get_frontend failed or if demod isn't locked */
+	if (ret || !(status & FE_HAS_LOCK)) {
+		p->strength.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+		p->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+		p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+		return ret;
+	}
+
+	stv0367ddb_read_ucblocks(fe);
+
+	return 0;
 }
 
 static int stv0367ddb_sleep(struct dvb_frontend *fe)
@@ -3035,6 +3078,7 @@ static int stv0367ddb_sleep(struct dvb_frontend *fe)
 static int stv0367ddb_init(struct stv0367_state *state)
 {
 	struct stv0367ter_state *ter_state = state->ter_state;
+	struct dtv_frontend_properties *p = &state->fe.dtv_property_cache;
 
 	stv0367_writereg(state, R367TER_TOPCTRL, 0x10);
 
@@ -3109,6 +3153,13 @@ static int stv0367ddb_init(struct stv0367_state *state)
 	ter_state->first_lock = 0;
 	ter_state->unlock_counter = 2;
 
+	p->strength.len = 1;
+	p->strength.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+	p->cnr.len = 1;
+	p->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+	p->block_error.len = 1;
+	p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+
 	return 0;
 }
 
-- 
2.13.0

[PATCH 2/4] [media] dvb-frontends/stv0367: split SNR determination into functions

[Daniel Scheller]

From: Daniel Scheller <d.scheller@gmx.net>

The read_snr() functions currently do some magic to return relative scale
values when called. Split out register readouts into separate functions
so the functionality can be reused in some other way.

Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
---
 drivers/media/dvb-frontends/stv0367.c | 68 +++++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 26 deletions(-)

diff --git a/drivers/media/dvb-frontends/stv0367.c b/drivers/media/dvb-frontends/stv0367.c
index 5374d4eaabd6..bb498f942ebd 100644
--- a/drivers/media/dvb-frontends/stv0367.c
+++ b/drivers/media/dvb-frontends/stv0367.c
@@ -1437,7 +1437,7 @@ static int stv0367ter_get_frontend(struct dvb_frontend *fe,
 	return 0;
 }
 
-static int stv0367ter_read_snr(struct dvb_frontend *fe, u16 *snr)
+static u32 stv0367ter_snr_readreg(struct dvb_frontend *fe)
 {
 	struct stv0367_state *state = fe->demodulator_priv;
 	u32 snru32 = 0;
@@ -1453,10 +1453,16 @@ static int stv0367ter_read_snr(struct dvb_frontend *fe, u16 *snr)
 
 		cpt++;
 	}
-
 	snru32 /= 10;/*average on 10 values*/
 
-	*snr = snru32 / 1000;
+	return snru32;
+}
+
+static int stv0367ter_read_snr(struct dvb_frontend *fe, u16 *snr)
+{
+	u32 snrval = stv0367ter_snr_readreg(fe);
+
+	*snr = snrval / 1000;
 
 	return 0;
 }
@@ -2702,51 +2708,61 @@ static int stv0367cab_read_strength(struct dvb_frontend *fe, u16 *strength)
 	return 0;
 }
 
-static int stv0367cab_read_snr(struct dvb_frontend *fe, u16 *snr)
+static int stv0367cab_snr_power(struct dvb_frontend *fe)
 {
 	struct stv0367_state *state = fe->demodulator_priv;
-	u32 noisepercentage;
 	enum stv0367cab_mod QAMSize;
-	u32 regval = 0, temp = 0;
-	int power, i;
 
 	QAMSize = stv0367_readbits(state, F367CAB_QAM_MODE);
 	switch (QAMSize) {
 	case FE_CAB_MOD_QAM4:
-		power = 21904;
-		break;
+		return 21904;
 	case FE_CAB_MOD_QAM16:
-		power = 20480;
-		break;
+		return 20480;
 	case FE_CAB_MOD_QAM32:
-		power = 23040;
-		break;
+		return 23040;
 	case FE_CAB_MOD_QAM64:
-		power = 21504;
-		break;
+		return 21504;
 	case FE_CAB_MOD_QAM128:
-		power = 23616;
-		break;
+		return 23616;
 	case FE_CAB_MOD_QAM256:
-		power = 21760;
-		break;
-	case FE_CAB_MOD_QAM512:
-		power = 1;
-		break;
+		return 21760;
 	case FE_CAB_MOD_QAM1024:
-		power = 21280;
-		break;
+		return 21280;
 	default:
-		power = 1;
 		break;
 	}
 
+	return 1;
+}
+
+static int stv0367cab_snr_readreg(struct dvb_frontend *fe, int avgdiv)
+{
+	struct stv0367_state *state = fe->demodulator_priv;
+	u32 regval = 0;
+	int i;
+
 	for (i = 0; i < 10; i++) {
 		regval += (stv0367_readbits(state, F367CAB_SNR_LO)
 			+ 256 * stv0367_readbits(state, F367CAB_SNR_HI));
 	}
 
-	regval /= 10; /*for average over 10 times in for loop above*/
+	if (avgdiv)
+		regval /= 10;
+
+	return regval;
+}
+
+static int stv0367cab_read_snr(struct dvb_frontend *fe, u16 *snr)
+{
+	struct stv0367_state *state = fe->demodulator_priv;
+	u32 noisepercentage;
+	u32 regval = 0, temp = 0;
+	int power;
+
+	power = stv0367cab_snr_power(fe);
+	regval = stv0367cab_snr_readreg(fe, 1);
+
 	if (regval != 0) {
 		temp = power
 			* (1 << (3 + stv0367_readbits(state, F367CAB_SNR_PER)));
-- 
2.13.0

[PATCH 3/4] [media] dvb-frontends/stv0367: SNR DVBv5 statistics for DVB-C and T

[Daniel Scheller]

From: Daniel Scheller <d.scheller@gmx.net>

Add signal-to-noise-ratio as provided by the demodulator in decibel scale.
QAM/DVB-C needs some intlog calculation to have usable dB values, OFDM/
DVB-T values from the demod look alright already and are provided as-is.

Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
---
 drivers/media/dvb-frontends/stv0367.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/media/dvb-frontends/stv0367.c b/drivers/media/dvb-frontends/stv0367.c
index bb498f942ebd..0b13a407df23 100644
--- a/drivers/media/dvb-frontends/stv0367.c
+++ b/drivers/media/dvb-frontends/stv0367.c
@@ -25,6 +25,8 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 
+#include "dvb_math.h"
+
 #include "stv0367.h"
 #include "stv0367_defs.h"
 #include "stv0367_regs.h"
@@ -33,6 +35,9 @@
 /* Max transfer size done by I2C transfer functions */
 #define MAX_XFER_SIZE  64
 
+/* snr logarithmic calc */
+#define INTLOG10X100(x) ((u32) (((u64) intlog10(x) * 100) >> 24))
+
 static int stvdebug;
 module_param_named(debug, stvdebug, int, 0644);
 
@@ -3013,6 +3018,33 @@ static int stv0367ddb_read_status(struct dvb_frontend *fe,
 	return -EINVAL;
 }
 
+static void stv0367ddb_read_snr(struct dvb_frontend *fe)
+{
+	struct stv0367_state *state = fe->demodulator_priv;
+	struct dtv_frontend_properties *p = &fe->dtv_property_cache;
+	int cab_pwr;
+	u32 regval, tmpval, snrval = 0;
+
+	switch (state->activedemod) {
+	case demod_ter:
+		snrval = stv0367ter_snr_readreg(fe);
+		break;
+	case demod_cab:
+		cab_pwr = stv0367cab_snr_power(fe);
+		regval = stv0367cab_snr_readreg(fe, 0);
+
+		tmpval = (cab_pwr * 320) / regval;
+		snrval = ((tmpval != 0) ? INTLOG10X100(tmpval) : 0) * 100;
+		break;
+	default:
+		p->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+		return;
+	}
+
+	p->cnr.stat[0].scale = FE_SCALE_DECIBEL;
+	p->cnr.stat[0].uvalue = snrval;
+}
+
 static void stv0367ddb_read_ucblocks(struct dvb_frontend *fe)
 {
 	struct stv0367_state *state = fe->demodulator_priv;
@@ -3069,6 +3101,7 @@ static int stv0367ddb_get_frontend(struct dvb_frontend *fe,
 	}
 
 	stv0367ddb_read_ucblocks(fe);
+	stv0367ddb_read_snr(fe);
 
 	return 0;
 }
-- 
2.13.0

[PATCH 4/4] [media] dvb-frontends/stv0367: DVB-C signal strength statistics

[Daniel Scheller]

From: Daniel Scheller <d.scheller@gmx.net>

Provide QAM/DVB-C signal strength in decibel scale. Values returned from
stv0367cab_get_rf_lvl() are good but need to be multiplied as they're in
1dBm precision.

Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
---
 drivers/media/dvb-frontends/stv0367.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/media/dvb-frontends/stv0367.c b/drivers/media/dvb-frontends/stv0367.c
index 0b13a407df23..cf684ba70a3f 100644
--- a/drivers/media/dvb-frontends/stv0367.c
+++ b/drivers/media/dvb-frontends/stv0367.c
@@ -3018,6 +3018,25 @@ static int stv0367ddb_read_status(struct dvb_frontend *fe,
 	return -EINVAL;
 }
 
+static void stv0367ddb_read_signal_strength(struct dvb_frontend *fe)
+{
+	struct stv0367_state *state = fe->demodulator_priv;
+	struct dtv_frontend_properties *p = &fe->dtv_property_cache;
+	s32 signalstrength;
+
+	switch (state->activedemod) {
+	case demod_cab:
+		signalstrength = stv0367cab_get_rf_lvl(state) * 1000;
+		break;
+	default:
+		p->strength.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
+		return;
+	}
+
+	p->strength.stat[0].scale = FE_SCALE_DECIBEL;
+	p->strength.stat[0].uvalue = signalstrength;
+}
+
 static void stv0367ddb_read_snr(struct dvb_frontend *fe)
 {
 	struct stv0367_state *state = fe->demodulator_priv;
@@ -3102,6 +3121,7 @@ static int stv0367ddb_get_frontend(struct dvb_frontend *fe,
 
 	stv0367ddb_read_ucblocks(fe);
 	stv0367ddb_read_snr(fe);
+	stv0367ddb_read_signal_strength(fe);
 
 	return 0;
 }
-- 
2.13.0

Re: [PATCH 1/4] [media] dvb-frontends/stv0367: initial DDB DVBv5 stats, implement ucblocks

[Antti Palosaari]

On 06/20/2017 08:45 PM, Daniel Scheller wrote:
> From: Daniel Scheller <d.scheller@gmx.net>
> 
> This adds the basics to stv0367ddb_get_frontend() to be able to properly
> provide signal statistics in DVBv5 format. Also adds UCB readout and
> provides those values.
> 
> Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
> ---
>   drivers/media/dvb-frontends/stv0367.c | 59 ++++++++++++++++++++++++++++++++---
>   1 file changed, 55 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/dvb-frontends/stv0367.c b/drivers/media/dvb-frontends/stv0367.c
> index e726c2e00460..5374d4eaabd6 100644
> --- a/drivers/media/dvb-frontends/stv0367.c
> +++ b/drivers/media/dvb-frontends/stv0367.c
> @@ -2997,21 +2997,64 @@ static int stv0367ddb_read_status(struct dvb_frontend *fe,
>   	return -EINVAL;
>   }
>   
> +static void stv0367ddb_read_ucblocks(struct dvb_frontend *fe)
> +{
> +	struct stv0367_state *state = fe->demodulator_priv;
> +	struct dtv_frontend_properties *p = &fe->dtv_property_cache;
> +	u32 ucblocks = 0;
> +
> +	switch (state->activedemod) {
> +	case demod_ter:
> +		stv0367ter_read_ucblocks(fe, &ucblocks);
> +		break;
> +	case demod_cab:
> +		stv0367cab_read_ucblcks(fe, &ucblocks);
> +		break;
> +	default:
> +		p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> +		return;
> +	}
> +
> +	p->block_error.stat[0].scale = FE_SCALE_COUNTER;
> +	p->block_error.stat[0].uvalue = ucblocks;
> +}
> +
>   static int stv0367ddb_get_frontend(struct dvb_frontend *fe,
>   				   struct dtv_frontend_properties *p)
>   {
>   	struct stv0367_state *state = fe->demodulator_priv;
> +	int ret = -EINVAL;
> +	enum fe_status status = 0;
>   
>   	switch (state->activedemod) {
>   	case demod_ter:
> -		return stv0367ter_get_frontend(fe, p);
> +		ret = stv0367ter_get_frontend(fe, p);
> +		break;
>   	case demod_cab:
> -		return stv0367cab_get_frontend(fe, p);
> -	default:
> +		ret = stv0367cab_get_frontend(fe, p);
>   		break;
> +	default:
> +		p->strength.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> +		p->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> +		p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> +		return ret;
>   	}
>   
> -	return -EINVAL;
> +	/* read fe lock status */
> +	if (!ret)
> +		ret = stv0367ddb_read_status(fe, &status);
> +
> +	/* stop if get_frontend failed or if demod isn't locked */
> +	if (ret || !(status & FE_HAS_LOCK)) {
> +		p->strength.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> +		p->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> +		p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> +		return ret;
> +	}

Requiring LOCK for strength and cnr sounds wrong. Demod usually 
calculates strength from IF and RF AGC and those are available even 
there is no signal at all (demod set those gains to max on that case). 
CNR is pretty often available when inner FEC (viterbi, LDPC) is on sync.

And for ber and per you need outer fec (reed-solomon, bch) too which is 
FE_HAS_SYNC flag on api. ber is error bit and count after inner fec, per 
is error packet and count after outer fec. Usually ber is counted as a 
bits and per is counted as a 204 ts packets.

Also having that statistics stuff updated inside a get_frontend() sounds 
wrong. I think that callback is optional and is not called unless 
userspace polls it.


> +
> +	stv0367ddb_read_ucblocks(fe);
> +
> +	return 0;
>   }
>   
>   static int stv0367ddb_sleep(struct dvb_frontend *fe)
> @@ -3035,6 +3078,7 @@ static int stv0367ddb_sleep(struct dvb_frontend *fe)
>   static int stv0367ddb_init(struct stv0367_state *state)
>   {
>   	struct stv0367ter_state *ter_state = state->ter_state;
> +	struct dtv_frontend_properties *p = &state->fe.dtv_property_cache;
>   
>   	stv0367_writereg(state, R367TER_TOPCTRL, 0x10);
>   
> @@ -3109,6 +3153,13 @@ static int stv0367ddb_init(struct stv0367_state *state)
>   	ter_state->first_lock = 0;
>   	ter_state->unlock_counter = 2;
>   
> +	p->strength.len = 1;
> +	p->strength.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> +	p->cnr.len = 1;
> +	p->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> +	p->block_error.len = 1;
> +	p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> +
>   	return 0;
>   }
>   
> 

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 3/4] [media] dvb-frontends/stv0367: SNR DVBv5 statistics for DVB-C and T

[Antti Palosaari]

On 06/20/2017 08:45 PM, Daniel Scheller wrote:
> From: Daniel Scheller <d.scheller@gmx.net>
> 
> Add signal-to-noise-ratio as provided by the demodulator in decibel scale.
> QAM/DVB-C needs some intlog calculation to have usable dB values, OFDM/
> DVB-T values from the demod look alright already and are provided as-is.
> 
> Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
> ---
>   drivers/media/dvb-frontends/stv0367.c | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/media/dvb-frontends/stv0367.c b/drivers/media/dvb-frontends/stv0367.c
> index bb498f942ebd..0b13a407df23 100644
> --- a/drivers/media/dvb-frontends/stv0367.c
> +++ b/drivers/media/dvb-frontends/stv0367.c
> @@ -25,6 +25,8 @@
>   #include <linux/slab.h>
>   #include <linux/i2c.h>
>   
> +#include "dvb_math.h"
> +
>   #include "stv0367.h"
>   #include "stv0367_defs.h"
>   #include "stv0367_regs.h"
> @@ -33,6 +35,9 @@
>   /* Max transfer size done by I2C transfer functions */
>   #define MAX_XFER_SIZE  64
>   
> +/* snr logarithmic calc */
> +#define INTLOG10X100(x) ((u32) (((u64) intlog10(x) * 100) >> 24))
> +
>   static int stvdebug;
>   module_param_named(debug, stvdebug, int, 0644);
>   
> @@ -3013,6 +3018,33 @@ static int stv0367ddb_read_status(struct dvb_frontend *fe,
>   	return -EINVAL;
>   }
>   
> +static void stv0367ddb_read_snr(struct dvb_frontend *fe)
> +{
> +	struct stv0367_state *state = fe->demodulator_priv;
> +	struct dtv_frontend_properties *p = &fe->dtv_property_cache;
> +	int cab_pwr;
> +	u32 regval, tmpval, snrval = 0;
> +
> +	switch (state->activedemod) {
> +	case demod_ter:
> +		snrval = stv0367ter_snr_readreg(fe);
> +		break;
> +	case demod_cab:
> +		cab_pwr = stv0367cab_snr_power(fe);
> +		regval = stv0367cab_snr_readreg(fe, 0);
> +
> +		tmpval = (cab_pwr * 320) / regval;
> +		snrval = ((tmpval != 0) ? INTLOG10X100(tmpval) : 0) * 100;

How much there will be rounding errors due to that signal/noise 
division? I would convert it to calculation of sums (tip logarithm 
calculation rules).

Also, that INTLOG10X100 is pretty much useless. Use just what 
intlog10/intlog2 offers without yet again another conversion.



> +		break;
> +	default:
> +		p->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> +		return;
> +	}
> +
> +	p->cnr.stat[0].scale = FE_SCALE_DECIBEL;
> +	p->cnr.stat[0].uvalue = snrval;
> +}
> +
>   static void stv0367ddb_read_ucblocks(struct dvb_frontend *fe)
>   {
>   	struct stv0367_state *state = fe->demodulator_priv;
> @@ -3069,6 +3101,7 @@ static int stv0367ddb_get_frontend(struct dvb_frontend *fe,
>   	}
>   
>   	stv0367ddb_read_ucblocks(fe);
> +	stv0367ddb_read_snr(fe);
>   
>   	return 0;
>   }
> 

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 1/4] [media] dvb-frontends/stv0367: initial DDB DVBv5 stats, implement ucblocks

[Daniel Scheller]

Am Wed, 21 Jun 2017 09:06:22 +0300
schrieb Antti Palosaari <crope@iki.fi>:

> On 06/20/2017 08:45 PM, Daniel Scheller wrote:
> > From: Daniel Scheller <d.scheller@gmx.net>
> > 
> > This adds the basics to stv0367ddb_get_frontend() to be able to properly
> > provide signal statistics in DVBv5 format. Also adds UCB readout and
> > provides those values.
> > 
> > Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
> > ---
> >   drivers/media/dvb-frontends/stv0367.c | 59 ++++++++++++++++++++++++++++++++---
> >   1 file changed, 55 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/dvb-frontends/stv0367.c b/drivers/media/dvb-frontends/stv0367.c
> > index e726c2e00460..5374d4eaabd6 100644
> > --- a/drivers/media/dvb-frontends/stv0367.c
> > +++ b/drivers/media/dvb-frontends/stv0367.c
> > @@ -2997,21 +2997,64 @@ static int stv0367ddb_read_status(struct dvb_frontend *fe,
> >   	return -EINVAL;
> >   }
> >   
> > +static void stv0367ddb_read_ucblocks(struct dvb_frontend *fe)
> > +{
> > +	struct stv0367_state *state = fe->demodulator_priv;
> > +	struct dtv_frontend_properties *p = &fe->dtv_property_cache;
> > +	u32 ucblocks = 0;
> > +
> > +	switch (state->activedemod) {
> > +	case demod_ter:
> > +		stv0367ter_read_ucblocks(fe, &ucblocks);
> > +		break;
> > +	case demod_cab:
> > +		stv0367cab_read_ucblcks(fe, &ucblocks);
> > +		break;
> > +	default:
> > +		p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > +		return;
> > +	}
> > +
> > +	p->block_error.stat[0].scale = FE_SCALE_COUNTER;
> > +	p->block_error.stat[0].uvalue = ucblocks;
> > +}
> > +
> >   static int stv0367ddb_get_frontend(struct dvb_frontend *fe,
> >   				   struct dtv_frontend_properties *p)
> >   {
> >   	struct stv0367_state *state = fe->demodulator_priv;
> > +	int ret = -EINVAL;
> > +	enum fe_status status = 0;
> >   
> >   	switch (state->activedemod) {
> >   	case demod_ter:
> > -		return stv0367ter_get_frontend(fe, p);
> > +		ret = stv0367ter_get_frontend(fe, p);
> > +		break;
> >   	case demod_cab:
> > -		return stv0367cab_get_frontend(fe, p);
> > -	default:
> > +		ret = stv0367cab_get_frontend(fe, p);
> >   		break;
> > +	default:
> > +		p->strength.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > +		p->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > +		p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > +		return ret;
> >   	}
> >   
> > -	return -EINVAL;
> > +	/* read fe lock status */
> > +	if (!ret)
> > +		ret = stv0367ddb_read_status(fe, &status);
> > +
> > +	/* stop if get_frontend failed or if demod isn't locked */
> > +	if (ret || !(status & FE_HAS_LOCK)) {
> > +		p->strength.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > +		p->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > +		p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > +		return ret;
> > +	}  
> 
> Requiring LOCK for strength and cnr sounds wrong. Demod usually 
> calculates strength from IF and RF AGC and those are available even 
> there is no signal at all (demod set those gains to max on that case). 
> CNR is pretty often available when inner FEC (viterbi, LDPC) is on sync.
> 
> And for ber and per you need outer fec (reed-solomon, bch) too which is 
> FE_HAS_SYNC flag on api. ber is error bit and count after inner fec, per 
> is error packet and count after outer fec. Usually ber is counted as a 
> bits and per is counted as a 204 ts packets.

Re ber/per, note that I don't have any register documentation available, everything has been gathered from this and from DD's stv0367dd driver. That said, the same applies to FE_HAS_SYNC. This driver currently only reports FE_HAS_LOCK for both OFDM and QAM operation modes, see L1503 (OFDM) and L2152. In stv0367dd, lock state acquisition is a bit more detailed. For the ddb-parts though, I even had to implement a var which carries the register which tells us in QAM mode where to acquire the lockstate from, so I don't want to blindly carry over that code since this will risk breakage of all other consumers of the stv0367 demod driver and thus the card support, neither do I want to additionally port over the read_status code since this will result in unneeded duplication of things. So atm things won't improve unless someone with some other hardware using this demod pops up, willing to experiment.

Of course I can do snr/cnr readout regardless of FE_HAS_LOCK - no strong opinion on this (needs a quick test though). Depending on if you make this a strong change requirement - please elaborate.
 
> Also having that statistics stuff updated inside a get_frontend() sounds 
> wrong. I think that callback is optional and is not called unless 
> userspace polls it.

I oriented myself on other drivers (cxd2841er for example also does this stuff in get_frontend). In your af9033 I saw you're doing this in read_status though. Would that be preferred?

-- 
Best regards,
Daniel Scheller
-- 
https://github.com/herrnst

Re: [PATCH 3/4] [media] dvb-frontends/stv0367: SNR DVBv5 statistics for DVB-C and T

[Daniel Scheller]

Am Wed, 21 Jun 2017 09:30:27 +0300
schrieb Antti Palosaari <crope@iki.fi>:

> On 06/20/2017 08:45 PM, Daniel Scheller wrote:
> > From: Daniel Scheller <d.scheller@gmx.net>
> > 
> > Add signal-to-noise-ratio as provided by the demodulator in decibel scale.
> > QAM/DVB-C needs some intlog calculation to have usable dB values, OFDM/
> > DVB-T values from the demod look alright already and are provided as-is.
> > 
> > Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
> > ---
> >   drivers/media/dvb-frontends/stv0367.c | 33 +++++++++++++++++++++++++++++++++
> >   1 file changed, 33 insertions(+)
> > 
> > diff --git a/drivers/media/dvb-frontends/stv0367.c b/drivers/media/dvb-frontends/stv0367.c
> > index bb498f942ebd..0b13a407df23 100644
> > --- a/drivers/media/dvb-frontends/stv0367.c
> > +++ b/drivers/media/dvb-frontends/stv0367.c
> > @@ -25,6 +25,8 @@
> >   #include <linux/slab.h>
> >   #include <linux/i2c.h>
> >   
> > +#include "dvb_math.h"
> > +
> >   #include "stv0367.h"
> >   #include "stv0367_defs.h"
> >   #include "stv0367_regs.h"
> > @@ -33,6 +35,9 @@
> >   /* Max transfer size done by I2C transfer functions */
> >   #define MAX_XFER_SIZE  64
> >   
> > +/* snr logarithmic calc */
> > +#define INTLOG10X100(x) ((u32) (((u64) intlog10(x) * 100) >> 24))
> > +
> >   static int stvdebug;
> >   module_param_named(debug, stvdebug, int, 0644);
> >   
> > @@ -3013,6 +3018,33 @@ static int stv0367ddb_read_status(struct dvb_frontend *fe,
> >   	return -EINVAL;
> >   }
> >   
> > +static void stv0367ddb_read_snr(struct dvb_frontend *fe)
> > +{
> > +	struct stv0367_state *state = fe->demodulator_priv;
> > +	struct dtv_frontend_properties *p = &fe->dtv_property_cache;
> > +	int cab_pwr;
> > +	u32 regval, tmpval, snrval = 0;
> > +
> > +	switch (state->activedemod) {
> > +	case demod_ter:
> > +		snrval = stv0367ter_snr_readreg(fe);
> > +		break;
> > +	case demod_cab:
> > +		cab_pwr = stv0367cab_snr_power(fe);
> > +		regval = stv0367cab_snr_readreg(fe, 0);
> > +
> > +		tmpval = (cab_pwr * 320) / regval;
> > +		snrval = ((tmpval != 0) ? INTLOG10X100(tmpval) : 0) * 100;  
> 
> How much there will be rounding errors due to that signal/noise 
> division? I would convert it to calculation of sums (tip logarithm 
> calculation rules).

This is taken from stv0367dd aswell, the reported and calculated values are in 0.1dB precision. This and to not diverge any more from the "source" driver, I'd prefer to keep it how it is. These are just simple tuner cards anyway and by no means professional measurement gear, and should only give a more or less rough estimate on reception quality. E.g. my stv0367 cards report around 36dB SNR, whereas the cxd2841er reports ~37dB, compared to my DOCSIS modem, which reports 34dB on DOCSIS channels (another variant I had earlier even reported 39dB on the same channels), so... Even, we get way more precision than on the relative scale calc on the cab_read_snr functions which is in 10%-steps...

> Also, that INTLOG10X100 is pretty much useless. Use just what 
> intlog10/intlog2 offers without yet again another conversion.

Will check and experiment. Again, taken from stv0367dd :-)

-- 
Best regards,
Daniel Scheller
-- 
https://github.com/herrnst

Re: [PATCH 3/4] [media] dvb-frontends/stv0367: SNR DVBv5 statistics for DVB-C and T

[Antti Palosaari]

On 06/21/2017 06:50 PM, Daniel Scheller wrote:
> Am Wed, 21 Jun 2017 09:30:27 +0300
> schrieb Antti Palosaari <crope@iki.fi>:
> 
>> On 06/20/2017 08:45 PM, Daniel Scheller wrote:
>>> From: Daniel Scheller <d.scheller@gmx.net>
>>>
>>> Add signal-to-noise-ratio as provided by the demodulator in decibel scale.
>>> QAM/DVB-C needs some intlog calculation to have usable dB values, OFDM/
>>> DVB-T values from the demod look alright already and are provided as-is.
>>>
>>> Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
>>> ---
>>>    drivers/media/dvb-frontends/stv0367.c | 33 +++++++++++++++++++++++++++++++++
>>>    1 file changed, 33 insertions(+)
>>>
>>> diff --git a/drivers/media/dvb-frontends/stv0367.c b/drivers/media/dvb-frontends/stv0367.c
>>> index bb498f942ebd..0b13a407df23 100644
>>> --- a/drivers/media/dvb-frontends/stv0367.c
>>> +++ b/drivers/media/dvb-frontends/stv0367.c
>>> @@ -25,6 +25,8 @@
>>>    #include <linux/slab.h>
>>>    #include <linux/i2c.h>
>>>    
>>> +#include "dvb_math.h"
>>> +
>>>    #include "stv0367.h"
>>>    #include "stv0367_defs.h"
>>>    #include "stv0367_regs.h"
>>> @@ -33,6 +35,9 @@
>>>    /* Max transfer size done by I2C transfer functions */
>>>    #define MAX_XFER_SIZE  64
>>>    
>>> +/* snr logarithmic calc */
>>> +#define INTLOG10X100(x) ((u32) (((u64) intlog10(x) * 100) >> 24))
>>> +
>>>    static int stvdebug;
>>>    module_param_named(debug, stvdebug, int, 0644);
>>>    
>>> @@ -3013,6 +3018,33 @@ static int stv0367ddb_read_status(struct dvb_frontend *fe,
>>>    	return -EINVAL;
>>>    }
>>>    
>>> +static void stv0367ddb_read_snr(struct dvb_frontend *fe)
>>> +{
>>> +	struct stv0367_state *state = fe->demodulator_priv;
>>> +	struct dtv_frontend_properties *p = &fe->dtv_property_cache;
>>> +	int cab_pwr;
>>> +	u32 regval, tmpval, snrval = 0;
>>> +
>>> +	switch (state->activedemod) {
>>> +	case demod_ter:
>>> +		snrval = stv0367ter_snr_readreg(fe);
>>> +		break;
>>> +	case demod_cab:
>>> +		cab_pwr = stv0367cab_snr_power(fe);
>>> +		regval = stv0367cab_snr_readreg(fe, 0);
>>> +
>>> +		tmpval = (cab_pwr * 320) / regval;
>>> +		snrval = ((tmpval != 0) ? INTLOG10X100(tmpval) : 0) * 100;
>>
>> How much there will be rounding errors due to that signal/noise
>> division? I would convert it to calculation of sums (tip logarithm
>> calculation rules).
> 
> This is taken from stv0367dd aswell, the reported and calculated values are in 0.1dB precision. This and to not diverge any more from the "source" driver, I'd prefer to keep it how it is. These are just simple tuner cards anyway and by no means professional measurement gear, and should only give a more or less rough estimate on reception quality. E.g. my stv0367 cards report around 36dB SNR, whereas the cxd2841er reports ~37dB, compared to my DOCSIS modem, which reports 34dB on DOCSIS channels (another variant I had earlier even reported 39dB on the same channels), so... Even, we get way more precision than on the relative scale calc on the cab_read_snr functions which is in 10%-steps...
> 
>> Also, that INTLOG10X100 is pretty much useless. Use just what
>> intlog10/intlog2 offers without yet again another conversion.
> 
> Will check and experiment. Again, taken from stv0367dd :-)

You should understand that there is no floating points on kernel, thus 
that kind of divisions needs special attention. It should be written 
log10(signal) - log10(noise) in order to minimize rounding errors. Lets 
say as example if you divide 2 by 3 you will get 0, not 0.666... So 
depending on actual numbers used on calculation, there is more or less 
rounding errors which are easily avoidable.



Antti

-- 
http://palosaari.fi/

Re: [PATCH 1/4] [media] dvb-frontends/stv0367: initial DDB DVBv5 stats, implement ucblocks

[Mauro Carvalho Chehab]

Em Wed, 21 Jun 2017 17:45:04 +0200
Daniel Scheller <d.scheller.oss@gmail.com> escreveu:

> Am Wed, 21 Jun 2017 09:06:22 +0300
> schrieb Antti Palosaari <crope@iki.fi>:
> 
> > On 06/20/2017 08:45 PM, Daniel Scheller wrote:  
> > > From: Daniel Scheller <d.scheller@gmx.net>
> > > 
> > > This adds the basics to stv0367ddb_get_frontend() to be able to properly
> > > provide signal statistics in DVBv5 format. Also adds UCB readout and
> > > provides those values.
> > > 
> > > Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
> > > ---
> > >   drivers/media/dvb-frontends/stv0367.c | 59 ++++++++++++++++++++++++++++++++---
> > >   1 file changed, 55 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/media/dvb-frontends/stv0367.c b/drivers/media/dvb-frontends/stv0367.c
> > > index e726c2e00460..5374d4eaabd6 100644
> > > --- a/drivers/media/dvb-frontends/stv0367.c
> > > +++ b/drivers/media/dvb-frontends/stv0367.c
> > > @@ -2997,21 +2997,64 @@ static int stv0367ddb_read_status(struct dvb_frontend *fe,
> > >   	return -EINVAL;
> > >   }
> > >   
> > > +static void stv0367ddb_read_ucblocks(struct dvb_frontend *fe)
> > > +{
> > > +	struct stv0367_state *state = fe->demodulator_priv;
> > > +	struct dtv_frontend_properties *p = &fe->dtv_property_cache;
> > > +	u32 ucblocks = 0;
> > > +
> > > +	switch (state->activedemod) {
> > > +	case demod_ter:
> > > +		stv0367ter_read_ucblocks(fe, &ucblocks);
> > > +		break;
> > > +	case demod_cab:
> > > +		stv0367cab_read_ucblcks(fe, &ucblocks);
> > > +		break;
> > > +	default:
> > > +		p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > > +		return;
> > > +	}
> > > +
> > > +	p->block_error.stat[0].scale = FE_SCALE_COUNTER;
> > > +	p->block_error.stat[0].uvalue = ucblocks;
> > > +}
> > > +
> > >   static int stv0367ddb_get_frontend(struct dvb_frontend *fe,
> > >   				   struct dtv_frontend_properties *p)
> > >   {
> > >   	struct stv0367_state *state = fe->demodulator_priv;
> > > +	int ret = -EINVAL;
> > > +	enum fe_status status = 0;
> > >   
> > >   	switch (state->activedemod) {
> > >   	case demod_ter:
> > > -		return stv0367ter_get_frontend(fe, p);
> > > +		ret = stv0367ter_get_frontend(fe, p);
> > > +		break;
> > >   	case demod_cab:
> > > -		return stv0367cab_get_frontend(fe, p);
> > > -	default:
> > > +		ret = stv0367cab_get_frontend(fe, p);
> > >   		break;
> > > +	default:
> > > +		p->strength.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > > +		p->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > > +		p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > > +		return ret;
> > >   	}
> > >   
> > > -	return -EINVAL;
> > > +	/* read fe lock status */
> > > +	if (!ret)
> > > +		ret = stv0367ddb_read_status(fe, &status);
> > > +
> > > +	/* stop if get_frontend failed or if demod isn't locked */
> > > +	if (ret || !(status & FE_HAS_LOCK)) {
> > > +		p->strength.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > > +		p->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > > +		p->block_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > > +		return ret;
> > > +	}    
> > 
> > Requiring LOCK for strength and cnr sounds wrong. Demod usually 
> > calculates strength from IF and RF AGC and those are available even 
> > there is no signal at all (demod set those gains to max on that case). 
> > CNR is pretty often available when inner FEC (viterbi, LDPC) is on sync.
> > 
> > And for ber and per you need outer fec (reed-solomon, bch) too which is 
> > FE_HAS_SYNC flag on api. ber is error bit and count after inner fec, per 
> > is error packet and count after outer fec. Usually ber is counted as a 
> > bits and per is counted as a 204 ts packets.  
> 
> Re ber/per, note that I don't have any register documentation available, everything has been gathered from this and from DD's stv0367dd driver. That said, the same applies to FE_HAS_SYNC. This driver currently only reports FE_HAS_LOCK for both OFDM and QAM operation modes, see L1503 (OFDM) and L2152. In stv0367dd, lock state acquisition is a bit more detailed. For the ddb-parts though, I even had to implement a var which carries the register which tells us in QAM mode where to acquire the lockstate from, so I don't want to blindly carry over that code since this will risk breakage of all other consumers of the stv0367 demod driver and thus the card support, neither do I want to additionally port over the read_status code since this will result in unneeded duplication of things. So atm things won't improve unless someone with some other hardware using this demod pops up, willing to experiment.

> Of course I can do snr/cnr readout regardless of FE_HAS_LOCK - no strong opinion on this (needs a quick test though). Depending on if you make this a strong change requirement - please elaborate.

Daniel,

You don't need register documentation to know that UCB stats won't be 
available before locks.

As this is the second time this week a different developer is having
issues to implement it the right way, I'm, assuming a \mea culpa\, as
we have almost no documentation about that.

So, I decided to write a patch for the DVB documentation, explaining how
we expect it to be done and why:
	https://patchwork.linuxtv.org/patch/42081/

> Of course I can do snr/cnr readout regardless of FE_HAS_LOCK - no strong opinion on this (needs a quick test though). Depending on if you make this a strong change requirement - please elaborate.

> > Also having that statistics stuff updated inside a get_frontend() sounds 
> > wrong. I think that callback is optional and is not called unless 
> > userspace polls it.  
> 
> I oriented myself on other drivers (cxd2841er for example also does this stuff in get_frontend). In your af9033 I saw you're doing this in read_status though. Would that be preferred?

I guess we pointed this issue to Abylay when I reviewed his 
patchset adding the cxd2841 driver. I guess I ended merging without
this fix.

Thanks,
Mauro