Date:   Sun, 25 Jun 2017 09:15:06 -0300
From:   Mauro Carvalho Chehab <mchehab@...pensource.com>
To:     "Takiguchi, Yasunari" <Yasunari.Takiguchi@...y.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "tbird20d@...il.com" <tbird20d@...il.com>,
        "frowand.list@...il.com" <frowand.list@...il.com>,
        "Yamamoto, Masayuki" <Masayuki.Yamamoto@...y.com>,
        "Nozawa, Hideki (STWN)" <Hideki.Nozawa@...y.com>,
        "Yonezawa, Kota" <Kota.Yonezawa@...y.com>,
        "Matsumoto, Toshihiko" <Toshihiko.Matsumoto@...y.com>,
        "Watanabe, Satoshi (SSS)" <Satoshi.C.Watanabe@...y.com>
Subject: Re: [PATCH v2 08/15] [media] cxd2880: Add top level of the driver

Em Fri, 23 Jun 2017 10:02:39 -0300
Mauro Carvalho Chehab <mchehab@...pensource.com> escreveu:

> Em Mon, 19 Jun 2017 16:56:13 +0900
> "Takiguchi, Yasunari" <Yasunari.Takiguchi@...y.com> escreveu:
> 
> > >> +static int cxd2880_get_frontend_t(struct dvb_frontend *fe,
> > >> +				struct dtv_frontend_properties *c)
> > >> +{
> > >> +	enum cxd2880_ret ret = CXD2880_RESULT_OK;
> > >> +	int result = 0;
> > >> +	struct cxd2880_priv *priv = NULL;
> > >> +	enum cxd2880_dvbt_mode mode = CXD2880_DVBT_MODE_2K;
> > >> +	enum cxd2880_dvbt_guard guard = CXD2880_DVBT_GUARD_1_32;
> > >> +	struct cxd2880_dvbt_tpsinfo tps;
> > >> +	enum cxd2880_tnrdmd_spectrum_sense sense;
> > >> +	u16 snr = 0;
> > >> +	int strength = 0;
> > >> +	u32 pre_bit_err = 0, pre_bit_count = 0;
> > >> +	u32 post_bit_err = 0, post_bit_count = 0;
> > >> +	u32 block_err = 0, block_count = 0;
> > >> +
> > >> +	if ((!fe) || (!c)) {
> > >> +		pr_err("%s: invalid arg\n", __func__);
> > >> +		return -EINVAL;
> > >> +	}
> > >> +
> > >> +	priv = (struct cxd2880_priv *)fe->demodulator_priv;
> > >> +
> > >> +	mutex_lock(priv->spi_mutex);
> > >> +	ret = cxd2880_tnrdmd_dvbt_mon_mode_guard(&priv->tnrdmd,
> > >> +						 &mode, &guard);
> > >> +	mutex_unlock(priv->spi_mutex);
> > >> +	if (ret == CXD2880_RESULT_OK) {
> > >> +		switch (mode) {
> > >> +		case CXD2880_DVBT_MODE_2K:
> > >> +			c->transmission_mode = TRANSMISSION_MODE_2K;
> > >> +			break;
> > >> +		case CXD2880_DVBT_MODE_8K:
> > >> +			c->transmission_mode = TRANSMISSION_MODE_8K;
> > >> +			break;
> > >> +		default:
> > >> +			c->transmission_mode = TRANSMISSION_MODE_2K;
> > >> +			dev_err(&priv->spi->dev, "%s: get invalid mode %d\n",
> > >> +					__func__, mode);
> > >> +			break;
> > >> +		}
> > >> +		switch (guard) {
> > >> +		case CXD2880_DVBT_GUARD_1_32:
> > >> +			c->guard_interval = GUARD_INTERVAL_1_32;
> > >> +			break;
> > >> +		case CXD2880_DVBT_GUARD_1_16:
> > >> +			c->guard_interval = GUARD_INTERVAL_1_16;
> > >> +			break;
> > >> +		case CXD2880_DVBT_GUARD_1_8:
> > >> +			c->guard_interval = GUARD_INTERVAL_1_8;
> > >> +			break;
> > >> +		case CXD2880_DVBT_GUARD_1_4:
> > >> +			c->guard_interval = GUARD_INTERVAL_1_4;
> > >> +			break;
> > >> +		default:
> > >> +			c->guard_interval = GUARD_INTERVAL_1_32;
> > >> +			dev_err(&priv->spi->dev, "%s: get invalid guard %d\n",
> > >> +					__func__, guard);
> > >> +			break;
> > >> +		}
> > >> +	} else {
> > >> +		c->transmission_mode = TRANSMISSION_MODE_2K;
> > >> +		c->guard_interval = GUARD_INTERVAL_1_32;
> > >> +		dev_dbg(&priv->spi->dev,
> > >> +			"%s: ModeGuard err %d\n", __func__, ret);
> > >> +	}
> > >> +
> > >> +	mutex_lock(priv->spi_mutex);
> > >> +	ret = cxd2880_tnrdmd_dvbt_mon_tps_info(&priv->tnrdmd, &tps);
> > >> +	mutex_unlock(priv->spi_mutex);
> > >> +	if (ret == CXD2880_RESULT_OK) {
> > >> +		switch (tps.hierarchy) {
> > >> +		case CXD2880_DVBT_HIERARCHY_NON:
> > >> +			c->hierarchy = HIERARCHY_NONE;
> > >> +			break;
> > >> +		case CXD2880_DVBT_HIERARCHY_1:
> > >> +			c->hierarchy = HIERARCHY_1;
> > >> +			break;
> > >> +		case CXD2880_DVBT_HIERARCHY_2:
> > >> +			c->hierarchy = HIERARCHY_2;
> > >> +			break;
> > >> +		case CXD2880_DVBT_HIERARCHY_4:
> > >> +			c->hierarchy = HIERARCHY_4;
> > >> +			break;
> > >> +		default:
> > >> +			c->hierarchy = HIERARCHY_NONE;
> > >> +			dev_err(&priv->spi->dev,
> > >> +				"%s: TPSInfo hierarchy invalid %d\n",
> > >> +				__func__, tps.hierarchy);
> > >> +			break;
> > >> +		}
> > >> +
> > >> +		switch (tps.rate_hp) {
> > >> +		case CXD2880_DVBT_CODERATE_1_2:
> > >> +			c->code_rate_HP = FEC_1_2;
> > >> +			break;
> > >> +		case CXD2880_DVBT_CODERATE_2_3:
> > >> +			c->code_rate_HP = FEC_2_3;
> > >> +			break;
> > >> +		case CXD2880_DVBT_CODERATE_3_4:
> > >> +			c->code_rate_HP = FEC_3_4;
> > >> +			break;
> > >> +		case CXD2880_DVBT_CODERATE_5_6:
> > >> +			c->code_rate_HP = FEC_5_6;
> > >> +			break;
> > >> +		case CXD2880_DVBT_CODERATE_7_8:
> > >> +			c->code_rate_HP = FEC_7_8;
> > >> +			break;
> > >> +		default:
> > >> +			c->code_rate_HP = FEC_NONE;
> > >> +			dev_err(&priv->spi->dev,
> > >> +				"%s: TPSInfo rateHP invalid %d\n",
> > >> +				__func__, tps.rate_hp);
> > >> +			break;
> > >> +		}
> > >> +		switch (tps.rate_lp) {
> > >> +		case CXD2880_DVBT_CODERATE_1_2:
> > >> +			c->code_rate_LP = FEC_1_2;
> > >> +			break;
> > >> +		case CXD2880_DVBT_CODERATE_2_3:
> > >> +			c->code_rate_LP = FEC_2_3;
> > >> +			break;
> > >> +		case CXD2880_DVBT_CODERATE_3_4:
> > >> +			c->code_rate_LP = FEC_3_4;
> > >> +			break;
> > >> +		case CXD2880_DVBT_CODERATE_5_6:
> > >> +			c->code_rate_LP = FEC_5_6;
> > >> +			break;
> > >> +		case CXD2880_DVBT_CODERATE_7_8:
> > >> +			c->code_rate_LP = FEC_7_8;
> > >> +			break;
> > >> +		default:
> > >> +			c->code_rate_LP = FEC_NONE;
> > >> +			dev_err(&priv->spi->dev,
> > >> +				"%s: TPSInfo rateLP invalid %d\n",
> > >> +				__func__, tps.rate_lp);
> > >> +			break;
> > >> +		}
> > >> +		switch (tps.constellation) {
> > >> +		case CXD2880_DVBT_CONSTELLATION_QPSK:
> > >> +			c->modulation = QPSK;
> > >> +			break;
> > >> +		case CXD2880_DVBT_CONSTELLATION_16QAM:
> > >> +			c->modulation = QAM_16;
> > >> +			break;
> > >> +		case CXD2880_DVBT_CONSTELLATION_64QAM:
> > >> +			c->modulation = QAM_64;
> > >> +			break;
> > >> +		default:
> > >> +			c->modulation = QPSK;
> > >> +			dev_err(&priv->spi->dev,
> > >> +				"%s: TPSInfo constellation invalid %d\n",
> > >> +				__func__, tps.constellation);
> > >> +			break;
> > >> +		}
> > >> +	} else {
> > >> +		c->hierarchy = HIERARCHY_NONE;
> > >> +		c->code_rate_HP = FEC_NONE;
> > >> +		c->code_rate_LP = FEC_NONE;
> > >> +		c->modulation = QPSK;
> > >> +		dev_dbg(&priv->spi->dev,
> > >> +			"%s: TPS info err %d\n", __func__, ret);
> > >> +	}
> > >> +
> > >> +	mutex_lock(priv->spi_mutex);
> > >> +	ret = cxd2880_tnrdmd_dvbt_mon_spectrum_sense(&priv->tnrdmd, &sense);
> > >> +	mutex_unlock(priv->spi_mutex);
> > >> +	if (ret == CXD2880_RESULT_OK) {
> > >> +		switch (sense) {
> > >> +		case CXD2880_TNRDMD_SPECTRUM_NORMAL:
> > >> +			c->inversion = INVERSION_OFF;
> > >> +			break;
> > >> +		case CXD2880_TNRDMD_SPECTRUM_INV:
> > >> +			c->inversion = INVERSION_ON;
> > >> +			break;
> > >> +		default:
> > >> +			c->inversion = INVERSION_OFF;
> > >> +			dev_err(&priv->spi->dev,
> > >> +				"%s: spectrum sense invalid %d\n",
> > >> +				__func__, sense);
> > >> +			break;
> > >> +		}
> > >> +	} else {
> > >> +		c->inversion = INVERSION_OFF;
> > >> +		dev_dbg(&priv->spi->dev,
> > >> +			"%s: spectrum_sense %d\n", __func__, ret);
> > >> +	}
> > >> +
> > >> +	mutex_lock(priv->spi_mutex);
> > >> +	ret = cxd2880_tnrdmd_mon_rf_lvl(&priv->tnrdmd, &strength);
> > >> +	mutex_unlock(priv->spi_mutex);
> > >> +	if (ret == CXD2880_RESULT_OK) {
> > >> +		c->strength.len = 1;
> > >> +		c->strength.stat[0].scale = FE_SCALE_DECIBEL;
> > >> +		c->strength.stat[0].svalue = strength;
> > >> +	} else {
> > >> +		c->strength.len = 1;
> > >> +		c->strength.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > >> +		dev_dbg(&priv->spi->dev, "%s: mon_rf_lvl %d\n",
> > >> +			__func__, result);
> > >> +	}
> > >> +
> > >> +	result = cxd2880_read_snr(fe, &snr);
> > >> +	if (!result) {
> > >> +		c->cnr.len = 1;
> > >> +		c->cnr.stat[0].scale = FE_SCALE_DECIBEL;
> > >> +		c->cnr.stat[0].svalue = snr;
> > >> +	} else {
> > >> +		c->cnr.len = 1;
> > >> +		c->cnr.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
> > >> +		dev_dbg(&priv->spi->dev, "%s: read_snr %d\n", __func__, result);
> > >> +	}
> > >> +
> > >> +	mutex_lock(priv->spi_mutex);
> > >> +	ret = cxd2880_pre_bit_err_t(&priv->tnrdmd, &pre_bit_err,
> > >> +					&pre_bit_count);    
> > > 
> > > Hmm... reading BER-based measures at get_frontend() may not be
> > > the best idea, depending on how the hardware works.
> > > 
> > > I mean, you need to be sure that, if userspace is calling it too
> > > often, it won't affect the hardware or the measurement.
> > > 
> > > On other drivers, where each call generates an I2C access,
> > > we do that by moving the code that actually call the hardware
> > > at get status, and we use jiffies to be sure that it won't be
> > > called too often.
> > > 
> > > I dunno if, on your SPI design, this would be an issue or not.    
> > 
> > CXD2880 IC can accept frequently counter register access by SPI.
> > Even if such access occurred, it will not affect to IC behavior.
> > We checked Sony demodulator IC driver code, dvb_frontends/cxd2841er.c and it reads register information
> > when get_frontend called.
> > 
> > In fact, CXD2880 IC do NOT have bit error/packet error counter function to achieve DVB framework API completely.
> > Sorry for long explanation, but I'll write in detail.
> > 
> > DTV_STAT_PRE_ERROR_BIT_COUNT
> > DTV_STAT_PRE_TOTAL_BIT_COUNT
> > DTV_STAT_POST_ERROR_BIT_COUNT
> > DTV_STAT_POST_TOTAL_BIT_COUNT
> > DTV_STAT_ERROR_BLOCK_COUNT
> > DTV_STAT_TOTAL_BLOCK_COUNT
> > 
> > From DVB framework documentation, it seems that the demodulator hardware should have counter
> > to accumulate total bit/packet count to decode, and bit/packet error corrected by FEC.
> > But unfortunately, CXD2880 demodulator does not have such function.  
> 
> No, this is not a requirement. What actually happens is that userspace
> expect those values to be monotonically incremented, as new data is
> made available.
> 
> > 
> > Instead of it, Sony demodulator has bit/packet error counter for BER, PER calculation.
> > The user should configure total bit/packet count (denominator) for BER/PER measurement by some register setting.  
> 
> Yeah, that's a common practice: usually, the counters are initialized
> by some registers (on a few hardware, it is hardcoded).
> 
> > The IC hardware will update the error counter automatically in period determined by
> > configured total bit/packet count (denominator).  
> 
> So, the driver need to be sure that it won't read the register too
> early, e. g. it should either be reading some register bits to know
> when to read the data, or it needs a logic that will estimate when
> the data will be ready, not updating the values to early.
> 
> An example of the first case is the mb86a20s. There, you can see at:
> mb86a20s_get_pre_ber():
> 
> 	/* Check if the BER measures are already available */
> 	rc = mb86a20s_readreg(state, 0x54);
> 	if (rc < 0)
> 		return rc;
> 
> 	/* Check if data is available for that layer */
> 	if (!(rc & (1 << layer))) {
> 		dev_dbg(&state->i2c->dev,
> 			"%s: preBER for layer %c is not available yet.\n",
> 			__func__, 'A' + layer);
> 		return -EBUSY;
> 	}
> 
> An example of the second case is dib8000. There, you can see at:
> dib8000_get_stats():
> 
> 
> 	/* Check if time for stats was elapsed */
> 	if (time_after(jiffies, state->per_jiffies_stats)) {
> 		state->per_jiffies_stats = jiffies + msecs_to_jiffies(1000);
> 
> ...
> 		/* Get UCB measures */
> 		dib8000_read_unc_blocks(fe, &val);
> 		if (val < state->init_ucb)
> 			state->init_ucb += 0x100000000LL;
> 
> 		c->block_error.stat[0].scale = FE_SCALE_COUNTER;
> 		c->block_error.stat[0].uvalue = val + state->init_ucb;
> 
> 		/* Estimate the number of packets based on bitrate */
> 		if (!time_us)
> 			time_us = dib8000_get_time_us(fe, -1);
> 
> 		if (time_us) {
> 			blocks = 1250000ULL * 1000000ULL;
> 			do_div(blocks, time_us * 8 * 204);
> 			c->block_count.stat[0].scale = FE_SCALE_COUNTER;
> 			c->block_count.stat[0].uvalue += blocks;
> 		}
> 
> 		show_per_stats = 1;
> 	}
> 
> At the above, the hardware is set to provide the number of uncorrected
> blocks on every second, and the logic there calculates the number of
> blocks based on the bitrate. It shouldn't be hard to do the opposite
> (e. g. set the hardware for a given number of blocks and calculate
> the time - I remember I coded it already - just don't remember on
> what driver - perhaps on an earlier implementation at mb86a20s).
> 
> In any case, as the code will require periodic pulls, in order
> to provide monotonic counters, we implement it via get_stats(), as
> the DVB core will ensure that it will be called periodically
> (typically 3 times per second).
> 
> > 
> > So, we implemented like as follows.
> > 
> > DTV_STAT_PRE_ERROR_BIT_COUNT
> > DTV_STAT_POST_ERROR_BIT_COUNT
> > DTV_STAT_ERROR_BLOCK_COUNT  
> > => Return bit/packet error counter value in period determined by configured total bit/packet count (numerator).    
> > 
> > DTV_STAT_PRE_TOTAL_BIT_COUNT
> > DTV_STAT_POST_TOTAL_BIT_COUNT
> > DTV_STAT_TOTAL_BLOCK_COUNT  
> > => Return currently configured total bit/packet count (denominator).    
> > 
> > By this implementation, the user can calculate BER, PER by following calculation.
> > 
> > DTV_STAT_PRE_ERROR_BIT_COUNT / DTV_STAT_PRE_TOTAL_BIT_COUNT
> > DTV_STAT_POST_ERROR_BIT_COUNT / DTV_STAT_POST_TOTAL_BIT_COUNT
> > DTV_STAT_ERROR_BLOCK_COUNT / DTV_STAT_TOTAL_BLOCK_COUNT
> > 
> > But instead, DTV_STAT_XXX_ERROR_XXX_COUNT values are not monotonically increased,
> > and DTV_STAT_XXX_TOTAL_XX_COUNT will return fixed value.  
> 
> That's wrong. you should be doing:
> 
> 	DTV_STAT_TOTAL_BLOCK_COUNT += number_of_blocks
> 
> every time the block counter registers are updated (and the
> same for the total bit counter).

Btw, as we had another developer with some doubts about stats
implementation, I wrote a chapter at the documentation in
order to explain how to properly implement it:

	https://linuxtv.org/downloads/v4l-dvb-apis-new/kapi/dtv-core.html#digital-tv-frontend-statistics

> 
> Thanks,
> Mauro



Thanks,
Mauro

Powered by blists - more mailing lists