LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Liam Beguin <liambeguin@gmail.com>
To: Peter Rosin <peda@axentia.se>
Cc: Jonathan Cameron <jic23@kernel.org>,
	lars@metafoo.de, linux-kernel@vger.kernel.org,
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	robh+dt@kernel.org
Subject: Re: [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small fractional scales
Date: Wed, 1 Sep 2021 22:27:15 -0400	[thread overview]
Message-ID: <YTA2g2iHB3WtEMBp@shaak> (raw)
In-Reply-To: <e1542f14-f271-a0a3-eaa1-092e12c3ed6c@axentia.se>

On Mon, Aug 30, 2021 at 04:30:54PM +0200, Peter Rosin wrote:
> On 2021-08-30 13:22, Jonathan Cameron wrote:
> > On Mon, 23 Aug 2021 00:18:55 +0200
> > Peter Rosin <peda@axentia.se> wrote:
> > 
> >> [I started to write an answer to your plans in the v7 thread, but didn't
> >> have time to finish before v8 appeared...]
> >>
> >> On 2021-08-20 21:17, Liam Beguin wrote:
> >>> From: Liam Beguin <lvb@xiphos.com>
> >>>
> >>> The approximation caused by integer divisions can be costly on smaller
> >>> scale values since the decimal part is significant compared to the
> >>> integer part. Switch to an IIO_VAL_INT_PLUS_NANO scale type in such
> >>> cases to maintain accuracy.  
> >>
> >> The conversion to int-plus-nano may also carry a cost of accuracy.
> >>
> >> 90/1373754273 scaled by 261/509 is 3.359e-8, the old code returns 3.348e-8,
> >> but the new one gets you 3.3e-8 (0.000000033, it simply cannot provide more
> >> digits). So, in this case you lose precision with the new code.
> >>
> >> Similar problem with 100 / 2^30 scaled by 3782/7000. It is 5.032e-8, the old
> >> code returns 5.029e-8, but the new one gets you the inferior 5.0e-8.
> >>
> >> I'm also wondering if it is wise to not always return the same scale type?
> >> What happens if we want to extend this driver to scale a buffered channel?
> >> Honest question! I don't know, but I fear that this patch may make that
> >> step more difficult to take??
> >>
> >> Jonathan, do you have any input on that?
> > 
> > I'm a bit lost.  As things currently stand IIO buffered data flows all use
> > _RAW.  It's either up to userspace or the inkernel user to query scale
> > and use that to compute the appropriate _processed values.  There have been
> > various discussions over the years on how to add metadata but it's tricky
> > without adding significant complexity and for vast majority of usecases not
> > necessary.  Given the rescaler copes with _raw and _processed inputs, we
> > would only support buffered flows if using the _raw ones.
> > 
> > If nothing changes in configuration of the rescaler, the scale should be
> > static for a given device.  What format that 'scale' takes is something
> > that userspace code or inkernel users should cope fine with given they
> > need to do that anyway for different devices.
> 
> Ok, if 'scale' (and 'offset') of the source channel is to be considered
> static, then it is much safer to ignore the "island problem" and rescale
> each value independently on a case-by-case basis. We should add an
> explicit comment somewhere that we make this assumption.
> 
> Sorry for wasting time and effort by not realizing by myself (and earlier).

No worries, I was also under the impression that the source channel
scale (and offset) could change.

> 
> Maybe something like this?
> 
> /*
>  * The rescaler assumes that the 'scale' and 'offset' properties of
>  * the source channel are static. If they are not, there exist some
>  * corner cases where rounding/truncating might cause confusing
>  * mathematical properties (non-linearity).
>  */
> 

If this is more of a general assumption, is there a place in the
Documentation/ that we could put this comment?

If not, I'll add it here.

> I then propose that we rescale IIO_VAL_FRACTIONAL as before if that works,
> thus preserving any previous exact rescaling, but if there is an overflow
> while doing that, then we fall back to new code that rescales to a
> IIO_VAL_INT_PLUS_NANO value. Trying the gcd-thing as it ended up in v7 still
> seems expensive to me, but maybe I overestimate the cost of gcd? Anyway, my
> guts vote for completely skipping gcd and that we aim directly for
> IIO_VAL_INT_PLUS_NANO in case of overflow while doing the old thing.

I agree with you, I'd much rather drop gcd() from rescale_process_scale()
altogether.

I was planning on keeping IIO_VAL_FRACTIONAL and IIO_VAL_FRACTIONAL_LOG2
combined, but getting rid of the 100ppm condition in favor of a simple
if (rem).

> 
> Having said that, if 'scale' and 'offset' indeed are static, then the gcd
> cost can be mitigated by caching the result. Exact rescaling is always
> nice...
> 
> If IIO_VAL_INT overflows while rescaling, we are SOL whichever way we turn,
> so ignore doing anything about that.

I was thinking of using check_mul_overflow() to do something about the
overflow, but I'm happy to leave it out for now.

> 
> Liam, was it IIO_VAL_FRACTIONAL or IIO_VAL_FRACTIONAL_LOG2 that was your
> real use case? Anyway, your 100 ppm threshold is probably as good a
> compromise as any for this case. However, that threshold does nothing for
> the case where the conversion to IIO_VAL_INT_PLUS_NANO throws away way
> more precision than the existing operations. It would probably be good
> to somehow stay with the old method for the really tiny values, if there
> is a viable test/threshold for when to do what.
> 

My original issue was with IIO_VAL_FRACTIONAL.
I'll look into what we can do for small IIO_VAL_FRACTIONAL_LOG2 values,
and will add more tests for that too.

Thanks,
Liam

> Cheers,
> Peter

  parent reply	other threads:[~2021-09-02  2:27 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-20 19:17 [PATCH v8 00/14] iio: afe: add temperature rescaling support Liam Beguin
2021-08-20 19:17 ` [PATCH v8 01/14] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
2021-08-20 19:17 ` [PATCH v8 02/14] iio: inkern: apply consumer scale when no channel scale is available Liam Beguin
2021-08-20 19:17 ` [PATCH v8 03/14] iio: inkern: make a best effort on offset calculation Liam Beguin
2021-08-20 19:17 ` [PATCH v8 04/14] iio: afe: rescale: expose scale processing function Liam Beguin
2021-08-20 19:17 ` [PATCH v8 05/14] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
2021-08-26  8:11   ` Peter Rosin
2021-08-29  2:50     ` Liam Beguin
2021-08-20 19:17 ` [PATCH v8 06/14] iio: afe: rescale: add offset support Liam Beguin
2021-08-20 19:17 ` [PATCH v8 07/14] iio: afe: rescale: use s64 for temporary scale calculations Liam Beguin
2021-08-20 19:17 ` [PATCH v8 08/14] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
2021-08-26  9:13   ` Peter Rosin
2021-08-29  4:01     ` Liam Beguin
2021-08-20 19:17 ` [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small fractional scales Liam Beguin
2021-08-20 23:37   ` kernel test robot
2021-08-21  1:33     ` Liam Beguin
2021-08-23  6:53       ` Peter Rosin
2021-08-21  2:00   ` kernel test robot
2021-08-21  7:21   ` kernel test robot
2021-08-22 22:18   ` Peter Rosin
2021-08-24 20:28     ` [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small Liam Beguin
2021-08-26  9:53       ` Peter Rosin
2021-08-29  4:41         ` Liam Beguin
2021-08-30 11:27           ` Jonathan Cameron
2021-09-11 23:31             ` Liam Beguin
2021-08-30 13:03           ` Peter Rosin
2021-09-11 23:20             ` Liam Beguin
2021-08-30 11:22     ` [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small fractional scales Jonathan Cameron
2021-08-30 14:30       ` Peter Rosin
2021-08-30 17:03         ` Jonathan Cameron
2021-09-02  2:27         ` Liam Beguin [this message]
2021-09-02 21:52           ` Peter Rosin
2021-08-20 19:17 ` [PATCH v8 10/14] iio: test: add basic tests for the iio-rescale driver Liam Beguin
2021-08-20 19:17 ` [PATCH v8 11/14] iio: afe: rescale: add RTD temperature sensor support Liam Beguin
2021-08-20 19:17 ` [PATCH v8 12/14] iio: afe: rescale: add temperature transducers Liam Beguin
2021-08-26  8:56   ` Peter Rosin
2021-08-29  2:33     ` Liam Beguin
2021-08-20 19:17 ` [PATCH v8 13/14] dt-bindings: iio: afe: add bindings for temperature-sense-rtd Liam Beguin
2021-08-20 19:17 ` [PATCH v8 14/14] dt-bindings: iio: afe: add bindings for temperature transducers Liam Beguin

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=YTA2g2iHB3WtEMBp@shaak \
    --to=liambeguin@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jic23@kernel.org \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peda@axentia.se \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small fractional scales' \
    /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).