On 7/28/2020 3:04 AM, Daniel Vetter wrote:
On Mon, Jul 27, 2020 at 2:27 PM Michel Dänzer michel@daenzer.net wrote:
On 2020-07-25 1:26 a.m., Paulo Zanoni wrote:
Em seg, 2020-07-20 às 17:01 +0530, Karthik B S escreveu:
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 1fa67700d8f4..95953b393941 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -697,14 +697,24 @@ u32 i915_get_vblank_counter(struct drm_crtc *crtc) return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xffffff; }
+static u32 g4x_get_flip_counter(struct drm_crtc *crtc) +{
- struct drm_i915_private *dev_priv = to_i915(crtc->dev);
- enum pipe pipe = to_intel_crtc(crtc)->pipe;
- return I915_READ(PIPE_FLIPCOUNT_G4X(pipe));
+}
u32 g4x_get_vblank_counter(struct drm_crtc *crtc) { struct drm_i915_private *dev_priv = to_i915(crtc->dev); enum pipe pipe = to_intel_crtc(crtc)->pipe;
if (crtc->state->async_flip)
return g4x_get_flip_counter(crtc);return I915_READ(PIPE_FRMCOUNT_G4X(pipe));I don't understand the intention behind this, can you please clarify? This goes back to my reply of the cover letter. It seems that here we're going to alternate between two different counters in our vblank count. So if user space alternates between sometimes using async flips and sometimes using normal flip it's going to get some very weird deltas, isn't it? At least this is what I remember from when I played with these registers: FLIPCOUNT drifts away from FRMCOUNT when we start using async flips.
This definitely looks wrong. The counter value returned by the get_vblank_counter hook is supposed to increment when a vertical blank period occurs; page flips are not supposed to affect this in any way.
Also you just flat out can't access crtc->state from interrupt context. Anything you need in there needs to be protected by the right irq-type spin_lock, updates correctly synchronized against both the interrupt handler and atomic updates, and data copied over, not pointers. Otherwise just crash&burn.
Thanks for the review. I will be removing this change in the next revision based on the feedback received, but I will keep this in mind whenever I'll have to access something from the interrupt context.
Thanks, Karthik.B.S
-Daniel