Em Wed, 21 Jun 2017 17:45:04 +0200
Daniel Scheller <d.scheller.oss@xxxxxxxxx> escreveu:
> Am Wed, 21 Jun 2017 09:06:22 +0300
> schrieb Antti Palosaari <crope@xxxxxx>:
>
> > On 06/20/2017 08:45 PM, Daniel Scheller wrote:
> > > From: Daniel Scheller <d.scheller@xxxxxxx>
> > >
> > > 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@xxxxxxx>
> > > ---
> > > 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
