Message-ID: <20200621195347.GA59797@shinobu>
Date:   Sun, 21 Jun 2020 15:53:47 -0400
From:   William Breathitt Gray <vilhelm.gray@...il.com>
To:     David Lechner <david@...hnology.com>
Cc:     jic23@...nel.org, kamel.bouhara@...tlin.com, gwendal@...omium.org,
        alexandre.belloni@...tlin.com, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, syednwaris@...il.com,
        patrick.havelange@...ensium.com, fabrice.gasnier@...com,
        mcoquelin.stm32@...il.com, alexandre.torgue@...com
Subject: Re: [PATCH v3 3/4] counter: Add character device interface

On Sat, Jun 20, 2020 at 01:50:59PM -0500, David Lechner wrote:
> On 6/16/20 8:40 PM, William Breathitt Gray wrote:
> > This patch introduces a character device interface for the Counter
> > subsystem. Device control is exposed through standard character device
> > read and write operations.
> > 
> > A /sys/bus/counter/devices/counterX/chrdev_format sysfs attribute is
> > introduced to expose the character device data format:
> > 
> > * Users may write to this sysfs attribute to select the components they
> >    want to interface -- the layout can be determined as well from the
> >    order. For example:
> > 
> >    # echo "C0 C3 C2" > /sys/bus/counter/devices/counter0/chrdev_format
> > 
> >    This would select Counts 0, 3, and 2 (in that order) to be available
> >    in the /dev/counter0 node as a contiguous memory region.
> > 
> >    You can select extensions in a similar fashion:
> > 
> >    # echo "C4E2 S1E0" > /sys/bus/counter/devices/counter0/chrdev_format
> > 
> >    This would select extension 2 from Count 4, and extension 0 from
> >    Signal 1.
> > 
> > * Users may read from this chrdev_format sysfs attribute in order to see
> >    the currently configured format of the character device.
> > 
> > * Users may perform read/write operations on the /dev/counterX node
> >    directly; the layout of the data is what they user has configured via
> >    the chrdev_format sysfs attribute. For example:
> > 
> >    # echo "C0 C1 S0 S1" > /sys/bus/counter/devices/counter0/chrdev_format
> > 
> >    Yields the following /dev/counter0 memory layout:
> > 
> >    +-----------------+------------------+----------+----------+
> >    | Byte 0 - Byte 7 | Byte 8 - Byte 15 | Byte 16  | Byte 17  |
> >    +-----------------+------------------+----------+----------+
> >    | Count 0         | Count 1          | Signal 0 | Signal 2 |
> >    +-----------------+------------------+----------+----------+
> > 
> >    The number of bytes allotted for each component or extension is
> >    determined by its respective data type: u8 will have 1 byte allotted,
> >    u64 will have 8 bytes allotted, etc.
> > 
> 
> Instead of the proposed character device, I would really rather have one
> that gives past events instead of the current state.
> 
> I have thought about some of the suggestions from previous version of
> this patch series and I'm starting to think something similar to the
> input and gpio subsystems would work fairly well.
> 
> 
> There would have to be a fixed size event data structure:
> 
> struct counter_event {
> 	/** Best approximation of when event occurred in nanoseconds. */
> 	__u64 timestamp;
> 	/**
> 	 * Description of the synapse that triggered the event and the
> 	 * signal/counter that the value represents.
> 	 */
> 	__u64 descriptor;
> 	/** The signal/counter value recorded when the synapse fired. */
> 	__u64 value;
> };
> 
> The descriptor field would actually probably be a union of __u64 and a
> struct with its own fields to describe the synapse and signal or count.
> 
> If a synapse involves more than one signal or count, then there would
> be multiple events with identical timestamps.
> 
> Userspace programs should be able to enable only the events/synapses they
> are interested in and then the poll() the character device to wait for
> events in an efficient way instead of having to constantly read - which
> could still miss events.
> 
> ---
> 
> Real world use case - measuring the speed of a motor:
> 
> At low speeds it is more accurate to measure the time difference between
> count events. In this case we would want events from two synapses. One
> triggered by the rising and falling edges of signal A and one triggered
> by the direction signal. The magnitude of the speed is calculated by
> taking the difference in timestamps between signal A events and the +/-
> sign is determined by the direction signal.
> 
> At high speeds a different configuration is needed. Assuming the counter
> has a timer clock signal a synapse would be configured to fire every 10
> or 20 milliseconds. This would trigger an event that contains the count.
> The speed is calculated as the difference in counts divided by the fixed
> time interval.
> 
> Some applications may need to do both and be able to change the
> configuration at runtime. It may start out in the low speed configuration,
> but as the speed increases, events triggered by the change in count will
> need to be disabled to prevent being overwhelmed with too many count
> events. But if the speed drops low again, the count events will need to
> be turned back on.
> 
> ---
> 
> Regarding the implementation, the character device can be backed by a
> kfifo. Interrupts from the counter hardware push events to the kfifo
> and reading from the character device drains the kfifo.
> 
> drivers/gpio/gpiolib.c could be a good example to follow.
> 
> If we only want to allow one consumer to open the chardev at a time,
> then enabling/disabling events via sysfs would probably be fine since
> we are already sort of doing that anyway to enable/disable counters.
> But if we need to allow multiple consumers per chardev that would each
> want different events, then configuring via ioctl would be required so
> that per-file descriptor configuration could be done (each call to open()
> would create a new kfifo and ioctl would configure what gets pushed to
> that kfifo).

Perhaps structuring the interface around events would be a good approach
afterall. The way I see it is that we have two types of use cases to
support: those that are sensitive to latency and those that are not. Use
cases where latency is not a concern can simply use the existing sysfs
interface, so really all we care about in this discussion are the
latency-sensitive use cases.

If a user cares about latency, I think it's because their use case
involves some sort of realtime deadline. In this kind of situation, I
suspect they will not simply be checking the current count in a loop,
but rather design their system around interrupts from the counter device
to notify their application when some deadline or condition has been
met.

Because a latency-conscious user would be waiting on events, the Counter
character device interface should be structured around configuring and
receiving events with respective data. The user should be able to choose
which events they want to receive, as well as the particular data they
want when that event occurs.

Synapses simply indicate a change in a Count value; every signal Count
value change is not necessarily interesting, so Counter events would
likely be defined separately from Synapses. Regardless, users would need
the ability to choose the particular data they want associated with a
particular event.

For example, in the dual-axes positioning table scenario, a user
application would likely want to know the exact X and Y position at the
time of a given event -- that means an event should provide two Count
values (and possibly associated device flags) when it occurs. I'm not
sure yet how the struct counter_event should be defined in order to
support this; we will need to indicate the format of data as well as
provide the data itself. Perhaps, we can handle this by providing an
unique id field so that only a single datum (e.g. a single count value)
is provided via the value field, but subsequent struct counter_event
items share the same id so that the user knows that a particular datum
is part of a larger group of data for a specific event.

William Breathitt Gray

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists