All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled
@ 2015-03-18 19:15 Paulo Zanoni
  2015-03-18 22:01 ` Matt Roper
  0 siblings, 1 reply; 8+ messages in thread
From: Paulo Zanoni @ 2015-03-18 19:15 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Otherwise we'll get a WARN from drm_wait_one_vblank() saying that
vblanks are not available (since they were already disabled in
crtc_disable()).

This is certainly a regresison, but QA couldn't bisect it due to
other regressions breaking the bisect.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89550
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


I'm not really sure if this is the best way to fix the regression. Ville and/or
Matt should provide some comments here.


diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f1c0295..f2f7e81 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12193,7 +12193,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 
 	intel_runtime_pm_put(dev_priv);
 
-	if (intel_crtc->atomic.wait_vblank)
+	if (intel_crtc->active && intel_crtc->atomic.wait_vblank)
 		intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
-- 
2.1.4

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled
  2015-03-18 19:15 [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled Paulo Zanoni
@ 2015-03-18 22:01 ` Matt Roper
  2015-03-18 22:04   ` [PATCH] drm/i915: Move vblank wait determination to 'check' phase Matt Roper
  2015-03-19 16:31   ` [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled Daniel Vetter
  0 siblings, 2 replies; 8+ messages in thread
From: Matt Roper @ 2015-03-18 22:01 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni

On Wed, Mar 18, 2015 at 04:15:07PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Otherwise we'll get a WARN from drm_wait_one_vblank() saying that
> vblanks are not available (since they were already disabled in
> crtc_disable()).
> 
> This is certainly a regresison, but QA couldn't bisect it due to
> other regressions breaking the bisect.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89550
> Testcase: igt/pm_rpm/legacy-planes
> Testcase: igt/pm_rpm/legacy-planes-dpms
> Testcase: igt/pm_rpm/universal-planes
> Testcase: igt/pm_rpm/universal-planes-dpms
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> I'm not really sure if this is the best way to fix the regression. Ville and/or
> Matt should provide some comments here.
> 

This will definitely fix the problem (we shouldn't be waiting for vblank
with a disabled CRTC!), but I think the true bug in our code is that our
sprite commit function is setting some bits that should have been set
back in the 'check' phase under the 'if (intel_crtc->active)' branch.

I'll supply a patch shortly that I think should fix how we got into this
situation in the first place.  The whole 'wait_for_vblank' flag is
something we should be able to get rid of completely once I finish the
atomic watermark work.


Matt

> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f1c0295..f2f7e81 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12193,7 +12193,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>  
>  	intel_runtime_pm_put(dev_priv);
>  
> -	if (intel_crtc->atomic.wait_vblank)
> +	if (intel_crtc->active && intel_crtc->atomic.wait_vblank)
>  		intel_wait_for_vblank(dev, intel_crtc->pipe);
>  
>  	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
> -- 
> 2.1.4
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] drm/i915: Move vblank wait determination to 'check' phase
  2015-03-18 22:01 ` Matt Roper
@ 2015-03-18 22:04   ` Matt Roper
  2015-03-19 11:43     ` shuang.he
  2015-03-19 19:16     ` Paulo Zanoni
  2015-03-19 16:31   ` [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled Daniel Vetter
  1 sibling, 2 replies; 8+ messages in thread
From: Matt Roper @ 2015-03-18 22:04 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

Determining whether we'll need to wait for vblanks is something we
should determine during the atomic 'check' phase, not the 'commit'
phase.  Note that we only set these bits in the branch of 'check' where
intel_crtc->active is true so that we don't try to wait on a disabled
CRTC.

The whole 'wait for vblank after update' flag should go away in the
future, once we start handling watermarks in a proper atomic manner.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Root-cause-analysis-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89550
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
Paulo, can you test this patch and see if it solves the problem?  The only
platform I have access to (IVB) doesn't have runtime power management, so I
can't actually run the relevant testcases myself.

 drivers/gpu/drm/i915/intel_sprite.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index a828736..fad1d9f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -747,13 +747,6 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(SPRSURF(pipe), 0);
 
 	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
-
-	/*
-	 * Avoid underruns when disabling the sprite.
-	 * FIXME remove once watermark updates are done properly.
-	 */
-	intel_crtc->atomic.wait_vblank = true;
-	intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
 }
 
 static int
@@ -937,13 +930,6 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(DVSSURF(pipe), 0);
 
 	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
-
-	/*
-	 * Avoid underruns when disabling the sprite.
-	 * FIXME remove once watermark updates are done properly.
-	 */
-	intel_crtc->atomic.wait_vblank = true;
-	intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
 }
 
 /**
@@ -1262,6 +1248,16 @@ finish:
 		    plane->state->fb->modifier[0] !=
 		    state->base.fb->modifier[0])
 			intel_crtc->atomic.update_wm = true;
+
+		if (!state->visible) {
+			/*
+			 * Avoid underruns when disabling the sprite.
+			 * FIXME remove once watermark updates are done properly.
+			 */
+			intel_crtc->atomic.wait_vblank = true;
+			intel_crtc->atomic.update_sprite_watermarks |=
+				(1 << drm_plane_index(plane));
+		}
 	}
 
 	return 0;
-- 
1.8.5.1

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/i915: Move vblank wait determination to 'check' phase
  2015-03-18 22:04   ` [PATCH] drm/i915: Move vblank wait determination to 'check' phase Matt Roper
@ 2015-03-19 11:43     ` shuang.he
  2015-03-19 19:16     ` Paulo Zanoni
  1 sibling, 0 replies; 8+ messages in thread
From: shuang.he @ 2015-03-19 11:43 UTC (permalink / raw)
  To: shuang.he, ethan.gao, intel-gfx, matthew.d.roper

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5999
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  272/272              272/272
ILK                                  301/301              301/301
SNB                                  303/303              303/303
IVB                                  342/342              342/342
BYT                                  287/287              287/287
HSW                                  362/362              362/362
BDW                                  308/308              308/308
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled
  2015-03-18 22:01 ` Matt Roper
  2015-03-18 22:04   ` [PATCH] drm/i915: Move vblank wait determination to 'check' phase Matt Roper
@ 2015-03-19 16:31   ` Daniel Vetter
  1 sibling, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2015-03-19 16:31 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx, Paulo Zanoni

On Wed, Mar 18, 2015 at 03:01:22PM -0700, Matt Roper wrote:
> On Wed, Mar 18, 2015 at 04:15:07PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Otherwise we'll get a WARN from drm_wait_one_vblank() saying that
> > vblanks are not available (since they were already disabled in
> > crtc_disable()).
> > 
> > This is certainly a regresison, but QA couldn't bisect it due to
> > other regressions breaking the bisect.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89550
> > Testcase: igt/pm_rpm/legacy-planes
> > Testcase: igt/pm_rpm/legacy-planes-dpms
> > Testcase: igt/pm_rpm/universal-planes
> > Testcase: igt/pm_rpm/universal-planes-dpms
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 
> > I'm not really sure if this is the best way to fix the regression. Ville and/or
> > Matt should provide some comments here.
> > 
> 
> This will definitely fix the problem (we shouldn't be waiting for vblank
> with a disabled CRTC!), but I think the true bug in our code is that our
> sprite commit function is setting some bits that should have been set
> back in the 'check' phase under the 'if (intel_crtc->active)' branch.
> 
> I'll supply a patch shortly that I think should fix how we got into this
> situation in the first place.  The whole 'wait_for_vblank' flag is
> something we should be able to get rid of completely once I finish the
> atomic watermark work.

Yeah because of runtime PM we shouldn't do anything really for plane
updates when the crtc is off. Unnecessary vblank waits here really smell
like the canary, not the cause.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/i915: Move vblank wait determination to 'check' phase
  2015-03-18 22:04   ` [PATCH] drm/i915: Move vblank wait determination to 'check' phase Matt Roper
  2015-03-19 11:43     ` shuang.he
@ 2015-03-19 19:16     ` Paulo Zanoni
  2015-03-19 19:36       ` Ville Syrjälä
  1 sibling, 1 reply; 8+ messages in thread
From: Paulo Zanoni @ 2015-03-19 19:16 UTC (permalink / raw)
  To: Matt Roper; +Cc: Intel Graphics Development, Paulo Zanoni

2015-03-18 19:04 GMT-03:00 Matt Roper <matthew.d.roper@intel.com>:
> Determining whether we'll need to wait for vblanks is something we
> should determine during the atomic 'check' phase, not the 'commit'
> phase.  Note that we only set these bits in the branch of 'check' where
> intel_crtc->active is true so that we don't try to wait on a disabled
> CRTC.
>
> The whole 'wait for vblank after update' flag should go away in the
> future, once we start handling watermarks in a proper atomic manner.

Add this to the commit message:

Regression introduced by:

commit 2fdd7def16dd7580f297827930126c16b152ec11
Author: Matt Roper <matthew.d.roper@intel.com>
Date:   Wed Mar 4 10:49:04 2015 -0800
    drm/i915: Don't clobber plane state on internal disables

(at least according to QA's bisect)

>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Root-cause-analysis-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89550
> Testcase: igt/pm_rpm/legacy-planes
> Testcase: igt/pm_rpm/legacy-planes-dpms
> Testcase: igt/pm_rpm/universal-planes
> Testcase: igt/pm_rpm/universal-planes-dpms
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> Paulo, can you test this patch and see if it solves the problem?  The only
> platform I have access to (IVB) doesn't have runtime power management, so I
> can't actually run the relevant testcases myself.

Yes, this patch makes the WARN go away on my BDW machine :)
Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

One of the possible problems with this patch is that, before it, the
wait_vblank was only set for ILK-BDW, but now we set it for all
platforms. Do we really want this? Otherwise, it looks good, although
I haven't been closely following the atomic rework to be able to
assert this is really according to the grand plans. Daniel/Ville could
comment here :)

>
>  drivers/gpu/drm/i915/intel_sprite.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index a828736..fad1d9f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -747,13 +747,6 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>         I915_WRITE(SPRSURF(pipe), 0);
>
>         intel_flush_primary_plane(dev_priv, intel_crtc->plane);
> -
> -       /*
> -        * Avoid underruns when disabling the sprite.
> -        * FIXME remove once watermark updates are done properly.
> -        */
> -       intel_crtc->atomic.wait_vblank = true;
> -       intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
>  }
>
>  static int
> @@ -937,13 +930,6 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>         I915_WRITE(DVSSURF(pipe), 0);
>
>         intel_flush_primary_plane(dev_priv, intel_crtc->plane);
> -
> -       /*
> -        * Avoid underruns when disabling the sprite.
> -        * FIXME remove once watermark updates are done properly.
> -        */
> -       intel_crtc->atomic.wait_vblank = true;
> -       intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
>  }
>
>  /**
> @@ -1262,6 +1248,16 @@ finish:
>                     plane->state->fb->modifier[0] !=
>                     state->base.fb->modifier[0])
>                         intel_crtc->atomic.update_wm = true;
> +
> +               if (!state->visible) {
> +                       /*
> +                        * Avoid underruns when disabling the sprite.
> +                        * FIXME remove once watermark updates are done properly.
> +                        */
> +                       intel_crtc->atomic.wait_vblank = true;
> +                       intel_crtc->atomic.update_sprite_watermarks |=
> +                               (1 << drm_plane_index(plane));
> +               }
>         }
>
>         return 0;
> --
> 1.8.5.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/i915: Move vblank wait determination to 'check' phase
  2015-03-19 19:16     ` Paulo Zanoni
@ 2015-03-19 19:36       ` Ville Syrjälä
  2015-03-20 10:22         ` Daniel Vetter
  0 siblings, 1 reply; 8+ messages in thread
From: Ville Syrjälä @ 2015-03-19 19:36 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: Intel Graphics Development, Paulo Zanoni

On Thu, Mar 19, 2015 at 04:16:41PM -0300, Paulo Zanoni wrote:
> 2015-03-18 19:04 GMT-03:00 Matt Roper <matthew.d.roper@intel.com>:
> > Determining whether we'll need to wait for vblanks is something we
> > should determine during the atomic 'check' phase, not the 'commit'
> > phase.  Note that we only set these bits in the branch of 'check' where
> > intel_crtc->active is true so that we don't try to wait on a disabled
> > CRTC.
> >
> > The whole 'wait for vblank after update' flag should go away in the
> > future, once we start handling watermarks in a proper atomic manner.
> 
> Add this to the commit message:
> 
> Regression introduced by:
> 
> commit 2fdd7def16dd7580f297827930126c16b152ec11
> Author: Matt Roper <matthew.d.roper@intel.com>
> Date:   Wed Mar 4 10:49:04 2015 -0800
>     drm/i915: Don't clobber plane state on internal disables
> 
> (at least according to QA's bisect)
> 
> >
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Root-cause-analysis-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89550
> > Testcase: igt/pm_rpm/legacy-planes
> > Testcase: igt/pm_rpm/legacy-planes-dpms
> > Testcase: igt/pm_rpm/universal-planes
> > Testcase: igt/pm_rpm/universal-planes-dpms
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> > Paulo, can you test this patch and see if it solves the problem?  The only
> > platform I have access to (IVB) doesn't have runtime power management, so I
> > can't actually run the relevant testcases myself.
> 
> Yes, this patch makes the WARN go away on my BDW machine :)
> Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> One of the possible problems with this patch is that, before it, the
> wait_vblank was only set for ILK-BDW, but now we set it for all
> platforms. Do we really want this? Otherwise, it looks good, although
> I haven't been closely following the atomic rework to be able to
> assert this is really according to the grand plans. Daniel/Ville could
> comment here :)

Somethins seems a bit off to me if we end up calling .disable_plane()
with a disabled pipe. So fixing things so that won't happen
might be a better approach.

> 
> >
> >  drivers/gpu/drm/i915/intel_sprite.c | 24 ++++++++++--------------
> >  1 file changed, 10 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index a828736..fad1d9f 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -747,13 +747,6 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> >         I915_WRITE(SPRSURF(pipe), 0);
> >
> >         intel_flush_primary_plane(dev_priv, intel_crtc->plane);
> > -
> > -       /*
> > -        * Avoid underruns when disabling the sprite.
> > -        * FIXME remove once watermark updates are done properly.
> > -        */
> > -       intel_crtc->atomic.wait_vblank = true;
> > -       intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
> >  }
> >
> >  static int
> > @@ -937,13 +930,6 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> >         I915_WRITE(DVSSURF(pipe), 0);
> >
> >         intel_flush_primary_plane(dev_priv, intel_crtc->plane);
> > -
> > -       /*
> > -        * Avoid underruns when disabling the sprite.
> > -        * FIXME remove once watermark updates are done properly.
> > -        */
> > -       intel_crtc->atomic.wait_vblank = true;
> > -       intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
> >  }
> >
> >  /**
> > @@ -1262,6 +1248,16 @@ finish:
> >                     plane->state->fb->modifier[0] !=
> >                     state->base.fb->modifier[0])
> >                         intel_crtc->atomic.update_wm = true;
> > +
> > +               if (!state->visible) {
> > +                       /*
> > +                        * Avoid underruns when disabling the sprite.
> > +                        * FIXME remove once watermark updates are done properly.
> > +                        */
> > +                       intel_crtc->atomic.wait_vblank = true;
> > +                       intel_crtc->atomic.update_sprite_watermarks |=
> > +                               (1 << drm_plane_index(plane));
> > +               }
> >         }
> >
> >         return 0;
> > --
> > 1.8.5.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/i915: Move vblank wait determination to 'check' phase
  2015-03-19 19:36       ` Ville Syrjälä
@ 2015-03-20 10:22         ` Daniel Vetter
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2015-03-20 10:22 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Intel Graphics Development, Paulo Zanoni

On Thu, Mar 19, 2015 at 09:36:28PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 19, 2015 at 04:16:41PM -0300, Paulo Zanoni wrote:
> > 2015-03-18 19:04 GMT-03:00 Matt Roper <matthew.d.roper@intel.com>:
> > > Determining whether we'll need to wait for vblanks is something we
> > > should determine during the atomic 'check' phase, not the 'commit'
> > > phase.  Note that we only set these bits in the branch of 'check' where
> > > intel_crtc->active is true so that we don't try to wait on a disabled
> > > CRTC.
> > >
> > > The whole 'wait for vblank after update' flag should go away in the
> > > future, once we start handling watermarks in a proper atomic manner.
> > 
> > Add this to the commit message:
> > 
> > Regression introduced by:
> > 
> > commit 2fdd7def16dd7580f297827930126c16b152ec11
> > Author: Matt Roper <matthew.d.roper@intel.com>
> > Date:   Wed Mar 4 10:49:04 2015 -0800
> >     drm/i915: Don't clobber plane state on internal disables
> > 
> > (at least according to QA's bisect)
> > 
> > >
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Root-cause-analysis-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89550
> > > Testcase: igt/pm_rpm/legacy-planes
> > > Testcase: igt/pm_rpm/legacy-planes-dpms
> > > Testcase: igt/pm_rpm/universal-planes
> > > Testcase: igt/pm_rpm/universal-planes-dpms
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > > Paulo, can you test this patch and see if it solves the problem?  The only
> > > platform I have access to (IVB) doesn't have runtime power management, so I
> > > can't actually run the relevant testcases myself.
> > 
> > Yes, this patch makes the WARN go away on my BDW machine :)
> > Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Queued for -next, thanks for the patch.

> > One of the possible problems with this patch is that, before it, the
> > wait_vblank was only set for ILK-BDW, but now we set it for all
> > platforms. Do we really want this? Otherwise, it looks good, although
> > I haven't been closely following the atomic rework to be able to
> > assert this is really according to the grand plans. Daniel/Ville could
> > comment here :)
> 
> Somethins seems a bit off to me if we end up calling .disable_plane()
> with a disabled pipe. So fixing things so that won't happen
> might be a better approach.

Well we have some serious trouble still in these paths with the set_base
stuck somewhere in the middle where it shouldn't really hang out when
everything is off. So definitely more work to do in any case.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-03-20 10:20 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-18 19:15 [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled Paulo Zanoni
2015-03-18 22:01 ` Matt Roper
2015-03-18 22:04   ` [PATCH] drm/i915: Move vblank wait determination to 'check' phase Matt Roper
2015-03-19 11:43     ` shuang.he
2015-03-19 19:16     ` Paulo Zanoni
2015-03-19 19:36       ` Ville Syrjälä
2015-03-20 10:22         ` Daniel Vetter
2015-03-19 16:31   ` [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.