[RFC] iio: Add support for describing scan offsets in buffer ops

[RFC] iio: Add support for describing scan offsets in buffer ops

[Crestez Dan Leonard]

Hello,

This is a tentative proposal with no code attached. This issue came up
while hacking on the mpu6050 driver but it applies to other complex
devices and a solution belongs in the IIO core.

One IIO limitation is that while samples can be described through a
relatively generic scan_type they must be placed in the output buffer
according to an specific iio alignment algorithm. For example if the
device has one 16bit channel and one 32bit channel the output needs to
have 16bits of padding between them. When this does not match the
hardware format the solution is for the driver to unpack the hardware
samples and repack them in the iio format.

I propose that we add a new function in iio_buffer_setup_ops which looks
like this:

int (*get_scan_offsets)(struct iio_dev *indio_dev,
                        off_t *scan_offsets,
			size_t *sample_bytes);

This would be called when enabling a buffer to fetch the offset in the
sample for each channel (by scan_index) based on the currently
configured scan mask. Then the iio core would then handle rearranging
sample bytes as expected by the buffer consumer. Driver implementations
could look something like this:

off_t pos = 0;
if (scan_mask & (1 << SCAN_INDEX_TEMP))) {
    scan_offsets[SCAN_INDEX_TEMP] = pos++;
}
/* No 2-bytes alignment here! */
if (scan_mask & SCAN_MASK_ACCEL_ANY) {
    scan_offsets[SCAN_INDEX_ACCEL_X] = pos + 0;
    scan_offsets[SCAN_INDEX_ACCEL_Y] = pos + 2;
    scan_offsets[SCAN_INDEX_ACCEL_Z] = pos + 4;
    pos += 6;
}
*sample_bytes = pos;

This could be implemented as an extension to the current buffer demuxing
support which currently only deals with differences in scan_masks. This
mechanism would very nicely deal with devices where it is not possible
to select X/Y/Z individually because the iio core would just ignore the
gap in offsets.

This can even be exposed to userspace in a compatible manner. User would
do something like:

    ioctl(iio_fd, IIO_USER_HANDLES_SCAN_OFFSETS, 1);
    ioctl(iio_fd, IIO_GET_SCAN_OFFSETS, &scan_offsets, &sample_bytes);

When the consumer is marked as aware of scan offsets this way the iio
core can skip demuxing for increased performance. The IIO core can
implement IIO_GET_SCAN_OFFSETS itself when the driver has no custom
implementation. An application could rely on IIO_GET_SCAN_OFFSETS for
all devices as long as it's acceptable to ignore old kernels.

This would be particularly useful if IIO sample buffers are
memory-mapped eventually. I think that this extension would be
incrementally useful to that effort.

-- 
Regards,
Leonard

Re: [RFC] iio: Add support for describing scan offsets in buffer ops

[Jonathan Cameron]

On 03/05/16 14:37, Crestez Dan Leonard wrote:
> Hello,
> 
> This is a tentative proposal with no code attached. This issue came up
> while hacking on the mpu6050 driver but it applies to other complex
> devices and a solution belongs in the IIO core.
I'm not so convinced (yet).  This is actually a fairly unusual case and the
'byte mangling' can often be handled in a relatively fixed way in a
driver whereas it can be more complex in the core.

Interesting idea though..  Effectively you are hand specifying the
demux tables that we generate and making them more flexible.
> 
> One IIO limitation is that while samples can be described through a
> relatively generic scan_type they must be placed in the output buffer
> according to an specific iio alignment algorithm. For example if the
> device has one 16bit channel and one 32bit channel the output needs to
> have 16bits of padding between them. When this does not match the
> hardware format the solution is for the driver to unpack the hardware
> samples and repack them in the iio format.
It's pretty much got to happen somewhere to get the data into a form that
can be used at speed. Remember we are doing this on the back of expensive
bus transactions (a lot of the time).
> 
> I propose that we add a new function in iio_buffer_setup_ops which looks
> like this:
> 
> int (*get_scan_offsets)(struct iio_dev *indio_dev,
>                         off_t *scan_offsets,
> 			size_t *sample_bytes);
> 
> This would be called when enabling a buffer to fetch the offset in the
> sample for each channel (by scan_index) based on the currently
> configured scan mask. Then the iio core would then handle rearranging
> sample bytes as expected by the buffer consumer. Driver implementations
> could look something like this:
> 
> off_t pos = 0;
> if (scan_mask & (1 << SCAN_INDEX_TEMP))) {
>     scan_offsets[SCAN_INDEX_TEMP] = pos++;
> }
> /* No 2-bytes alignment here! */
> if (scan_mask & SCAN_MASK_ACCEL_ANY) {
>     scan_offsets[SCAN_INDEX_ACCEL_X] = pos + 0;
>     scan_offsets[SCAN_INDEX_ACCEL_Y] = pos + 2;
>     scan_offsets[SCAN_INDEX_ACCEL_Z] = pos + 4;
>     pos += 6;
> }
> *sample_bytes = pos;
Sure - this side is straight forward enough.  It's the core complexity
that makes me wonder if this is worthwhile.  Btw if this is worth doing
then we should really be looking at non byte aligned packed records as
well (just to make it really hideous!)

The killer in here is endian requirements. If the core is repacking data
it will need to understand these completely.  Two options exist, either
the core needs to 'edit' the shift provided by the driver, or it needs
to do endian dependent data realignment.

Take a typical 3 byte unaligned packed record being expanded to a
4 byte aligned record.  I think the easy cases are:

ABC -> 0ABC
CBA -> CBA0
so endian dependent shifts.

Whereas a repack in the driver should inherently know what the right combination
should be. 

I'm not saying it couldn't be done in the core of course, merely that
it is more complex that it initially looks.

I've already suggested we might look at doing a few 'special cases'
to relax the power of two requirement - but I'd do that off the back of
the current description rather than introducing another.  It does have
the same complexity issues though so until we have code I'm
unconvinced it is worth it.

> 
> This could be implemented as an extension to the current buffer demuxing
> support which currently only deals with differences in scan_masks. This
> mechanism would very nicely deal with devices where it is not possible
> to select X/Y/Z individually because the iio core would just ignore the
> gap in offsets.
> 
> This can even be exposed to userspace in a compatible manner. User would
> do something like:
> 
>     ioctl(iio_fd, IIO_USER_HANDLES_SCAN_OFFSETS, 1);
>     ioctl(iio_fd, IIO_GET_SCAN_OFFSETS, &scan_offsets, &sample_bytes);
> 
> When the consumer is marked as aware of scan offsets this way the iio
> core can skip demuxing for increased performance. The IIO core can
> implement IIO_GET_SCAN_OFFSETS itself when the driver has no custom
> implementation. An application could rely on IIO_GET_SCAN_OFFSETS for
> all devices as long as it's acceptable to ignore old kernels.
Be careful here. The demux wasn't actually written in the first
place for this usage at all (it was an added bonus)

It's about sending different streams
of data to different consumers. That is definitely not compatible
with passing this stuff up to user space as it's downright complex
to do.  Making in kernel consumers aware of this more complex handling
is going to be fiddly.  Again could be done, but is it worth it?
I suppose we could make the demux aware of what it is sending to, but
that adds more complexity.
> 
> This would be particularly useful if IIO sample buffers are
> memory-mapped eventually. I think that this extension would be
> incrementally useful to that effort.

Perhaps.  I'm interested to hear other views on this suggestion, but
right now I'm not convinced.  I'd argue that we want clean data
presentations to userspace and that the restrictions as they currently
stand are not really that burdensome on the data.

The exception is dma buffers - in those cases it really might be the
case that we want to allow more complex alignment.

Right now there is no option to have multiple consumers for a dma
buffer and I'm not sure there ever will be.  However, they don't
fall into categories where we use the multiconsumer stuff now
(which is mostly about SoC ADCs with lots of different types of
channel for things like thermal / battery monitoring - there are
other issues there between polled and pushed data flows, but that
is a whole different issue!)

Hence for DMA buffers there is no need to enforce the power of
two byte alignment requirements though, the first time this
happens we should think carefully about whether we 'want' to
allow the additional flexibility.

Jonathan
 

Re: [RFC] iio: Add support for describing scan offsets in buffer ops

[Crestez Dan Leonard]

On 05/04/2016 11:44 AM, Jonathan Cameron wrote:
> On 03/05/16 14:37, Crestez Dan Leonard wrote:
>> I propose that we add a new function in iio_buffer_setup_ops which looks
>> like this:
>>
>> int (*get_scan_offsets)(struct iio_dev *indio_dev,
>>                         off_t *scan_offsets,
>> 			size_t *sample_bytes);
>>
>> This would be called when enabling a buffer to fetch the offset in the
>> sample for each channel (by scan_index) based on the currently
>> configured scan mask. Then the iio core would then handle rearranging
>> sample bytes as expected by the buffer consumer. Driver implementations
>> could look something like this:
>>
>> off_t pos = 0;
>> if (scan_mask & (1 << SCAN_INDEX_TEMP))) {
>>     scan_offsets[SCAN_INDEX_TEMP] = pos++;
>> }
>> /* No 2-bytes alignment here! */
>> if (scan_mask & SCAN_MASK_ACCEL_ANY) {
>>     scan_offsets[SCAN_INDEX_ACCEL_X] = pos + 0;
>>     scan_offsets[SCAN_INDEX_ACCEL_Y] = pos + 2;
>>     scan_offsets[SCAN_INDEX_ACCEL_Z] = pos + 4;
>>     pos += 6;
>> }
>> *sample_bytes = pos;
> Sure - this side is straight forward enough.  It's the core complexity
> that makes me wonder if this is worthwhile.  Btw if this is worth doing
> then we should really be looking at non byte aligned packed records as
> well (just to make it really hideous!)

You could have a bunch of channels with different realbits/shift and the
same offset.

> The killer in here is endian requirements. If the core is repacking data
> it will need to understand these completely.  Two options exist, either
> the core needs to 'edit' the shift provided by the driver, or it needs
> to do endian dependent data realignment.
> 
> Take a typical 3 byte unaligned packed record being expanded to a
> 4 byte aligned record.  I think the easy cases are:
> 
> ABC -> 0ABC
> CBA -> CBA0
> so endian dependent shifts.
> 
I'm not sure how unaligned 24bit channels would be handled. Maybe it
could be done storagebits=32, realbits=24, shift=8 or 0 and offsets that
are multiples of 3?

I guess it would have to know about endianness.

>> This would be particularly useful if IIO sample buffers are
>> memory-mapped eventually. I think that this extension would be
>> incrementally useful to that effort.
> 
> Perhaps.  I'm interested to hear other views on this suggestion, but
> right now I'm not convinced.  I'd argue that we want clean data
> presentations to userspace and that the restrictions as they currently
> stand are not really that burdensome on the data.
> 
> The exception is dma buffers - in those cases it really might be the
> case that we want to allow more complex alignment.
> 
The point of exposing this through the iio core to userspace is that it
could be used to support this kind of devices. It's not worth exposing
as ABI unless it can support a lot of stuff at once.

It might make more sense to just provide some helpers for drivers to do
their own easy reformatting.

Re: [RFC] iio: Add support for describing scan offsets in buffer ops

[Jonathan Cameron]

On 4 May 2016 17:01:17 BST, Crestez Dan Leonard <leonard.crestez@intel.com> wrote:
>On 05/04/2016 11:44 AM, Jonathan Cameron wrote:
>> On 03/05/16 14:37, Crestez Dan Leonard wrote:
>>> I propose that we add a new function in iio_buffer_setup_ops which
>looks
>>> like this:
>>>
>>> int (*get_scan_offsets)(struct iio_dev *indio_dev,
>>>                         off_t *scan_offsets,
>>> 			size_t *sample_bytes);
>>>
>>> This would be called when enabling a buffer to fetch the offset in
>the
>>> sample for each channel (by scan_index) based on the currently
>>> configured scan mask. Then the iio core would then handle
>rearranging
>>> sample bytes as expected by the buffer consumer. Driver
>implementations
>>> could look something like this:
>>>
>>> off_t pos = 0;
>>> if (scan_mask & (1 << SCAN_INDEX_TEMP))) {
>>>     scan_offsets[SCAN_INDEX_TEMP] = pos++;
>>> }
>>> /* No 2-bytes alignment here! */
>>> if (scan_mask & SCAN_MASK_ACCEL_ANY) {
>>>     scan_offsets[SCAN_INDEX_ACCEL_X] = pos + 0;
>>>     scan_offsets[SCAN_INDEX_ACCEL_Y] = pos + 2;
>>>     scan_offsets[SCAN_INDEX_ACCEL_Z] = pos + 4;
>>>     pos += 6;
>>> }
>>> *sample_bytes = pos;
>> Sure - this side is straight forward enough.  It's the core
>complexity
>> that makes me wonder if this is worthwhile.  Btw if this is worth
>doing
>> then we should really be looking at non byte aligned packed records
>as
>> well (just to make it really hideous!)
>
>You could have a bunch of channels with different realbits/shift and
>the
>same offset.
Good point.
>
>> The killer in here is endian requirements. If the core is repacking
>data
>> it will need to understand these completely.  Two options exist,
>either
>> the core needs to 'edit' the shift provided by the driver, or it
>needs
>> to do endian dependent data realignment.
>> 
>> Take a typical 3 byte unaligned packed record being expanded to a
>> 4 byte aligned record.  I think the easy cases are:
>> 
>> ABC -> 0ABC
>> CBA -> CBA0
>> so endian dependent shifts.
>> 
>I'm not sure how unaligned 24bit channels would be handled. Maybe it
>could be done storagebits=32, realbits=24, shift=8 or 0 and offsets
>that
>are multiples of 3?
>
>I guess it would have to know about endianness.
>
>>> This would be particularly useful if IIO sample buffers are
>>> memory-mapped eventually. I think that this extension would be
>>> incrementally useful to that effort.
>> 
>> Perhaps.  I'm interested to hear other views on this suggestion, but
>> right now I'm not convinced.  I'd argue that we want clean data
>> presentations to userspace and that the restrictions as they
>currently
>> stand are not really that burdensome on the data.
>> 
>> The exception is dma buffers - in those cases it really might be the
>> case that we want to allow more complex alignment.
>> 
>The point of exposing this through the iio core to userspace is that it
>could be used to support this kind of devices. It's not worth exposing
>as ABI unless it can support a lot of stuff at once.
>
>It might make more sense to just provide some helpers for drivers to do
>their own easy reformatting.

Certainly seems like that to me. Nothing stops us looking again at this in the
future.

J


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.