Re: [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held.

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

Op 12-02-18 om 16:44 schreef Chris Wilson:
> Quoting Maarten Lankhorst (2018-02-12 15:39:16)
>> Op 12-02-18 om 16:19 schreef Chris Wilson:
>>> Quoting Maarten Lankhorst (2018-02-09 09:54:02)
>>>> This requires being able to read the vblank counter with the
>>>> uncore.lock already held. This is also a preparation for
>>>> being able to run the entire vblank update sequence with
>>>> the uncore lock held.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_irq.c     | 66 ++++++++++++++++++++++++++++++-------
>>>>  drivers/gpu/drm/i915/i915_trace.h   |  5 ++-
>>>>  drivers/gpu/drm/i915/intel_drv.h    |  1 +
>>>>  drivers/gpu/drm/i915/intel_sprite.c |  3 +-
>>>>  4 files changed, 60 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>>>> index eda9543a0199..6c491e63e07c 100644
>>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>>> @@ -736,13 +736,12 @@ static void i915_enable_asle_pipestat(struct drm_i915_private *dev_priv)
>>>>  /* Called from drm generic code, passed a 'crtc', which
>>>>   * we use as a pipe index
>>>>   */
>>>> -static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>>>> +static u32 __i915_get_vblank_counter(struct intel_crtc *crtc)
>>>>  {
>>>> -       struct drm_i915_private *dev_priv = to_i915(dev);
>>>> +       struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>>         i915_reg_t high_frame, low_frame;
>>>>         u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
>>>> -       const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
>>>> -       unsigned long irqflags;
>>>> +       const struct drm_display_mode *mode = &crtc->base.dev->vblank[crtc->pipe].hwmode;
>>> lockdep_assert_held(&dev_priv->uncore.lock);
>>>
>>>>  
>>>>         htotal = mode->crtc_htotal;
>>>>         hsync_start = mode->crtc_hsync_start;
>>>> @@ -756,10 +755,8 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>>>>         /* Start of vblank event occurs at start of hsync */
>>>>         vbl_start -= htotal - hsync_start;
>>>>  
>>>> -       high_frame = PIPEFRAME(pipe);
>>>> -       low_frame = PIPEFRAMEPIXEL(pipe);
>>>> -
>>>> -       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>>>> +       high_frame = PIPEFRAME(crtc->pipe);
>>>> +       low_frame = PIPEFRAMEPIXEL(crtc->pipe);
>>>>  
>>>>         /*
>>>>          * High & low register fields aren't synchronized, so make sure
>>>> @@ -772,8 +769,6 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>>>>                 high2 = I915_READ_FW(high_frame) & PIPE_FRAME_HIGH_MASK;
>>>>         } while (high1 != high2);
>>>>  
>>>> -       spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>>>> -
>>>>         high1 >>= PIPE_FRAME_HIGH_SHIFT;
>>>>         pixel = low & PIPE_PIXEL_MASK;
>>>>         low >>= PIPE_FRAME_LOW_SHIFT;
>>>> @@ -786,11 +781,60 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>>>>         return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xffffff;
>>>>  }
>>>>  
>>>> +static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>>>> +{
>>>> +       struct drm_i915_private *dev_priv = to_i915(dev);
>>>> +       unsigned long irqflags;
>>>> +       u32 ret;
>>>> +
>>>> +       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>>>> +       ret = i915_get_vblank_counter(dev, pipe);
>>>> +       spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>> +static u32 __g4x_get_vblank_counter(struct intel_crtc *crtc)
>>>> +{
>>>> +       struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>> lockdep_assert_held(&dev_priv->uncore.lock); ?
>>>
>>> Ok, why do we need uncore.lock held here at all? Serialisation of mmio
>>> access to the same cacheline is the age old reason, can we guarantee
>>> that doesn't happen anyway? (Probably not, but really most callers do
>>> not need the mmio w/a.)
>> Is the serialization only needed for writing?
> No, sadly not. Concurrent access of any type to the same cacheline is
> the trigger. (Supposed to be ivb-only.)
It's gonna be a pain to find all users, so I think keeping the uncore lock is good enough for now, or we need to split off the display engine lock..

>  
>> The only thing that can race with nonblocking atomic commits are legacy
>> cursor updates, but those can only happen if the cursor plane are not part
>> of the previous atomic commit. Those are also protected by plane->mutex,
>> so in theory same cache lines on the same pipes probably can't race..
> At worst, we could just use a vblank->spinlock?
Perhaps, but the amount of registers isn't exactly small, so I feel better if we use the same lock consistently..
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx