| Message ID | 20181119005347.747-1-martin@martingkelly.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] iio: bmi160: snap timestamp closer to event | expand |
On Mon, Nov 19, 2018 at 2:54 AM Martin Kelly <martin@martingkelly.com> wrote: > > From: Martin Kelly <martin@martingkelly.com> > > Currently, we snap the timestamp after reading from the buffer and > processing the event. Technically, we can get a slightly more accurate > timestamp by snapping it prior to reading the data, since the data was > already generated prior to entering the trigger handler. This is not going > to make a huge difference, but we might as well improve slightly. > > Signed-off-by: Martin Kelly <martin@martingkelly.com> Makes sense to me. Acked-by: Daniel Baluta <daniel.baluta@gmail.com> Would be nice to see some datapoints and compare the differences. > --- > drivers/iio/imu/bmi160/bmi160_core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c > index c85659ca9507..4d9d59d9e3a9 100644 > --- a/drivers/iio/imu/bmi160/bmi160_core.c > +++ b/drivers/iio/imu/bmi160/bmi160_core.c > @@ -384,6 +384,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p) > { > struct iio_poll_func *pf = p; > struct iio_dev *indio_dev = pf->indio_dev; > + s64 ts = iio_get_time_ns(indio_dev); > struct bmi160_data *data = iio_priv(indio_dev); > __le16 buf[16]; > /* 3 sens x 3 axis x __le16 + 3 x __le16 pad + 4 x __le16 tstamp */ > @@ -399,8 +400,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p) > buf[j++] = sample; > } > > - iio_push_to_buffers_with_timestamp(indio_dev, buf, > - iio_get_time_ns(indio_dev)); > + iio_push_to_buffers_with_timestamp(indio_dev, buf, ts); > done: > iio_trigger_notify_done(indio_dev->trig); > return IRQ_HANDLED; > -- > 2.11.0 >
On 11/19/18 12:45 AM, Daniel Baluta wrote: > On Mon, Nov 19, 2018 at 2:54 AM Martin Kelly <martin@martingkelly.com> wrote: >> >> From: Martin Kelly <martin@martingkelly.com> >> >> Currently, we snap the timestamp after reading from the buffer and >> processing the event. Technically, we can get a slightly more accurate >> timestamp by snapping it prior to reading the data, since the data was >> already generated prior to entering the trigger handler. This is not going >> to make a huge difference, but we might as well improve slightly. >> >> Signed-off-by: Martin Kelly <martin@martingkelly.com> > > Makes sense to me. > > Acked-by: Daniel Baluta <daniel.baluta@gmail.com> > > Would be nice to see some datapoints and compare the differences. > I agree. I'm a bit short of time this week because of Thanksgiving, but I'll try to do a bit of timing testing before and after the patch next week.
On 11/19/18 12:45 AM, Daniel Baluta wrote: > On Mon, Nov 19, 2018 at 2:54 AM Martin Kelly <martin@martingkelly.com> wrote: >> >> From: Martin Kelly <martin@martingkelly.com> >> >> Currently, we snap the timestamp after reading from the buffer and >> processing the event. Technically, we can get a slightly more accurate >> timestamp by snapping it prior to reading the data, since the data was >> already generated prior to entering the trigger handler. This is not going >> to make a huge difference, but we might as well improve slightly. >> >> Signed-off-by: Martin Kelly <martin@martingkelly.com> > > Makes sense to me. > > Acked-by: Daniel Baluta <daniel.baluta@gmail.com> > > Would be nice to see some datapoints and compare the differences. > I agree. I'm a bit short of time this week because of Thanksgiving, but I'll try to do a bit of timing testing before and after the patch next week.
On Sun, 18 Nov 2018 16:53:46 -0800 Martin Kelly <martin@martingkelly.com> wrote: > From: Martin Kelly <martin@martingkelly.com> > > Currently, we snap the timestamp after reading from the buffer and > processing the event. Technically, we can get a slightly more accurate > timestamp by snapping it prior to reading the data, since the data was > already generated prior to entering the trigger handler. This is not going > to make a huge difference, but we might as well improve slightly. > > Signed-off-by: Martin Kelly <martin@martingkelly.com> Now this is always an interesting one. We are running that handler off the back of a trigger that isn't a dataready signal. As such the start of the function doesn't necessarily correspond to the time that is closest to when the data is captured, it corresponds to the time that is closest to when we asked for it (which is not the most relevant number when processing the data) Now, here we are talking on either side of the actual hardware reads, so my suspicion is that it'll be just as inaccurate, but in the other direction. Also note that the compiler is probably within it's rights to reorder that entirely if it can tell there are no side effects (which it probably can't here... so this change will actually do what you intend.) So upshot is I'm currently unconvinced either way on whether this is a useful change or not! Jonathan > --- > drivers/iio/imu/bmi160/bmi160_core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c > index c85659ca9507..4d9d59d9e3a9 100644 > --- a/drivers/iio/imu/bmi160/bmi160_core.c > +++ b/drivers/iio/imu/bmi160/bmi160_core.c > @@ -384,6 +384,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p) > { > struct iio_poll_func *pf = p; > struct iio_dev *indio_dev = pf->indio_dev; > + s64 ts = iio_get_time_ns(indio_dev); > struct bmi160_data *data = iio_priv(indio_dev); > __le16 buf[16]; > /* 3 sens x 3 axis x __le16 + 3 x __le16 pad + 4 x __le16 tstamp */ > @@ -399,8 +400,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p) > buf[j++] = sample; > } > > - iio_push_to_buffers_with_timestamp(indio_dev, buf, > - iio_get_time_ns(indio_dev)); > + iio_push_to_buffers_with_timestamp(indio_dev, buf, ts); > done: > iio_trigger_notify_done(indio_dev->trig); > return IRQ_HANDLED;
On 11/25/18 5:59 AM, Jonathan Cameron wrote: > On Sun, 18 Nov 2018 16:53:46 -0800 > Martin Kelly <martin@martingkelly.com> wrote: > >> From: Martin Kelly <martin@martingkelly.com> >> >> Currently, we snap the timestamp after reading from the buffer and >> processing the event. Technically, we can get a slightly more accurate >> timestamp by snapping it prior to reading the data, since the data was >> already generated prior to entering the trigger handler. This is not going >> to make a huge difference, but we might as well improve slightly. >> >> Signed-off-by: Martin Kelly <martin@martingkelly.com> > Now this is always an interesting one. We are running that handler > off the back of a trigger that isn't a dataready signal. As such > the start of the function doesn't necessarily correspond to the > time that is closest to when the data is captured, it corresponds > to the time that is closest to when we asked for it (which is > not the most relevant number when processing the data) > > Now, here we are talking on either side of the actual hardware > reads, so my suspicion is that it'll be just as inaccurate, but > in the other direction. > > Also note that the compiler is probably within it's rights to > reorder that entirely if it can tell there are no side effects > (which it probably can't here... so this change will actually > do what you intend.) > > So upshot is I'm currently unconvinced either way on whether this > is a useful change or not! > > Jonathan > Yes, you are right that in the current driver, it's rather fuzzy when the "correct" timestamp to snap is. Separately, I'm working on adding interrupt support to this driver, so perhaps I should queue this up after that change. With interrupts (based on a data ready signal), I believe this change will be correct. > >> --- >> drivers/iio/imu/bmi160/bmi160_core.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c >> index c85659ca9507..4d9d59d9e3a9 100644 >> --- a/drivers/iio/imu/bmi160/bmi160_core.c >> +++ b/drivers/iio/imu/bmi160/bmi160_core.c >> @@ -384,6 +384,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p) >> { >> struct iio_poll_func *pf = p; >> struct iio_dev *indio_dev = pf->indio_dev; >> + s64 ts = iio_get_time_ns(indio_dev); >> struct bmi160_data *data = iio_priv(indio_dev); >> __le16 buf[16]; >> /* 3 sens x 3 axis x __le16 + 3 x __le16 pad + 4 x __le16 tstamp */ >> @@ -399,8 +400,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p) >> buf[j++] = sample; >> } >> >> - iio_push_to_buffers_with_timestamp(indio_dev, buf, >> - iio_get_time_ns(indio_dev)); >> + iio_push_to_buffers_with_timestamp(indio_dev, buf, ts); >> done: >> iio_trigger_notify_done(indio_dev->trig); >> return IRQ_HANDLED; >
On Mon, 26 Nov 2018 15:43:37 -0800 Martin Kelly <martin@martingkelly.com> wrote: > On 11/25/18 5:59 AM, Jonathan Cameron wrote: > > On Sun, 18 Nov 2018 16:53:46 -0800 > > Martin Kelly <martin@martingkelly.com> wrote: > > > >> From: Martin Kelly <martin@martingkelly.com> > >> > >> Currently, we snap the timestamp after reading from the buffer and > >> processing the event. Technically, we can get a slightly more accurate > >> timestamp by snapping it prior to reading the data, since the data was > >> already generated prior to entering the trigger handler. This is not going > >> to make a huge difference, but we might as well improve slightly. > >> > >> Signed-off-by: Martin Kelly <martin@martingkelly.com> > > Now this is always an interesting one. We are running that handler > > off the back of a trigger that isn't a dataready signal. As such > > the start of the function doesn't necessarily correspond to the > > time that is closest to when the data is captured, it corresponds > > to the time that is closest to when we asked for it (which is > > not the most relevant number when processing the data) > > > > Now, here we are talking on either side of the actual hardware > > reads, so my suspicion is that it'll be just as inaccurate, but > > in the other direction. > > > > Also note that the compiler is probably within it's rights to > > reorder that entirely if it can tell there are no side effects > > (which it probably can't here... so this change will actually > > do what you intend.) > > > > So upshot is I'm currently unconvinced either way on whether this > > is a useful change or not! > > > > Jonathan > > > > Yes, you are right that in the current driver, it's rather fuzzy when > the "correct" timestamp to snap is. Separately, I'm working on adding > interrupt support to this driver, so perhaps I should queue this up > after that change. With interrupts (based on a data ready signal), I > believe this change will be correct. Agreed. That is the time to make this change as then it'll be obvious why. Thanks, Jonathan > > > > >> --- > >> drivers/iio/imu/bmi160/bmi160_core.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c > >> index c85659ca9507..4d9d59d9e3a9 100644 > >> --- a/drivers/iio/imu/bmi160/bmi160_core.c > >> +++ b/drivers/iio/imu/bmi160/bmi160_core.c > >> @@ -384,6 +384,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p) > >> { > >> struct iio_poll_func *pf = p; > >> struct iio_dev *indio_dev = pf->indio_dev; > >> + s64 ts = iio_get_time_ns(indio_dev); > >> struct bmi160_data *data = iio_priv(indio_dev); > >> __le16 buf[16]; > >> /* 3 sens x 3 axis x __le16 + 3 x __le16 pad + 4 x __le16 tstamp */ > >> @@ -399,8 +400,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p) > >> buf[j++] = sample; > >> } > >> > >> - iio_push_to_buffers_with_timestamp(indio_dev, buf, > >> - iio_get_time_ns(indio_dev)); > >> + iio_push_to_buffers_with_timestamp(indio_dev, buf, ts); > >> done: > >> iio_trigger_notify_done(indio_dev->trig); > >> return IRQ_HANDLED; > > >
diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c index c85659ca9507..4d9d59d9e3a9 100644 --- a/drivers/iio/imu/bmi160/bmi160_core.c +++ b/drivers/iio/imu/bmi160/bmi160_core.c @@ -384,6 +384,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p) { struct iio_poll_func *pf = p; struct iio_dev *indio_dev = pf->indio_dev; + s64 ts = iio_get_time_ns(indio_dev); struct bmi160_data *data = iio_priv(indio_dev); __le16 buf[16]; /* 3 sens x 3 axis x __le16 + 3 x __le16 pad + 4 x __le16 tstamp */ @@ -399,8 +400,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p) buf[j++] = sample; } - iio_push_to_buffers_with_timestamp(indio_dev, buf, - iio_get_time_ns(indio_dev)); + iio_push_to_buffers_with_timestamp(indio_dev, buf, ts); done: iio_trigger_notify_done(indio_dev->trig); return IRQ_HANDLED;