Re: [Intel-gfx] [PATCH v3 1/5] drm/i915: Add enable/disable flip done and flip done handler

[Karthik B S <karthik.b.s@intel.com>]

On 6/17/2020 3:28 PM, Daniel Vetter wrote:
> On Wed, Jun 10, 2020 at 03:33:06PM -0700, Paulo Zanoni wrote:
>> Em qui, 2020-05-28 às 11:09 +0530, Karthik B S escreveu:
>>> Add enable/disable flip done functions and the flip done handler
>>> function which handles the flip done interrupt.
>>>
>>> Enable the flip done interrupt in IER.
>>>
>>> Enable flip done function is called before writing the
>>> surface address register as the write to this register triggers
>>> the flip done interrupt
>>>
>>> Flip done handler is used to send the page flip event as soon as the
>>> surface address is written as per the requirement of async flips.
>>> The interrupt is disabled after the event is sent.
>>>
>>> v2: -Change function name from icl_* to skl_* (Paulo)
>>>      -Move flip handler to this patch (Paulo)
>>>      -Remove vblank_put() (Paulo)
>>>      -Enable flip done interrupt for gen9+ only (Paulo)
>>>      -Enable flip done interrupt in power_well_post_enable hook (Paulo)
>>>      -Removed the event check in flip done handler to handle async
>>>       flips without pageflip events.
>>>
>>> v3: -Move skl_disable_flip_done out of interrupt handler (Paulo)
>>>      -Make the pending vblank event NULL in the begining of
>>>       flip_done_handler to remove sporadic WARN_ON that is seen.
>>>
>>> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/display/intel_display.c | 10 ++++
>>>   drivers/gpu/drm/i915/i915_irq.c              | 52 ++++++++++++++++++++
>>>   drivers/gpu/drm/i915/i915_irq.h              |  2 +
>>>   3 files changed, 64 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>> index f40b909952cc..48cc1fc9bc5a 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>> @@ -15530,6 +15530,13 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>>>
>>>      intel_dbuf_pre_plane_update(state);
>>>
>>> +   for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>>> +           if (new_crtc_state->uapi.async_flip) {
>>> +                   skl_enable_flip_done(&crtc->base);
>>> +                   break;
>>> +           }
>>> +   }
>>> +
>>>      /* Now enable the clocks, plane, pipe, and connectors that we set up. */
>>>      dev_priv->display.commit_modeset_enables(state);
>>>
>>> @@ -15551,6 +15558,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>>>      drm_atomic_helper_wait_for_flip_done(dev, &state->base);
>>>
>>>      for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>>> +           if (new_crtc_state->uapi.async_flip)
>>> +                   skl_disable_flip_done(&crtc->base);
>>> +
>>>              if (new_crtc_state->hw.active &&
>>>                  !needs_modeset(new_crtc_state) &&
>>>                  !new_crtc_state->preload_luts &&
>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>>> index efdd4c7b8e92..632e7b1deb87 100644
>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>> @@ -1295,6 +1295,23 @@ display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
>>>                           u32 crc4) {}
>>>   #endif
>>>
>>> +static void flip_done_handler(struct drm_i915_private *dev_priv,
>>> +                         unsigned int pipe)
>>> +{
>>> +   struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
>>> +   struct drm_crtc_state *crtc_state = crtc->base.state;
>>> +   struct drm_pending_vblank_event *e = crtc_state->event;
>>> +   struct drm_device *dev = &dev_priv->drm;
>>> +   unsigned long irqflags;
>>> +
>>> +   crtc_state->event = NULL;
>>> +
>>> +   spin_lock_irqsave(&dev->event_lock, irqflags);
>>> +
>>> +   drm_crtc_send_vblank_event(&crtc->base, e);
>>
>> I don't think this is what we want. With this, the events the Kernel
>> sends us all have the same sequence and timestamp. In fact, the IGT
>> test you submitted fails because of this.
>>
>> In my original hackish proof-of-concept patch I had changed
>> drm_update_vblank_count() to force diff=1 in order to always send
>> events and I also changed g4x_get_vblank_counter() to get the counter
>> from FLIPCOUNT (which updates every time there's a flip) instead of
>> FRMCOUNT (which doesn't seem to increment when you do async flips).
>> That is a drastic change, but the patch was just a PoC so I didn't care
>> about keeping anything else working.
>>
>> One thing that confused me a little bit when dealing the the
>> vblank/flip event interface from drm.ko is that "flips" and "vblanks"
>> seem to be changed interchangeably, which is confusing for async flips:
>> if you keep forever doing async flips in the very first few scanlines
>> you never actually reach the "vblank" period, yet you keep flipping
>> your frame. Then, what should your expectation regarding events be?
> 
> Hm vblank should keep happening I thought (this isn't VRR or DRRS or PSR
> where that changes), no idea why we can't keep sending out vblank
> interrupts.

Thanks for the review.
I was facing a race which lead the complete system to freeze with 
vblanks being enabled during async flips. Thus had made this change.
But I'll find a proper implementation for this and enable the vblank 
interrupts as well.
> 
> Now flip events look maybe conflated in drm.ko code with vblank events
> since most of the time a flip complete happens at exactly the same time
> the vblank event. But for async flip this is not the case.
> 
> Probably worth it to have new helpers/function in drm_vblank.c for
> async flips, so that this is less confusing. Plus good documentation.
> 

Sure, I'll look into this and make the required additions.

>> I think we may need to check how the other drivers handle async vblanks
>> (or how drm.ko wants us to handle async vblanks). Should we increment
>> sequence on every async flip? What about the timestamp?
>>
>> Daniel, Ville, do you happen to know the proper semantics here?
>>
>> There's certainly some adjustment to do to both this patch and the IGT.
> 
> I think it would be really good if we cc dri-devel on this. amdgpu.ko is
> currently the only implementation of async flips, we need to make sure we
> are fully aligned on all the semantic details.
>

Sure, I'll look into the amddgpu.ko and make sure we are aligned.

> That also means that the igt needs to be reviewed and tested by amdgpu
> people. Might also be good to get the implementation acked by amd DC
> people, just to make triple-sure we have the same semantics and generic
> userspace compositors like mutter can use this across drivers. We've had
> way too much pain here in the past, especially with the details you point
> out here.
> 

Sure, I'll get the IGT reviewed by the amdgpu and amd DC people.

> Also, I think we need to have updated drm core documentation for async
> flips, since the current ones are "do it like amdgpu does it". I think
> just documenting the various pieces and flags in detail and how it all
> interacts with e.g. other atomic commits and everything else would be
> great.
>

Sure, I'll do this.

> Harry and Nicholaus are the people you want from amd. Added everyone to cc.

Thanks for this.

Thanks and Regards,
Karthik.B.S
> -Daniel
> 
> 
>>
>>> +
>>> +   spin_unlock_irqrestore(&dev->event_lock, irqflags);
>>> +}
>>>
>>>   static void hsw_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
>>>                                   enum pipe pipe)
>>> @@ -2388,6 +2405,9 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
>>>              if (iir & GEN8_PIPE_VBLANK)
>>>                      intel_handle_vblank(dev_priv, pipe);
>>>
>>> +           if (iir & GEN9_PIPE_PLANE1_FLIP_DONE)
>>> +                   flip_done_handler(dev_priv, pipe);
>>> +
>>>              if (iir & GEN8_PIPE_CDCLK_CRC_DONE)
>>>                      hsw_pipe_crc_irq_handler(dev_priv, pipe);
>>>
>>> @@ -2669,6 +2689,19 @@ int bdw_enable_vblank(struct drm_crtc *crtc)
>>>      return 0;
>>>   }
>>>
>>> +void skl_enable_flip_done(struct drm_crtc *crtc)
>>> +{
>>> +   struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>>> +   enum pipe pipe = to_intel_crtc(crtc)->pipe;
>>> +   unsigned long irqflags;
>>> +
>>> +   spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>>> +
>>> +   bdw_enable_pipe_irq(dev_priv, pipe, GEN9_PIPE_PLANE1_FLIP_DONE);
>>> +
>>> +   spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>>> +}
>>> +
>>>   /* Called from drm generic code, passed 'crtc' which
>>>    * we use as a pipe index
>>>    */
>>> @@ -2729,6 +2762,19 @@ void bdw_disable_vblank(struct drm_crtc *crtc)
>>>      spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>>>   }
>>>
>>> +void skl_disable_flip_done(struct drm_crtc *crtc)
>>> +{
>>> +   struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>>> +   enum pipe pipe = to_intel_crtc(crtc)->pipe;
>>> +   unsigned long irqflags;
>>> +
>>> +   spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>>> +
>>> +   bdw_disable_pipe_irq(dev_priv, pipe, GEN9_PIPE_PLANE1_FLIP_DONE);
>>> +
>>> +   spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>>> +}
>>> +
>>>   static void ibx_irq_reset(struct drm_i915_private *dev_priv)
>>>   {
>>>      struct intel_uncore *uncore = &dev_priv->uncore;
>>> @@ -2936,6 +2982,9 @@ void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
>>>      u32 extra_ier = GEN8_PIPE_VBLANK | GEN8_PIPE_FIFO_UNDERRUN;
>>>      enum pipe pipe;
>>>
>>> +   if (INTEL_GEN(dev_priv) >= 9)
>>> +           extra_ier |= GEN9_PIPE_PLANE1_FLIP_DONE;
>>> +
>>>      spin_lock_irq(&dev_priv->irq_lock);
>>>
>>>      if (!intel_irqs_enabled(dev_priv)) {
>>> @@ -3410,6 +3459,9 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>>>      de_pipe_enables = de_pipe_masked | GEN8_PIPE_VBLANK |
>>>                                         GEN8_PIPE_FIFO_UNDERRUN;
>>>
>>> +   if (INTEL_GEN(dev_priv) >= 9)
>>> +           de_pipe_enables |= GEN9_PIPE_PLANE1_FLIP_DONE;
>>> +
>>>      de_port_enables = de_port_masked;
>>>      if (IS_GEN9_LP(dev_priv))
>>>              de_port_enables |= BXT_DE_PORT_HOTPLUG_MASK;
>>> diff --git a/drivers/gpu/drm/i915/i915_irq.h b/drivers/gpu/drm/i915/i915_irq.h
>>> index 25f25cd95818..2f10c8135116 100644
>>> --- a/drivers/gpu/drm/i915/i915_irq.h
>>> +++ b/drivers/gpu/drm/i915/i915_irq.h
>>> @@ -112,11 +112,13 @@ int i915gm_enable_vblank(struct drm_crtc *crtc);
>>>   int i965_enable_vblank(struct drm_crtc *crtc);
>>>   int ilk_enable_vblank(struct drm_crtc *crtc);
>>>   int bdw_enable_vblank(struct drm_crtc *crtc);
>>> +void skl_enable_flip_done(struct drm_crtc *crtc);
>>>   void i8xx_disable_vblank(struct drm_crtc *crtc);
>>>   void i915gm_disable_vblank(struct drm_crtc *crtc);
>>>   void i965_disable_vblank(struct drm_crtc *crtc);
>>>   void ilk_disable_vblank(struct drm_crtc *crtc);
>>>   void bdw_disable_vblank(struct drm_crtc *crtc);
>>> +void skl_disable_flip_done(struct drm_crtc *crtc);
>>>
>>>   void gen2_irq_reset(struct intel_uncore *uncore);
>>>   void gen3_irq_reset(struct intel_uncore *uncore, i915_reg_t imr,
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx