[PATCH v3] media: cxd2841er: avoid too many status inquires

[PATCH v3] media: cxd2841er: avoid too many status inquires

[Mauro Carvalho Chehab]

As reported at:
	https://tvheadend.org/issues/5625

Retrieving certain status can cause discontinuity issues.

Prevent that by adding a timeout to each status logic.

Currently, the timeout is estimated based at the channel
bandwidth. There are other parameters that may also affect
the timeout, but that would require a per-delivery system
calculus and probably more information about how cxd2481er
works, with we don't have.

So, do a poor man's best guess.

Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
 drivers/media/dvb-frontends/cxd2841er.c | 68 +++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/media/dvb-frontends/cxd2841er.c b/drivers/media/dvb-frontends/cxd2841er.c
index 1b30cf570803..218da631df19 100644
--- a/drivers/media/dvb-frontends/cxd2841er.c
+++ b/drivers/media/dvb-frontends/cxd2841er.c
@@ -60,6 +60,13 @@ struct cxd2841er_priv {
 	enum cxd2841er_xtal		xtal;
 	enum fe_caps caps;
 	u32				flags;
+
+	unsigned long			ber_interval;
+	unsigned long			ucb_interval;
+
+	unsigned long			ber_time;
+	unsigned long			ucb_time;
+	unsigned long			snr_time;
 };
 
 static const struct cxd2841er_cnr_data s_cn_data[] = {
@@ -1941,6 +1948,10 @@ static void cxd2841er_read_ber(struct dvb_frontend *fe)
 	struct cxd2841er_priv *priv = fe->demodulator_priv;
 	u32 ret, bit_error = 0, bit_count = 0;
 
+	if (priv->ber_time &&
+	   (!time_after(jiffies, priv->ber_time)))
+		return;
+
 	dev_dbg(&priv->i2c->dev, "%s()\n", __func__);
 	switch (p->delivery_system) {
 	case SYS_DVBC_ANNEX_A:
@@ -1953,9 +1964,15 @@ static void cxd2841er_read_ber(struct dvb_frontend *fe)
 		break;
 	case SYS_DVBS:
 		ret = cxd2841er_mon_read_ber_s(priv, &bit_error, &bit_count);
+
+		if (!priv->ber_interval && p->symbol_rate)
+			priv->ber_interval = (10000000) / (p->symbol_rate / 1000);
 		break;
 	case SYS_DVBS2:
 		ret = cxd2841er_mon_read_ber_s2(priv, &bit_error, &bit_count);
+
+		if (!priv->ber_interval && p->symbol_rate)
+			priv->ber_interval = (10000000) / (p->symbol_rate / 1000);
 		break;
 	case SYS_DVBT:
 		ret = cxd2841er_read_ber_t(priv, &bit_error, &bit_count);
@@ -1978,6 +1995,21 @@ static void cxd2841er_read_ber(struct dvb_frontend *fe)
 		p->post_bit_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
 		p->post_bit_count.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
 	}
+
+	/*
+	 * If the per-delivery system doesn't specify, set a default timeout
+	 * that will wait for ~12.5 seconds for 8MHz channels
+	 */
+	if (!priv->ber_interval && p->bandwidth_hz)
+		priv->ber_interval = (100000000) / (p->bandwidth_hz / 1000);
+
+	if (priv->ber_interval < 1000)
+		priv->ber_interval = 1000;
+
+// HACK:
+dev_info(&priv->i2c->dev, "BER interval: %d ms", priv->ber_interval);
+
+	priv->ber_time = jiffies + msecs_to_jiffies(priv->ber_interval);
 }
 
 static void cxd2841er_read_signal_strength(struct dvb_frontend *fe)
@@ -2037,6 +2069,16 @@ static void cxd2841er_read_snr(struct dvb_frontend *fe)
 	struct dtv_frontend_properties *p = &fe->dtv_property_cache;
 	struct cxd2841er_priv *priv = fe->demodulator_priv;
 
+	if (priv->snr_time &&
+	   (!time_after(jiffies, priv->snr_time)))
+		return;
+
+	/* Preventing asking SNR more than once per second */
+	priv->snr_time = jiffies + msecs_to_jiffies(1000);
+
+// HACK
+dev_info(&priv->i2c->dev, "SNR interval: %d ms", 1000);
+
 	dev_dbg(&priv->i2c->dev, "%s()\n", __func__);
 	switch (p->delivery_system) {
 	case SYS_DVBC_ANNEX_A:
@@ -2081,6 +2123,10 @@ static void cxd2841er_read_ucblocks(struct dvb_frontend *fe)
 	struct cxd2841er_priv *priv = fe->demodulator_priv;
 	u32 ucblocks = 0;
 
+	if (priv->ucb_time &&
+	   (!time_after(jiffies, priv->ucb_time)))
+		return;
+
 	dev_dbg(&priv->i2c->dev, "%s()\n", __func__);
 	switch (p->delivery_system) {
 	case SYS_DVBC_ANNEX_A:
@@ -2105,6 +2151,21 @@ static void cxd2841er_read_ucblocks(struct dvb_frontend *fe)
 
 	p->block_error.stat[0].scale = FE_SCALE_COUNTER;
 	p->block_error.stat[0].uvalue = ucblocks;
+
+	/*
+	 * If the per-delivery system doesn't specify, set a default timeout
+	 * that will wait 20-30 seconds
+	 */
+	if (!priv->ucb_interval && p->bandwidth_hz)
+		priv->ucb_interval = (100 * 204 * 1000 * 8) / p->bandwidth_hz;
+
+	if (priv->ucb_interval < 1000)
+		priv->ucb_interval = 1000;
+
+// HACK:
+dev_info(&priv->i2c->dev, "UCB interval: %d ms", priv->ucb_interval);
+
+	priv->ucb_time = jiffies + msecs_to_jiffies(priv->ucb_interval);
 }
 
 static int cxd2841er_dvbt2_set_profile(
@@ -3360,6 +3421,13 @@ static int cxd2841er_set_frontend_s(struct dvb_frontend *fe)
 	p->post_bit_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
 	p->post_bit_count.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
 
+	/* Reset the wait for jiffies logic */
+	priv->ber_interval = 0;
+	priv->ucb_interval = 0;
+	priv->ber_time = 0;
+	priv->ucb_time = 0;
+	priv->snr_time = 0;
+
 	return ret;
 }
 
-- 
2.21.0

Re: [PATCH v3] media: cxd2841er: avoid too many status inquires

[Daniel Scheller]

Am Fri,  4 Oct 2019 11:02:28 -0300
schrieb Mauro Carvalho Chehab <mchehab+samsung@kernel.org>:

> As reported at:
> 	https://tvheadend.org/issues/5625
> 
> Retrieving certain status can cause discontinuity issues.
> 
> Prevent that by adding a timeout to each status logic.
> 
> Currently, the timeout is estimated based at the channel
> bandwidth. There are other parameters that may also affect
> the timeout, but that would require a per-delivery system
> calculus and probably more information about how cxd2481er
> works, with we don't have.
> 
> So, do a poor man's best guess.

Such hardware quirk hack should clearly be enabled by a (new) config
flag (see the bits at the top of cxd2841er.h) which consumer drivers
can set if there are known issues with them. The reported issue is
nothing every piece of hardware with a cxd28xx demod soldered on has -
I believe the JokerTV devices which Abylay originally made this driver
for suffers from this and at least the Digital Devices C/C2/T/T2/I
boards (cxd2837/43/54) definitely don't have any issues (and I use them
regularily in my TVheadend server which is frequently used).

So please hide this behind a flag named ie. "CXD2841ER_STAT_TIMEOUT"
and enable that in the USB drivers which the affected USB sticks use.
 
> Cc: stable@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
>  drivers/media/dvb-frontends/cxd2841er.c | 68 +++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/drivers/media/dvb-frontends/cxd2841er.c b/drivers/media/dvb-frontends/cxd2841er.c
> index 1b30cf570803..218da631df19 100644
> --- a/drivers/media/dvb-frontends/cxd2841er.c
> +++ b/drivers/media/dvb-frontends/cxd2841er.c
> @@ -60,6 +60,13 @@ struct cxd2841er_priv {
>  	enum cxd2841er_xtal		xtal;
>  	enum fe_caps caps;
>  	u32				flags;
> +
> +	unsigned long			ber_interval;
> +	unsigned long			ucb_interval;
> +
> +	unsigned long			ber_time;
> +	unsigned long			ucb_time;
> +	unsigned long			snr_time;
>  };
>  
>  static const struct cxd2841er_cnr_data s_cn_data[] = {
> @@ -1941,6 +1948,10 @@ static void cxd2841er_read_ber(struct dvb_frontend *fe)
>  	struct cxd2841er_priv *priv = fe->demodulator_priv;
>  	u32 ret, bit_error = 0, bit_count = 0;
>  
> +	if (priv->ber_time &&
> +	   (!time_after(jiffies, priv->ber_time)))
> +		return;
> +
>  	dev_dbg(&priv->i2c->dev, "%s()\n", __func__);
>  	switch (p->delivery_system) {
>  	case SYS_DVBC_ANNEX_A:
> @@ -1953,9 +1964,15 @@ static void cxd2841er_read_ber(struct dvb_frontend *fe)
>  		break;
>  	case SYS_DVBS:
>  		ret = cxd2841er_mon_read_ber_s(priv, &bit_error, &bit_count);
> +
> +		if (!priv->ber_interval && p->symbol_rate)
> +			priv->ber_interval = (10000000) / (p->symbol_rate / 1000);
>  		break;
>  	case SYS_DVBS2:
>  		ret = cxd2841er_mon_read_ber_s2(priv, &bit_error, &bit_count);
> +
> +		if (!priv->ber_interval && p->symbol_rate)
> +			priv->ber_interval = (10000000) / (p->symbol_rate / 1000);
>  		break;
>  	case SYS_DVBT:
>  		ret = cxd2841er_read_ber_t(priv, &bit_error, &bit_count);
> @@ -1978,6 +1995,21 @@ static void cxd2841er_read_ber(struct dvb_frontend *fe)
>  		p->post_bit_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
>  		p->post_bit_count.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
>  	}
> +
> +	/*
> +	 * If the per-delivery system doesn't specify, set a default timeout
> +	 * that will wait for ~12.5 seconds for 8MHz channels
> +	 */
> +	if (!priv->ber_interval && p->bandwidth_hz)
> +		priv->ber_interval = (100000000) / (p->bandwidth_hz / 1000);
> +
> +	if (priv->ber_interval < 1000)
> +		priv->ber_interval = 1000;
> +
> +// HACK:
> +dev_info(&priv->i2c->dev, "BER interval: %d ms", priv->ber_interval);
> +
> +	priv->ber_time = jiffies + msecs_to_jiffies(priv->ber_interval);
>  }
>  
>  static void cxd2841er_read_signal_strength(struct dvb_frontend *fe)
> @@ -2037,6 +2069,16 @@ static void cxd2841er_read_snr(struct dvb_frontend *fe)
>  	struct dtv_frontend_properties *p = &fe->dtv_property_cache;
>  	struct cxd2841er_priv *priv = fe->demodulator_priv;
>  
> +	if (priv->snr_time &&
> +	   (!time_after(jiffies, priv->snr_time)))
> +		return;
> +
> +	/* Preventing asking SNR more than once per second */
> +	priv->snr_time = jiffies + msecs_to_jiffies(1000);
> +
> +// HACK
> +dev_info(&priv->i2c->dev, "SNR interval: %d ms", 1000);
> +
>  	dev_dbg(&priv->i2c->dev, "%s()\n", __func__);
>  	switch (p->delivery_system) {
>  	case SYS_DVBC_ANNEX_A:
> @@ -2081,6 +2123,10 @@ static void cxd2841er_read_ucblocks(struct dvb_frontend *fe)
>  	struct cxd2841er_priv *priv = fe->demodulator_priv;
>  	u32 ucblocks = 0;
>  
> +	if (priv->ucb_time &&
> +	   (!time_after(jiffies, priv->ucb_time)))
> +		return;
> +
>  	dev_dbg(&priv->i2c->dev, "%s()\n", __func__);
>  	switch (p->delivery_system) {
>  	case SYS_DVBC_ANNEX_A:
> @@ -2105,6 +2151,21 @@ static void cxd2841er_read_ucblocks(struct dvb_frontend *fe)
>  
>  	p->block_error.stat[0].scale = FE_SCALE_COUNTER;
>  	p->block_error.stat[0].uvalue = ucblocks;
> +
> +	/*
> +	 * If the per-delivery system doesn't specify, set a default timeout
> +	 * that will wait 20-30 seconds
> +	 */
> +	if (!priv->ucb_interval && p->bandwidth_hz)
> +		priv->ucb_interval = (100 * 204 * 1000 * 8) / p->bandwidth_hz;
> +
> +	if (priv->ucb_interval < 1000)
> +		priv->ucb_interval = 1000;
> +
> +// HACK:
> +dev_info(&priv->i2c->dev, "UCB interval: %d ms", priv->ucb_interval);
> +
> +	priv->ucb_time = jiffies + msecs_to_jiffies(priv->ucb_interval);
>  }
>  
>  static int cxd2841er_dvbt2_set_profile(
> @@ -3360,6 +3421,13 @@ static int cxd2841er_set_frontend_s(struct dvb_frontend *fe)
>  	p->post_bit_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
>  	p->post_bit_count.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
>  
> +	/* Reset the wait for jiffies logic */
> +	priv->ber_interval = 0;
> +	priv->ucb_interval = 0;
> +	priv->ber_time = 0;
> +	priv->ucb_time = 0;
> +	priv->snr_time = 0;
> +
>  	return ret;
>  }
>  




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

Re: [PATCH v3] media: cxd2841er: avoid too many status inquires

[Mauro Carvalho Chehab]

Em Sat, 5 Oct 2019 10:02:05 +0200
Daniel Scheller <d.scheller.oss@gmail.com> escreveu:

> Am Fri,  4 Oct 2019 11:02:28 -0300
> schrieb Mauro Carvalho Chehab <mchehab+samsung@kernel.org>:
> 
> > As reported at:
> > 	https://tvheadend.org/issues/5625
> > 
> > Retrieving certain status can cause discontinuity issues.
> > 
> > Prevent that by adding a timeout to each status logic.
> > 
> > Currently, the timeout is estimated based at the channel
> > bandwidth. There are other parameters that may also affect
> > the timeout, but that would require a per-delivery system
> > calculus and probably more information about how cxd2481er
> > works, with we don't have.
> > 
> > So, do a poor man's best guess.  
> 
> Such hardware quirk hack should clearly be enabled by a (new) config
> flag (see the bits at the top of cxd2841er.h) which consumer drivers
> can set if there are known issues with them. The reported issue is
> nothing every piece of hardware with a cxd28xx demod soldered on has -
> I believe the JokerTV devices which Abylay originally made this driver
> for suffers from this and at least the Digital Devices C/C2/T/T2/I
> boards (cxd2837/43/54) definitely don't have any issues (and I use them
> regularily in my TVheadend server which is frequently used).
> 
> So please hide this behind a flag named ie. "CXD2841ER_STAT_TIMEOUT"
> and enable that in the USB drivers which the affected USB sticks use.

I see your point.

There are a few things to consider here, though:

1) I2C bus calls are expensive, as the bus speed is typically 100kbps.
   Doing such ops too fast is known to have caused issues in the past
   with other frontends.

   So, a good practice, followed by almost all frontends, is to have 
   some logic that would prevent calling stats too fast;

2) Obtaining stats like BER, UCB, block error count and even S/N ratio takes
   time, as the frontend need to wait for enough data to arrive, in order to
   update the internal registers. At best, calling stats too fast will
   just make the frontend return a previously cached value, just spending
   I2C bus bandwidth for nothing.

3) If you look at the cxd2880 driver, wrote by Sony developers, you'll
   see that it has a complex logic to estimate the time where the
   next stats value is available. I bet that the timings calculated there
   would be similar to the minimal time to obtain a reliable measure also
   on cxd2841er.

4) The cxd2841er can't do any real estimation right now, as it tunes
   into a channel using auto mode and it misses the code that would
   retrieve the tuning parameters and would allow to properly estimate
   the bit rate of the received stream.

5) With regards to the tvheadend issue, I've been talking with the
   reporter at the IRC channel, sending him some patches to test.
   After some tests, I'm pretty sure that the issue is not Kernel
   related, but, instead, there's something broken at tvheadend
   (at least at the version he is using) that it is causing troubles
   when using the DVBv5 stats API.

With all the above considerations, I agree with you that this patch
should not be applied. Yet, we should at least apply a patch that would
prevent retrieving the stats registers too fast.

I'm sending a new version in a few.

Thanks,
Mauro