Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751895AbdFZBYp (ORCPT ); Sun, 25 Jun 2017 21:24:45 -0400 Received: from ec2-52-27-115-49.us-west-2.compute.amazonaws.com ([52.27.115.49]:38490 "EHLO osg.samsung.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751798AbdFZBYm (ORCPT ); Sun, 25 Jun 2017 21:24:42 -0400 Date: Sun, 25 Jun 2017 22:24:32 -0300 From: Mauro Carvalho Chehab To: "Takiguchi, Yasunari" Cc: "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-media@vger.kernel.org" , "tbird20d@gmail.com" , "frowand.list@gmail.com" , "Yamamoto, Masayuki" , "Nozawa, Hideki (STWN)" , "Yonezawa, Kota" , "Matsumoto, Toshihiko" , "Watanabe, Satoshi (SSS)" Subject: Re: [PATCH v2 08/15] [media] cxd2880: Add top level of the driver Message-ID: <20170625222432.4f266b1e@vento.lan> In-Reply-To: <20170625091506.1f591fcb@vento.lan> References: <20170414015043.16731-1-Yasunari.Takiguchi@sony.com> <20170414023150.17685-1-Yasunari.Takiguchi@sony.com> <20170613103014.3443406b@vento.lan> <84b4b9d6-5b91-897f-378f-4851f0d1e313@sony.com> <20170623100239.2a4bf8bb@vento.lan> <20170625091506.1f591fcb@vento.lan> Organization: Samsung X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14577 Lines: 390 Em Sun, 25 Jun 2017 09:15:06 -0300 Mauro Carvalho Chehab escreveu: > Em Fri, 23 Jun 2017 10:02:39 -0300 > Mauro Carvalho Chehab escreveu: > > > Em Mon, 19 Jun 2017 16:56:13 +0900 > > "Takiguchi, Yasunari" 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 I added a few more patches today, improving the docs. They're not yet upstream, but you can see the documentation at this link: https://www.infradead.org/~mchehab/kernel_docs/media/kapi/dtv-core.html#digital-tv-frontend-kabi > > > > > Thanks, > > Mauro > > > > Thanks, > Mauro Thanks, Mauro