Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 12-02-18 om 21:55 schreef Chris Wilson:
> Quoting Ville Syrjälä (2018-02-12 18:06:58)
>> On Mon, Feb 12, 2018 at 05:24:54PM +0000, Chris Wilson wrote:
>>> Quoting Ville Syrjälä (2018-02-12 16:55:28)
>>>> On Mon, Feb 12, 2018 at 04:41:05PM +0100, Maarten Lankhorst wrote:
>>>>> Op 12-02-18 om 16:31 schreef Chris Wilson:
>>>>>> Quoting Maarten Lankhorst (2018-02-12 15:27:34)
>>>>>>> Op 12-02-18 om 16:22 schreef Chris Wilson:
>>>>>>>> Quoting Maarten Lankhorst (2018-02-12 15:16:39)
>>>>>>>>> Op 12-02-18 om 16:10 schreef Chris Wilson:
>>>>>>>>>> Quoting Maarten Lankhorst (2018-02-09 09:54:00)
>>>>>>>>>>> This is a nice preparation for grabbing the uncore lock during evasion.
>>>>>>>>>>> Grabbing the spinlock with the lock held messes up the locking,
>>>>>>>>>>> so it's easier to handover the reference to the eve
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++-------
>>>>>>>>>>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>>>>> index 3be22c0fcfb5..971a1ea0db45 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>>>>> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>>>>>>>>>>>  
>>>>>>>>>>>         local_irq_disable();
>>>>>>>>>>>  
>>>>>>>>>>> -       if (min <= 0 || max <= 0)
>>>>>>>>>>> +       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>>>>>>>>>>>                 return;
>>>>>>>>>>>  
>>>>>>>>>>> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>>>>>>>>>>> +       if (min <= 0 || max <= 0)
>>>>>>>>>>>                 return;
>>>>>>>>>>>  
>>>>>>>>>> The corresponding vblank_put is the one later in update_start(), right?
>>>>>>>>>> I don't think you intended to keep this chunk.
>>>>>>>>>> -Chris
>>>>>>>>> I'm not sure what you mean? The vblank_put is now in pipe_update_end, except if the
>>>>>>>>> event takes over the reference. I think the code is correct. :)
>>>>>>>> Then it's unbalanced in the case of error still.
>>>>>>>> -Chris
>>>>>>> It already would have been for events, hence the WARN_ON there.
>>>>>>> I don't think we can do anything about it, this shouldn't ever
>>>>>>> happen in practice, could be a BUG_ON for all I care. :)
>>>>>> I would much prefer that over intentionally bad code.
>>>>>>
>>>>>> But do we really need to enable the vblank irq here? If the event
>>>>>> requires it, doesn't it already enable the vblank. Here, we only need it
>>>>>> when sleeping, can we not determine we have enough time before the
>>>>>> vblank without enabling the interrupt?
>>>>> I'm not sure why we get a reference to the vblank counter here. Perhaps Ville does?
>>>> We need the vblank irq to be enabled before we check the scanline since
>>>> otherwise we may end up doing:
>>>>
>>>> 1. check scanline
>>>> 3. vblank irq fires
>>>> 2. enable vblank irq
>>>> 3. wait for the next vblank
>>>>
>>>> So we'd end up wasting an entire frame.
>>> Step: 2.5, check_scanline?
>>>
>>> Something like,
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>>> index 574bd02c5a2e..70c2ee1c7b8c 100644
>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>> @@ -97,6 +97,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>>>         bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>>>                 intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI);
>>>         DEFINE_WAIT(wait);
>>> +       bool have_vblank_irq = false;
>>>  
>>>         vblank_start = adjusted_mode->crtc_vblank_start;
>>>         if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
>>> @@ -112,9 +113,6 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>>>         if (min <= 0 || max <= 0)
>>>                 return;
>>>  
>>> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>>> -               return;
>>> -
>>>         crtc->debug.min_vbl = min;
>>>         crtc->debug.max_vbl = max;
>>>         trace_i915_pipe_update_start(crtc);
>>> @@ -127,6 +125,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>>>                  */
>>>                 prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>>>  
>>> +               if (!have_vblank_irq)
>>> +                       have_vblank_irq = !drm_crtc_vblank_get(&crtc->base);
>>> +
>> This doesn't seem to change anything.
> Nothing wrt to the existing code :)
>
> The idea is that we have to enable the vblank-irq before doing the
> scanline check in order to not miss the interrupt, which as I understand
> it was the danger you highlighted with trying to avoid taking the
> vblank-irq.
> -Chris

I've taken a look at the code, and most of the time we set crtc_state->event.
Either through calls like pageflip, or if not set we always allocate one in
drm_atomic_helper_setup_commit() except for legacy cursor updates.

Because of this I think the original patch is fine, and I kind of like
having everything prepared in pipe_update_start, while pipe_update_end
only has to release it.

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx