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

[Karthik B S]

On 7/28/2020 3:04 AM, Daniel Vetter wrote:
> On Mon, Jul 27, 2020 at 2:27 PM Michel Dänzer <michel at 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
>