Re: [PATCH 2/3] iio: imu: Add triggered buffer for Bosch BMI270 IMU

[Jonathan Cameron]

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