On 28/05/16 09:32, Gucea Doru wrote: > On Fri, May 27, 2016 at 6:15 PM, Crestez Dan Leonard > <leonard.crestez@xxxxxxxxx> wrote: >> On 05/26/2016 03:23 PM, Doru Gucea wrote: >>> In order to support software triggers (e.g.: hr-based trigger) the >>> operations from trigger enable/disable are moved to buffer enable/ >>> disable. >>> >>> This also allows us to remove the mutex from inv_mpu6050_read_fifo: >>> inv_reset_fifo can't access in parallel shared fields because the >>> buffers must be enabled prior to reading data. >>> >>> A benefit of removing the mutex is that we avoid a deadlock at >>> buffer wqdisable time. The scenario was the following: >>> (1) at buffer disable time the indio_dev->mlock is acquired >>> (2) free_irq is called and waits for IRQs completion >>> (3) inv_mpu6050_read_fifo tries to acquire the indio_dev->mlock >> >> I'm not sure this is safe, it seems to me that inv_reset_fifo assumes >> mlock is taken and this needs to be assured. I think properly removing >> the irq disabling deadlock require either using something other than >> mlock or not calling inv_reset_fifo from the interrupt at all. The >> latter seems preferable to me. But this issue should be handled >> separately from software trigger support. >> > > inv_reset_fifo still needs to be called: it might happen that we miss a > DATA_RDY interrupt in which case we'll have a mismatch between > time-stamps and bytes from the FIFO queues. The only way to recover > from such a situation is to flush the FIFO's and restart the interrupt. > > Regarding the locking, before my patch, inv_reset_fifo could flush the > FIFO's (see inv_mpu_data_rdy_trigger_set_state) while > inv_mpu6050_read_fifo was reading data and the role of the mlock was > to avoid this situation. This situation is not possible anymore because > the operations from trigger enable/disable were moved to buffer > enable/disable: trigger state can't be changed while buffers are enabled. > Do you see another scenario? > > I'll split the software trigger support and deadlock solution in two patches. > >> Another issue is that the fifo is filled at the sampling frequency >> configured in device registers and this can be different from the >> sampling frequency configured on the software trigger. It seems to me >> that the user needs to manually ensure that they are identical. >> > > A solution would be to hook the iio_hrtimer_info_store_sampling_frequency > method of the hrtimer for setting the sampling frequency together with > the timer frequency. I think this will require changes to the hrtimer for > exposing the above function to drivers. Daniel, what would be the right > approach? Back to basic questions from me ;) Why do you want to use a software trigger here at all? Trying to align the two sampling frequencies is really a non starter - it'll never work however hard you try. So I am guessing you want to support sampling when the interrupts are not wired up? If so the only way to handle that is to use a thread to sample the interrupt status register fast enough to get a reasonable idea of when it changed... basically fake the interrupt wire. If you want to be able to run this device on say a much slower clock then we need to look at how to bypass the fifo entirely (read from the relevant sensor registers instead I think...) > >> Even if they are identical the timer ticks might not match perfectly. >> When reading from the fifo all available samples are handled. >> >> If device frequency is set too high you will push multiple samples to >> userspace, all but one of them with bad timestamps. It's also possible >> to find zero samples if device frequency is set higher than timer frequency. >> > > In case of timer ticks mismatch there will also be a mismatch between > time-stamps and bytes from the FIFO queues. I wonder if a clean recovery > from such a situation would be to call inv_reset_fifo together with resetting > the hrtimer. This is going to happen frequently - how frequently will depend on the drift between the two clocks. Not a lot of point of having a high frequency sampling system that resets every few seconds dropping some samples on the floor. Jonathan > >> -- >> Regards, >> Leonard >> -- >> 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 > > Regards, > Doru > -- 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
