Re: [PATCH RFC] counter: Expand API with a function for an exact timestamp

[William Breathitt Gray <vilhelm.gray@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5223 bytes --]

On Tue, Dec 07, 2021 at 07:10:45PM +0100, Uwe Kleine-König wrote:
> Some hardware units capture a timestamp for the counted event. To
> increase precision add a variant of counter_push_event() that allows
> passing this timestamp.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> the difficulty is that the captured timer value is in a unit different
> from the one provided by ktime_get_ns(). So maybe some helper functions
> will be needed to convert the local timer value to a ktime timestamp?
> 
> So usage would be something like:
> 
> 	ktime_now = ktime_get_ns();
> 	local_now = readl(CNT);
> 	local_event = readl(...);
> 
> 	ktime_event = ktime_now - (local_now - local_event) * somefactor >> someshift;
> 
> 	counter_push_event_ts(count, event, channel, ktime_event);
> 
> This improves the precision because irq latency doesn't influence
> the resulting timestamp. The precision then only depends on the timer
> resolution and the delay between ktime_get_ns() and readl(CNT);
> 
> I don't have a driver (yet) that makes use of this, the hardware where
> this will matter will be stm32mp1.
> 
> Best regards
> Uwe

Hello Uwe,

Precision logging of counter events was one of the main motivations for
the creation of the Counter character device interface, so if we can
reduce the jitter of the timestamp by utilizing hardware-provided
values, I'm all for it. That being said, we should take care in deciding
how to expose this data in the Counter interfaces because not all
devices support such functionality and yet users should expect a
standard data format to code against.

Considering this, I think it better to keep the struct counter_event
timestamp the way it is right now as ktime_get_ns() so that users have a
consistent way to retrieve the timestamp of when the Counter event was
pushed to the queue. In order to support the hardware-provided
timestamp, how about exposing local_event and local_now as Counter
extensions? You can set a watches for the local timestamps to log them
into the queue with each event, then perform the ktime_event calculation
in userspace using the struct counter_event timestamp value.

William Breathitt Gray

> 
>  drivers/counter/counter-chrdev.c | 25 +++++++++++++++++++++----
>  include/linux/counter.h          |  2 ++
>  2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
> index b7c62f957a6a..6381f7246d59 100644
> --- a/drivers/counter/counter-chrdev.c
> +++ b/drivers/counter/counter-chrdev.c
> @@ -521,16 +521,17 @@ static int counter_get_data(struct counter_device *const counter,
>  }
>  
>  /**
> - * counter_push_event - queue event for userspace reading
> + * counter_push_event_ts - queue event with a timestamp for userspace reading
>   * @counter:	pointer to Counter structure
>   * @event:	triggered event
>   * @channel:	event channel
> + * @timestamp:	the ktime when the event occurred
>   *
>   * Note: If no one is watching for the respective event, it is silently
>   * discarded.
>   */
> -void counter_push_event(struct counter_device *const counter, const u8 event,
> -			const u8 channel)
> +void counter_push_event_ts(struct counter_device *const counter, const u8 event,
> +			   const u8 channel, u64 timestamp)
>  {
>  	struct counter_event ev;
>  	unsigned int copied = 0;
> @@ -538,7 +539,7 @@ void counter_push_event(struct counter_device *const counter, const u8 event,
>  	struct counter_event_node *event_node;
>  	struct counter_comp_node *comp_node;
>  
> -	ev.timestamp = ktime_get_ns();
> +	ev.timestamp = timestamp;
>  	ev.watch.event = event;
>  	ev.watch.channel = channel;
>  
> @@ -570,4 +571,20 @@ void counter_push_event(struct counter_device *const counter, const u8 event,
>  	if (copied)
>  		wake_up_poll(&counter->events_wait, EPOLLIN);
>  }
> +EXPORT_SYMBOL_GPL(counter_push_event_ts);
> +
> +/**
> + * counter_push_event - queue event for userspace reading
> + * @counter:	pointer to Counter structure
> + * @event:	triggered event
> + * @channel:	event channel
> + *
> + * Note: If no one is watching for the respective event, it is silently
> + * discarded.
> + */
> +void counter_push_event(struct counter_device *const counter, const u8 event,
> +			const u8 channel)
> +{
> +	counter_push_event_ts(counter, event, channel, ktime_get_ns());
> +}
>  EXPORT_SYMBOL_GPL(counter_push_event);
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> index b7d0a00a61cf..596e7e58e463 100644
> --- a/include/linux/counter.h
> +++ b/include/linux/counter.h
> @@ -333,6 +333,8 @@ int counter_register(struct counter_device *const counter);
>  void counter_unregister(struct counter_device *const counter);
>  int devm_counter_register(struct device *dev,
>  			  struct counter_device *const counter);
> +void counter_push_event_ts(struct counter_device *const counter, const u8 event,
> +			   const u8 channel, u64 timestamp)
>  void counter_push_event(struct counter_device *const counter, const u8 event,
>  			const u8 channel);
>  
> -- 
> 2.30.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]