Re: [PATCH v2 08/15] [media] cxd2880: Add top level of the driver
From: Mauro Carvalho Chehab
Date: Sun Jun 25 2017 - 08:15:47 EST
Em Fri, 23 Jun 2017 10:02:39 -0300
Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx> escreveu:
> Em Mon, 19 Jun 2017 16:56:13 +0900
> "Takiguchi, Yasunari" <Yasunari.Takiguchi@xxxxxxxx> 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