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@xxxxxxxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
