[PATCH] Revert "drm/i915: Use atomic commits for legacy page_flips"

[PATCH] Revert "drm/i915: Use atomic commits for legacy page_flips"

[Chris Wilson]

This reverts commit ee042aa40b66d18d465206845b0752c6a617ba3f.

Something appears to be off in the timing, but as far as I can tell it
is not along the event delivery path. The net effect appears to be
rendering flicker (the current render buffer appears on the scanout,
with what appears to be active rendering for a fraction of a frame) and
is causing me a headache.

The cursor is also being stalled by page flips, causing a "heavy mouse"
and jitter.

Reported-and-tested-by: Steven Newbury <steve@snewbury.org.uk>
Reported-by:  Rafael Ristovski  <rafael.ristovski@gmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96593
Testcase: igt/kms_cursor_legacy/basic-flip-vs-cursor
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1141b86..d4a7872 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11646,7 +11646,6 @@ void intel_check_page_flip(struct drm_i915_private *dev_priv, int pipe)
 	spin_unlock(&dev->event_lock);
 }
 
-__maybe_unused
 static int intel_crtc_page_flip(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
 				struct drm_pending_vblank_event *event,
@@ -14008,7 +14007,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.set_property = drm_atomic_helper_crtc_set_property,
 	.destroy = intel_crtc_destroy,
-	.page_flip = drm_atomic_helper_page_flip,
+	.page_flip = intel_crtc_page_flip,
 	.atomic_duplicate_state = intel_crtc_duplicate_state,
 	.atomic_destroy_state = intel_crtc_destroy_state,
 };
-- 
2.8.1

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

✓ Ro.CI.BAT: success for Revert "drm/i915: Use atomic commits for legacy page_flips"

[Patchwork]

== Series Details ==

Series: Revert "drm/i915: Use atomic commits for legacy page_flips"
URL   : https://patchwork.freedesktop.org/series/9137/
State : success

== Summary ==

Series 9137v1 Revert "drm/i915: Use atomic commits for legacy page_flips"
http://patchwork.freedesktop.org/api/1.0/series/9137/revisions/1/mbox

Test kms_cursor_legacy:
        Subgroup basic-cursor-vs-flip:
                fail       -> PASS       (fi-skl-i5-6260u)
                fail       -> PASS       (fi-snb-i7-2600)

fi-hsw-i7-4770k  total:228  pass:195  dwarn:0   dfail:0   fail:0   skip:33 
fi-kbl-qkkr      total:228  pass:161  dwarn:28  dfail:0   fail:0   skip:39 
fi-skl-i5-6260u  total:228  pass:203  dwarn:0   dfail:0   fail:0   skip:25 
fi-snb-i7-2600   total:228  pass:175  dwarn:0   dfail:0   fail:0   skip:53 
fi-skl-i7-6700k failed to connect after reboot
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1297/

8660f44 drm-intel-nightly: 2016y-06m-24d-11h-09m-19s UTC integration manifest
f24c9bb Revert "drm/i915: Use atomic commits for legacy page_flips"

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

Re: [PATCH] Revert "drm/i915: Use atomic commits for legacy page_flips"

[Maarten Lankhorst]

Op 24-06-16 om 14:44 schreef Chris Wilson:
> This reverts commit ee042aa40b66d18d465206845b0752c6a617ba3f.
>
> Something appears to be off in the timing, but as far as I can tell it
> is not along the event delivery path. The net effect appears to be
> rendering flicker (the current render buffer appears on the scanout,
> with what appears to be active rendering for a fraction of a frame) and
> is causing me a headache.
>
> The cursor is also being stalled by page flips, causing a "heavy mouse"
> and jitter.
>
> Reported-and-tested-by: Steven Newbury <steve@snewbury.org.uk>
> Reported-by:  Rafael Ristovski  <rafael.ristovski@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96593
> Testcase: igt/kms_cursor_legacy/basic-flip-vs-cursor
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1141b86..d4a7872 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11646,7 +11646,6 @@ void intel_check_page_flip(struct drm_i915_private *dev_priv, int pipe)
>  	spin_unlock(&dev->event_lock);
>  }
>  
> -__maybe_unused
>  static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
>  				struct drm_pending_vblank_event *event,
> @@ -14008,7 +14007,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
>  	.set_config = drm_atomic_helper_set_config,
>  	.set_property = drm_atomic_helper_crtc_set_property,
>  	.destroy = intel_crtc_destroy,
> -	.page_flip = drm_atomic_helper_page_flip,
> +	.page_flip = intel_crtc_page_flip,
>  	.atomic_duplicate_state = intel_crtc_duplicate_state,
>  	.atomic_destroy_state = intel_crtc_destroy_state,
>  };

This still wouldn't fix updates based on nonblocking atomic commits, though probably less likely.

Does this bug get fixed by "drm/i915: Don't stall too much for cursor vs. pageflip" ?

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

Re: [PATCH] Revert "drm/i915: Use atomic commits for legacy page_flips"

[Chris Wilson]

On Tue, Jun 28, 2016 at 09:56:47AM +0200, Maarten Lankhorst wrote:
> Op 24-06-16 om 14:44 schreef Chris Wilson:
> > This reverts commit ee042aa40b66d18d465206845b0752c6a617ba3f.
> >
> > Something appears to be off in the timing, but as far as I can tell it
> > is not along the event delivery path. The net effect appears to be
> > rendering flicker (the current render buffer appears on the scanout,
> > with what appears to be active rendering for a fraction of a frame) and
> > is causing me a headache.
> >
> > The cursor is also being stalled by page flips, causing a "heavy mouse"
> > and jitter.
> >
> > Reported-and-tested-by: Steven Newbury <steve@snewbury.org.uk>
> > Reported-by:  Rafael Ristovski  <rafael.ristovski@gmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96593
> > Testcase: igt/kms_cursor_legacy/basic-flip-vs-cursor
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 1141b86..d4a7872 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11646,7 +11646,6 @@ void intel_check_page_flip(struct drm_i915_private *dev_priv, int pipe)
> >  	spin_unlock(&dev->event_lock);
> >  }
> >  
> > -__maybe_unused
> >  static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >  				struct drm_framebuffer *fb,
> >  				struct drm_pending_vblank_event *event,
> > @@ -14008,7 +14007,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
> >  	.set_config = drm_atomic_helper_set_config,
> >  	.set_property = drm_atomic_helper_crtc_set_property,
> >  	.destroy = intel_crtc_destroy,
> > -	.page_flip = drm_atomic_helper_page_flip,
> > +	.page_flip = intel_crtc_page_flip,
> >  	.atomic_duplicate_state = intel_crtc_duplicate_state,
> >  	.atomic_destroy_state = intel_crtc_destroy_state,
> >  };
> 
> This still wouldn't fix updates based on nonblocking atomic commits, though probably less likely.
> 
> Does this bug get fixed by "drm/i915: Don't stall too much for cursor vs. pageflip" ?

Only one side, cursor still stalls flips (or rather a flip can miss 10s of
vblanks under load), and the wrong buffers are still visible.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Revert "drm/i915: Use atomic commits for legacy page_flips"

[Daniel Stone]

Hi Chris,

On 24 June 2016 at 22:44, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> This reverts commit ee042aa40b66d18d465206845b0752c6a617ba3f.
>
> Something appears to be off in the timing, but as far as I can tell it
> is not along the event delivery path. The net effect appears to be
> rendering flicker (the current render buffer appears on the scanout,
> with what appears to be active rendering for a fraction of a frame) and
> is causing me a headache.

Does this change at all with the fixup in
https://lists.freedesktop.org/archives/intel-gfx/2016-June/099466.html
?

I was seeing no waits whatsoever for rendering, causing active
rendering to be visible towards positive x and negative y (top-right
corner) with this.

> The cursor is also being stalled by page flips, causing a "heavy mouse"
> and jitter.

This I can't attest to though.

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

Re: [PATCH] Revert "drm/i915: Use atomic commits for legacy page_flips"

[Maarten Lankhorst]

Op 28-06-16 om 10:35 schreef Chris Wilson:
> On Tue, Jun 28, 2016 at 09:56:47AM +0200, Maarten Lankhorst wrote:
>> Op 24-06-16 om 14:44 schreef Chris Wilson:
>>> This reverts commit ee042aa40b66d18d465206845b0752c6a617ba3f.
>>>
>>> Something appears to be off in the timing, but as far as I can tell it
>>> is not along the event delivery path. The net effect appears to be
>>> rendering flicker (the current render buffer appears on the scanout,
>>> with what appears to be active rendering for a fraction of a frame) and
>>> is causing me a headache.
>>>
>>> The cursor is also being stalled by page flips, causing a "heavy mouse"
>>> and jitter.
>>>
>>> Reported-and-tested-by: Steven Newbury <steve@snewbury.org.uk>
>>> Reported-by:  Rafael Ristovski  <rafael.ristovski@gmail.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96593
>>> Testcase: igt/kms_cursor_legacy/basic-flip-vs-cursor
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 1141b86..d4a7872 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -11646,7 +11646,6 @@ void intel_check_page_flip(struct drm_i915_private *dev_priv, int pipe)
>>>  	spin_unlock(&dev->event_lock);
>>>  }
>>>  
>>> -__maybe_unused
>>>  static int intel_crtc_page_flip(struct drm_crtc *crtc,
>>>  				struct drm_framebuffer *fb,
>>>  				struct drm_pending_vblank_event *event,
>>> @@ -14008,7 +14007,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
>>>  	.set_config = drm_atomic_helper_set_config,
>>>  	.set_property = drm_atomic_helper_crtc_set_property,
>>>  	.destroy = intel_crtc_destroy,
>>> -	.page_flip = drm_atomic_helper_page_flip,
>>> +	.page_flip = intel_crtc_page_flip,
>>>  	.atomic_duplicate_state = intel_crtc_duplicate_state,
>>>  	.atomic_destroy_state = intel_crtc_destroy_state,
>>>  };
>> This still wouldn't fix updates based on nonblocking atomic commits, though probably less likely.
>>
>> Does this bug get fixed by "drm/i915: Don't stall too much for cursor vs. pageflip" ?
> Only one side, cursor still stalls flips (or rather a flip can miss 10s of
> vblanks under load), and the wrong buffers are still visible.
> -Chris
Since this only affects legacy page flips the fix is fine for now till we fix the root cause.

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

Re: [PATCH] Revert "drm/i915: Use atomic commits for legacy page_flips"

[Chris Wilson]

On Tue, Jun 28, 2016 at 04:44:42PM +0200, Maarten Lankhorst wrote:
> Op 28-06-16 om 10:35 schreef Chris Wilson:
> > On Tue, Jun 28, 2016 at 09:56:47AM +0200, Maarten Lankhorst wrote:
> >> Op 24-06-16 om 14:44 schreef Chris Wilson:
> >>> This reverts commit ee042aa40b66d18d465206845b0752c6a617ba3f.
> >>>
> >>> Something appears to be off in the timing, but as far as I can tell it
> >>> is not along the event delivery path. The net effect appears to be
> >>> rendering flicker (the current render buffer appears on the scanout,
> >>> with what appears to be active rendering for a fraction of a frame) and
> >>> is causing me a headache.
> >>>
> >>> The cursor is also being stalled by page flips, causing a "heavy mouse"
> >>> and jitter.
> >>>
> >>> Reported-and-tested-by: Steven Newbury <steve@snewbury.org.uk>
> >>> Reported-by:  Rafael Ristovski  <rafael.ristovski@gmail.com>
> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96593
> >>> Testcase: igt/kms_cursor_legacy/basic-flip-vs-cursor
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_display.c | 3 +--
> >>>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>> index 1141b86..d4a7872 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -11646,7 +11646,6 @@ void intel_check_page_flip(struct drm_i915_private *dev_priv, int pipe)
> >>>  	spin_unlock(&dev->event_lock);
> >>>  }
> >>>  
> >>> -__maybe_unused
> >>>  static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >>>  				struct drm_framebuffer *fb,
> >>>  				struct drm_pending_vblank_event *event,
> >>> @@ -14008,7 +14007,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
> >>>  	.set_config = drm_atomic_helper_set_config,
> >>>  	.set_property = drm_atomic_helper_crtc_set_property,
> >>>  	.destroy = intel_crtc_destroy,
> >>> -	.page_flip = drm_atomic_helper_page_flip,
> >>> +	.page_flip = intel_crtc_page_flip,
> >>>  	.atomic_duplicate_state = intel_crtc_duplicate_state,
> >>>  	.atomic_destroy_state = intel_crtc_destroy_state,
> >>>  };
> >> This still wouldn't fix updates based on nonblocking atomic commits, though probably less likely.
> >>
> >> Does this bug get fixed by "drm/i915: Don't stall too much for cursor vs. pageflip" ?
> > Only one side, cursor still stalls flips (or rather a flip can miss 10s of
> > vblanks under load), and the wrong buffers are still visible.
> > -Chris
> Since this only affects legacy page flips the fix is fine for now till we fix the root cause.
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I've pushed this now. Daniel has undoubtably found the cause of the
tearing bug, but he is also right that with one old/new state mixup
there is likely more. Then there is the challenge of getting
nonblocking cursors and unstalling flips.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx