On Sun, 13 Oct 2024 13:54:36 -0700
Justin Weiss <justin@xxxxxxxxxxxxxxx> wrote:
> Jonathan Cameron <jic23@xxxxxxxxxx> writes:
>
> > On Sat, 12 Oct 2024 19:43:19 -0700
> > Justin Weiss <justin@xxxxxxxxxxxxxxx> wrote:
> >
> >> Jonathan Cameron <jic23@xxxxxxxxxx> writes:
> >>
> >> > On Fri, 11 Oct 2024 08:37:48 -0700
> >> > Justin Weiss <justin@xxxxxxxxxxxxxxx> wrote:
> >> >
> >> >> Set up a triggered buffer for the accel and angl_vel values.
> >> >>
> >> >> Signed-off-by: Justin Weiss <justin@xxxxxxxxxxxxxxx>
> >> > Hi Justin
> >> >
> >> > A few suggestions inline. Other than the DMA safe buffer thing, looks good
> >> > but you might want to consider using a single bulk read.
> >> >
> >> > My cynical view is that if someone paid for an IMU they probably want all
> >> > the channels, so optimizing for that case is a good plan.
> >> >
> >> >> ...
> >> >>
> >> >> + __le16 sample;
> >> >> +
> >> >> + for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
> >> >> + ret = regmap_bulk_read(bmi270_device->regmap,
> >> >> + base + i * sizeof(sample),
> >> >> + &sample, sizeof(sample));
> >> >
> >> > This is always a fun corner.
> >> > regmap doesn't guarantee to bounce buffer the data used by the underlying
> >> > transport. In the case of SPI that means we need a DMA safe buffer for bulk
> >> > accesses. In practice it may well bounce the data today but there are optmizations
> >> > that would make it zero copy that might get applied in future.
> >> >
> >> > Easiest way to do that is put your sample variable in the iio_priv structure
> >> > at the end and mark it __aligned(IIO_DMA_MINALIGN)
> >> >
> >> > Given you are reading a bunch of contiguous registers here it may well make
> >> > sense to do a single bulk read directly into buf and then use
> >> > the available_scan_masks to let the IIO core know it always gets a full set
> >> > of samples. Then if the user selects a subset the IIO core will reorganize
> >> > the data that they get presented with.
> >>
> >> That's convenient :-)
> >>
> >> It should make this much simpler. To clarify, I'll use regmap_bulk_read
> >> to read all of the registers at once into a stack-allocated buffer, and
> >> then push that buffer. Then I can remove bmi270_device->buf entirely,
> >> and avoid the DMA problem that way.
> >
> > Given this supports SPI. The target buffer can't be on the stack.
> > You still need the __aligned(IIO_DMA_MINALIGN) element in your iio_priv()
> > structure.
> >
>
> Got it. I see that the BMI323 driver does the regmap_read into the
> DMA-aligned buffer, and then copies it to the timestamp-aligned buffer,
> which it then sends to iio_push_to_buffers_with_timestamp. Is that a
> good way to handle this?
Yes. We don't have a zero copy path so that's the best we can do.
>
> I think the timestamp-aligned buffer could still be stack-allocated in
> that case.
No it can't. __aligned doesn't do anything useful for us on the stack
as we don't control what comes after it. You could in theory force alignment
and pad - as long as compilers now respect __aligned for this case (they
didn't use to!) It's potentially a few hundred bytes on the stack, so
better on the heap.
> Or maybe a second buffer isn't even necessary, if
> DMA_MINALIGN is at least the correct alignment for
> iio_push_to_buffers_with_timestamp and I could pass the DMA-aligned
> buffer in.
Common trick is to just DMA into almost what you already have in bmi270_data.
But now the timestamp is in a fixed place you can use a structure to handle
the alignment for you.
/*
+ * Where IIO_DMA_MINALIGN is larger than 8 bytes, align to that
+ * to ensure a DMA safe buffer.
+ */
+ //__le16 buf[12] __aligned(IIO_DMA_MINALIGN);
struct {
__le16 channels[6];
aligned_s64 timestamp; //type is only in iio tree currently.
} data __aligned(IIO_DMA_MINALIGN);
The aligned_s64 is needed for x86_32 where s64 is only aligned to 4 bytes
whereas IIO assume natural alignment of everything (so 8 bytes).
};
Then pass that to the push_to_buffers call just as you currently do.
Jonathan
