linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
To: Nick Desaulniers <ndesaulniers@google.com>
Cc: Nathan Chancellor <natechancellor@gmail.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-media@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] [media] dib7000p: Remove dead code
Date: Mon, 17 Sep 2018 19:39:36 -0300	[thread overview]
Message-ID: <20180917193936.33e90d5a@coco.lan> (raw)
In-Reply-To: <CAKwvOdmQ4pbbPuvYrVYB9myD8ap36h6nLjEdL-mSbYjM37UJ_g@mail.gmail.com>

Em Mon, 17 Sep 2018 10:58:32 -0700
Nick Desaulniers <ndesaulniers@google.com> escreveu:

> On Fri, Sep 14, 2018 at 10:47 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang warns that 'interleaving' is assigned to itself in this function.
> >
> > drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> > assigning value of variable of type 'int' to itself [-Wself-assign]
> >         interleaving = interleaving;
> >         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> > 1 warning generated.
> >
> > It's correct. Just removing the self-assignment would sufficiently hide
> > the warning but all of this code is dead because 'tmp' is zero due to
> > being multiplied by zero. This doesn't appear to be an issue with
> > dib8000, which this code was copied from in commit 041ad449683b
> > ("[media] dib7000p: Add DVBv5 stats support").
> >
> > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  drivers/media/dvb-frontends/dib7000p.c | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> > index 58387860b62d..25843658fc68 100644
> > --- a/drivers/media/dvb-frontends/dib7000p.c
> > +++ b/drivers/media/dvb-frontends/dib7000p.c
> > @@ -1800,9 +1800,8 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)  
> 
> Something looks wrong here (with this function).  The patch is no
> functional change, since as you point out `interleaving` is
> initialized to 0, then never updated before read, but I think there's
> an underlying bug here that should be fixed differently.  Thanks for
> the patch though, as it does raise the question.
> 
> dib7000p_get_time_us has this comment above it:
> 
>   1798 /* FIXME: may require changes - this one was borrowed from
> dib8000 */

The goal of dib7000p_get_time_us() is to estimate how much time it
takes, with current tuning parameters, to have a certain number of
DVB-T packets. This is used for block error count. That's said,
on a quick look, it seems that the code is not right on many ways.

It should be aligned with the amount of data it is required for
dib7000 to update the block/bit error counters. There are two kinds
of implementation:

1) the frontend has an internal counter that it is shifted and made
   available to the driver after a certain amount of received data
   (usually in the order of 10^5 to 10^7 bits);

2) the frontend has an internal timer that shifts the data from its
   internal counter after a certain amount of time (usually at the
   seconds range).

Different vendors opt for either one of the strategy. Some updates
a counter with the amount of bits taken. Unfortunately, this is not
the case of those dib* frontends. So, the Kernel has to estimate
it, based on the tuning parameters.

From the code, it seems that, for block errors, it waits for 1,250,000
bits to arrive (e. g. about 766 packets), so, it uses type (1) strategy:

                /* Estimate the number of packets based on bitrate */
                if (!time_us)
                        time_us = dib7000p_get_time_us(demod);

                if (time_us) {
                        blocks = 1250000ULL * 1000000ULL;	// the multiply here is to convert to microsseconds...
                        do_div(blocks, time_us * 8 * 204);	// As here it divides by the time in microsseconds
                        c->block_count.stat[0].scale = FE_SCALE_COUNTER;
                        c->block_count.stat[0].uvalue += blocks;
                }

For BER, the logic assumes that the bit error count should be divided
by 10^-8:

                c->post_bit_count.stat[0].uvalue += 100000000;

and the counter is updated every second. So, it uses (2).

> 
> Wondering if it has the same bug, it seems it does not:
> drivers/media/dvb-frontends/dib8000.c#dib8000_get_time_us():3986
> 
> dib8000_get_time_us() seems to loop over multiple layers, and then
> assigns interleaving to the final layers interleaving (that looks like
> loop invariant code to me).
> 
> Mauro, should dib7000p_get_time_us() use c->layer[???].interleaving?

I don't think that time interleaving would affect the bit rate.
I suspect that the dead code on dib8000 is just a dead code.

> I don't see a single reference to `layer` in
> drivers/media/dvb-frontends/dib7000p.c.

Layers are specific for ISDB-T, but I think DVB-T (or at least DVB-T2)
may use time interleaving. 

Yet, as I said, the goal is to estimate the streaming bit rate. 

I don't remember anymore from where the dib8000 formula came.

My guts tell that time interleaving shouldn't do much changes (if any)
to the bit rate. I suspect that removing the dead code is likely
OK, but I'll try to see if I can find something related to where this
formula came.

> 
> >  {
> >         struct dtv_frontend_properties *c = &demod->dtv_property_cache;
> >         u64 time_us, tmp64;
> > -       u32 tmp, denom;
> > -       int guard, rate_num, rate_denum = 1, bits_per_symbol;
> > -       int interleaving = 0, fft_div;
> > +       u32 denom;
> > +       int guard, rate_num, rate_denum = 1, bits_per_symbol, fft_div;
> >
> >         switch (c->guard_interval) {
> >         case GUARD_INTERVAL_1_4:
> > @@ -1871,8 +1870,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
> >                 break;
> >         }
> >
> > -       interleaving = interleaving;
> > -
> >         denom = bits_per_symbol * rate_num * fft_div * 384;
> >
> >         /* If calculus gets wrong, wait for 1s for the next stats */
> > @@ -1887,9 +1884,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
> >         time_us += denom / 2;
> >         do_div(time_us, denom);
> >
> > -       tmp = 1008 * 96 * interleaving;
> > -       time_us += tmp + tmp / guard;
> > -
> >         return time_us;
> >  }
> >
> > --
> > 2.18.0
> >  
> 
> 



Thanks,
Mauro

  reply	other threads:[~2018-09-17 22:39 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-15  5:47 Nathan Chancellor
2018-09-17 17:58 ` Nick Desaulniers
2018-09-17 22:39   ` Mauro Carvalho Chehab [this message]
2018-09-19 18:55     ` Nick Desaulniers
2018-10-25 18:09     ` Nathan Chancellor
2018-12-04 10:26     ` Sean Young
2018-12-04 11:57       ` Mauro Carvalho Chehab
2018-12-04 13:39         ` Sean Young
2018-12-04 18:54           ` Nick Desaulniers
2018-12-04 19:04           ` Nathan Chancellor

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180917193936.33e90d5a@coco.lan \
    --to=mchehab+samsung@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=natechancellor@gmail.com \
    --cc=ndesaulniers@google.com \
    --subject='Re: [PATCH] [media] dib7000p: Remove dead code' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).