On 05/01/15 11:29, Octavian Purdila wrote:
> On Mon, Jan 5, 2015 at 2:07 AM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>> On 21/12/14 00:42, Octavian Purdila wrote:
>>
>> Thanks for taking this on!
>>
>> This all looks fairly sensible, though a few comments inline.
>> One big semantic change I'd suggest is to allow the watermark
>> level passed to the hardware to be a 'hint' rather than a hard
>> and fast rull. A lot of these hardware buffers are fairly small
>> (perhaps 32 entries) and the devices can run fast so whilst
>> we will obviously have to handle an interrupt often, we may well
>> want to have a much larger software front end buffer with a
>> much larger watermark. For example, a 8192Hz accelerometer
>> with 32 entry fifo (watermark at 16). Will fire an interrupt 512 times
>> a second. Userspace might be only interested in getting data 32 times
>> a second and hence want a watermark of 256 entries and probably have
>> a buffer that is 512 entries long or more.
>>
>
> Very good point with the above example, but I find the hint approach a
> bit too hard to diagnose and tune by the application.
>
> Could we perhaps expose the hwfifo watermark as well, in addition to
> the software watermark? We can even keep the hint behavior if the
> application only touches the software watermark, but if it the
> application directly sets the hwfifo level then we use that.
Hmm. Not sure how well this would work. We probably need a way of indicating
the hardware buffer has not been 'pinned' to a particular value.
Maybe we can have it read only?
That way it is obvious what effect the 'hint' is having on the hardware
without adding a confusing double control for the same thing...
>
>>> Some devices have hardware buffers that can store a number of samples
>>> for later consumption. Hardware usually provides interrupts to notify
>>> the processor when the fifo is full or when it has reached a certain
>>> threshold. This helps with reducing the number of interrupts to the
>>> host processor and thus it helps decreasing the power consumption.
>>>
>>> This patch adds support for hardware fifo to IIO by allowing the
>>> drivers to register operations for flushing the hadware fifo and
>>> setting the watermark level.
>>
>> Perhaps put something in here to observe that this is a different approach
>> to the straight hardware buffer stuff we already have - of most interest
>> for hybrid buffering rather than a pure hardware buffer.
>>
>
> Will do.
>
>>>
>>> A driver implementing hardware fifo support must also provide a
>>> watermark trigger which must contain "watermark" in its name.
>>>
>>> Signed-off-by: Octavian Purdila <octavian.purdila@xxxxxxxxx>
>>> ---
>>> Documentation/ABI/testing/sysfs-bus-iio | 22 +++++++++++++++++
>>> drivers/iio/industrialio-buffer.c | 44 ++++++++++++++++++++++++++++-----
>>> include/linux/iio/iio.h | 17 +++++++++++++
>>> 3 files changed, 77 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>>> index 7260f1f..6bb67ac 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>> @@ -1152,3 +1152,25 @@ Description:
>>> Poll will block until the watermark is reached.
>>> Blocking read will wait until the minimum between the requested
>>> read amount or the low water mark is available.
>>> + If the device has a hardware fifo this value is going to be used
>>> + for the hardware fifo watermark as well.
>>
>> I'd make this a litle vaguer (deliberately ;). Perhaps used as a 'hint'
>> for the hardware fifo watermark as well. The reason being that if we set
>> the software watermark at say 20 and the hardware only has 15 fifo entries,
>> then we might want to be clever and say set the hardware fifo watershed to 10.
>> For now that would be a decision of the hardware driver rather than one for
>> the core.
>
> Sure, I'll update the docs with what ever we end-up deciding its best :)
>
>>> +
>>> +What: /sys/bus/iio/devices/iio:deviceX/buffer/hwfifo-length
>>> +KernelVersion: 3.20
>>> +Contact: linux-iio@xxxxxxxxxxxxxxx
>>> +Description:
>>> + A single positive integer specifying the maximum number of
>>> + samples that the hardware fifo has. If the device does not
>>> + support hardware fifo this is zero.
>>> + When a device supports hardware fifo it will expose a trigger
>>> + with the name that contains "watermark"
>>> + (e.g. i2c-BMC150A:00-watermark-dev0).
>>> + To use the hardware fifo the user must set an appropriate value
>>> + in the buffer/length and buffer/low_watermark entries and select
>>> + the watermark trigger. At that poin the hardware fifo will be
>> point
>>> + enabled and the samples will be collected in a hardware buffer.
>> Hmm. I wonder to a degree if the trigger approach really makes sense for
>> fifo equiped devices. We've deliberately not added one in a few existing
>> cases.
>>
>> Otherwise, is there a reason to run this separately from a trigger not using
>> the fifo. Surely that's just the same as a watermark of 1?
>>
>> Anyhow, a point for discussion!
>
> I might be misunderstand you here, but the reason for which we use a
> separate trigger is to have a way to enable/disable FIFO mode, because
> enabling the FIFO might have some downsides, e.g. more power
> consumption. This matters only when the application is interested in
> low latency sampling ( watermark of 1).
Perhaps we use a watermark of one to disable the fifo if present. Can't
think why you'd want it under those circumstances unless the data can't
be read without. This then becomes a driver issue as all the core
cares about is that the data turns up, not particularly whether it very
briefly bounces through a buffer or not.
>
> I don't know if this is really an issue in practice, but I chose the
> safe option.
>
>>
>>> + When the number of samples in the hardware fifo reaches the
>>> + watermark level the watermark trigger is issued and data is
>>> + flushed to the devices buffer.
>>> + A hardware buffer flush will also be triggered when reading from
>>> + the device buffer and there is not enough data available.
>>> \ No newline at end of file
>> Fix this..
>>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>>> index 7f74c7f..3da6d07 100644
>>> --- a/drivers/iio/industrialio-buffer.c
>>> +++ b/drivers/iio/industrialio-buffer.c
>>> @@ -37,9 +37,17 @@ static bool iio_buffer_is_active(struct iio_buffer *buf)
>>> return !list_empty(&buf->buffer_list);
>>> }
>>>
>>> -static size_t iio_buffer_data_available(struct iio_buffer *buf)
>>> +static bool iio_buffer_data_available(struct iio_dev *indio_dev,
>>> + struct iio_buffer *buf, size_t required)
>>> {
>>> - return buf->access->data_available(buf);
>>> + size_t avail = buf->access->data_available(buf);
>>> +
>>> + if (avail < required && indio_dev->hwfifo) {
>>> + indio_dev->hwfifo->flush(indio_dev);
>>> + avail = buf->access->data_available(buf);
>>> + }
>>> +
>>> + return avail >= required;
>> Does it make sense to move the decision in here? Could just as easily
>> have left this as returning the length and done the logic outside...
>> I don't mind that much... However, we probably want to rename the function
>> now that it is doing more than strictly querying availability.
>
> I've put it here so that it affects both read and poll so that we
> avoid avoid latencies if possible. Will change the name.
>
>>
>> Could also have flush take a parameter for what is desired and only read
>> that many? On relatively slow buses might make sense...
>
> Good point, will add that.
>
>>> /**
>>> + * struct iio_buffer_hwfifo_ops - hardware fifo operations
>>> + *
>>> + * @length: [DRIVER] the hardware fifo length
>>> + * @set_watermark: [DRIVER] setups the watermark level
>>> + * @flush: [DRIVER] copies data from the hardware buffer to the
>>> + * device buffer
>>> + * @watermark_trig: [DRIVER] an allocated and registered trigger containing
>>> + * "watermark" in its name
>> err. This last one isn't actually in the structure...
>
> Yeah, I wanted to add some checks in the core to make sure that if the
> driver is registering the hwfifo ops then it should allocate and
> register a watermark trigger as well. If we decide to go with the
> trigger approach, do you think it is a good idea to add the checks?
>
If so, but I'll be honest I really don't like the trigger approach here.
It feels like an abuse of an interface that is really there to allow
synchronized capture/ controllable capture on devices without fixed
timing... (when they are fixed, the point is really to allow other
non fixed devices to lock on and sample at the same time - very handy
for inertial data fusion and other places...)
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
