On Wed, 25 Sep 2013 06:32:10 +0200 Mario Kleiner mario.kleiner@tuebingen.mpg.de wrote:
But given the new situation, your proposal is great! If we push the clock readouts into the get_scanoutpos routine, we can make this robust without causing grief for the rt people and without the need for a new separate lock for display regs in intel-kms.
E.g., for intel-kms:
i915_get_crtc_scanoutpos(..., ktime_t *stime, ktime_t *etime) { ... spin_lock_irqsave(...uncore.lock); preempt_disable(); *stime = ktime_get(); position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)); *etime = ktime_get(); preempt_enable(); spin_unlock_irqrestore(...uncore.lock) ... }
With your patchset to reduce the amount of register reads needed in that function, and given that forcewake handling isn't needed for these registers, this should make it robust again and wouldn't need new locks.
Unless ktime_get is also a bad thing to do in a preempt disabled section?
ktime_get() works fine in preempt_disable sections, although it may add some latencies, but you shouldn't need to worry about it.
I like this solution the best too, but if it does go in, I would ask to send us the patch for adding the preempt_disable() and we can add the preempt_disable_rt() to it. Why make mainline have a little more overhead?
-- Steve