| Message ID | 1436362312-12843-1-git-send-email-patrik.jakobsson@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6751
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK 302/302 302/302
SNB 312/316 312/316
IVB 343/343 343/343
BYT -1 287/287 286/287
HSW 380/380 380/380
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*BYT igt@gem_partial_pwrite_pread@reads-display PASS(1) FAIL(1)
Note: You need to pay more attention to line start with '*'
Hi Patrik, Please do Cc the patch author and reviewer when finding a regression, they are superb candidates for the review, especially when they are busy rewriting the display code. On Wed, Jul 08, 2015 at 03:31:52PM +0200, Patrik Jakobsson wrote: > Watermark calculations depend on the intel_crtc->active flag to be set > properly. Suspend/resume is broken on SKL and we also get DDB mismatches > without this patch. > > The regression was introduced in: > > commit eddfcbcdc27fbecb33bff098967bbdd7ca75bfa6 > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Date: Mon Jun 15 12:33:53 2015 +0200 > > drm/i915: Update less state during modeset. > > No need to repeatedly call update_watermarks, or update_fbc. > Down to a single call to update_watermarks in .crtc_enable > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > Tested-by(IVB): Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > v2: Don't touch disable_shared_dpll() > > Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> We do need to update the watermarks in disable() as well, to repartition the DDB and update the watermarks based on the new allocation. Maarten, Matt, I've no clue where you want the crtc state update, where the atomic WM work is at, could you comment? Thanks,
On Fri, Jul 10, 2015 at 12:22:51PM +0100, Damien Lespiau wrote: > Hi Patrik, > > Please do Cc the patch author and reviewer when finding a regression, > they are superb candidates for the review, especially when they are busy > rewriting the display code. Hmm, I figured they would be picked up from the commit msg by git-send-email but maybe that doesn't work on indented lines. Anyways, CCing them now. Thanks Patrik > > On Wed, Jul 08, 2015 at 03:31:52PM +0200, Patrik Jakobsson wrote: > > Watermark calculations depend on the intel_crtc->active flag to be set > > properly. Suspend/resume is broken on SKL and we also get DDB mismatches > > without this patch. > > > > The regression was introduced in: > > > > commit eddfcbcdc27fbecb33bff098967bbdd7ca75bfa6 > > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Date: Mon Jun 15 12:33:53 2015 +0200 > > > > drm/i915: Update less state during modeset. > > > > No need to repeatedly call update_watermarks, or update_fbc. > > Down to a single call to update_watermarks in .crtc_enable > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > Tested-by(IVB): Matt Roper <matthew.d.roper@intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > v2: Don't touch disable_shared_dpll() > > > > Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > We do need to update the watermarks in disable() as well, to repartition > the DDB and update the watermarks based on the new allocation. > > Maarten, Matt, I've no clue where you want the crtc state update, where > the atomic WM work is at, could you comment? > > Thanks, > > -- > Damien > > > --- > > drivers/gpu/drm/i915/intel_display.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 3c2425f..b4f7a4f 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -5046,6 +5046,9 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) > > > > ironlake_fdi_pll_disable(intel_crtc); > > } > > + > > + intel_crtc->active = false; > > + intel_update_watermarks(crtc); > > } > > > > static void haswell_crtc_disable(struct drm_crtc *crtc) > > @@ -5091,6 +5094,9 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) > > for_each_encoder_on_crtc(dev, crtc, encoder) > > if (encoder->post_disable) > > encoder->post_disable(encoder); > > + > > + intel_crtc->active = false; > > + intel_update_watermarks(crtc); > > } > > > > static void i9xx_pfit_enable(struct intel_crtc *crtc) > > @@ -6155,6 +6161,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > > > > if (!IS_GEN2(dev)) > > intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); > > + > > + intel_crtc->active = false; > > + intel_update_watermarks(crtc); > > } > > > > static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) > > -- > > 2.1.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 10-07-15 om 13:22 schreef Damien Lespiau: > Hi Patrik, > > Please do Cc the patch author and reviewer when finding a regression, > they are superb candidates for the review, especially when they are busy > rewriting the display code. > > On Wed, Jul 08, 2015 at 03:31:52PM +0200, Patrik Jakobsson wrote: >> Watermark calculations depend on the intel_crtc->active flag to be set >> properly. Suspend/resume is broken on SKL and we also get DDB mismatches >> without this patch. >> >> The regression was introduced in: >> >> commit eddfcbcdc27fbecb33bff098967bbdd7ca75bfa6 >> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Date: Mon Jun 15 12:33:53 2015 +0200 >> >> drm/i915: Update less state during modeset. >> >> No need to repeatedly call update_watermarks, or update_fbc. >> Down to a single call to update_watermarks in .crtc_enable >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> >> Tested-by(IVB): Matt Roper <matthew.d.roper@intel.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> >> v2: Don't touch disable_shared_dpll() >> >> Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > We do need to update the watermarks in disable() as well, to repartition > the DDB and update the watermarks based on the new allocation. > > Maarten, Matt, I've no clue where you want the crtc state update, where > the atomic WM work is at, could you comment? > I'd rather have we had a better way of updating watermarks, but keeping it in the .crtc_disable hook looks good to me right now. Some proper atomic solution will eventually have to be done. :) Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
On Mon, Jul 13, 2015 at 11:10:51AM +0200, Maarten Lankhorst wrote: > Op 10-07-15 om 13:22 schreef Damien Lespiau: > > Hi Patrik, > > > > Please do Cc the patch author and reviewer when finding a regression, > > they are superb candidates for the review, especially when they are busy > > rewriting the display code. Yeah you need to list everyone you want to Cc: explicitly. > > On Wed, Jul 08, 2015 at 03:31:52PM +0200, Patrik Jakobsson wrote: > >> Watermark calculations depend on the intel_crtc->active flag to be set > >> properly. Suspend/resume is broken on SKL and we also get DDB mismatches > >> without this patch. > >> > >> The regression was introduced in: > >> > >> commit eddfcbcdc27fbecb33bff098967bbdd7ca75bfa6 > >> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Date: Mon Jun 15 12:33:53 2015 +0200 > >> > >> drm/i915: Update less state during modeset. > >> > >> No need to repeatedly call update_watermarks, or update_fbc. > >> Down to a single call to update_watermarks in .crtc_enable > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > >> Tested-by(IVB): Matt Roper <matthew.d.roper@intel.com> > >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > >> v2: Don't touch disable_shared_dpll() > >> > >> Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > We do need to update the watermarks in disable() as well, to repartition > > the DDB and update the watermarks based on the new allocation. > > > > Maarten, Matt, I've no clue where you want the crtc state update, where > > the atomic WM work is at, could you comment? > > > I'd rather have we had a better way of updating watermarks, but keeping it in > the .crtc_disable hook looks good to me right now. Some proper atomic > solution will eventually have to be done. :) > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Queued for -next, thanks for the patch. -Daniel
On Mon, Jul 13, 2015 at 11:49:49AM +0200, Daniel Vetter wrote: > On Mon, Jul 13, 2015 at 11:10:51AM +0200, Maarten Lankhorst wrote: > > Op 10-07-15 om 13:22 schreef Damien Lespiau: > > > Hi Patrik, > > > > > > Please do Cc the patch author and reviewer when finding a regression, > > > they are superb candidates for the review, especially when they are busy > > > rewriting the display code. > > Yeah you need to list everyone you want to Cc: explicitly. Also need the Bugzilla reference when fixing a bug. In this case: https://bugs.freedesktop.org/show_bug.cgi?id=91203
On Mon, Jul 13, 2015 at 11:42:34AM +0100, Damien Lespiau wrote: > On Mon, Jul 13, 2015 at 11:49:49AM +0200, Daniel Vetter wrote: > > On Mon, Jul 13, 2015 at 11:10:51AM +0200, Maarten Lankhorst wrote: > > > Op 10-07-15 om 13:22 schreef Damien Lespiau: > > > > Hi Patrik, > > > > > > > > Please do Cc the patch author and reviewer when finding a regression, > > > > they are superb candidates for the review, especially when they are busy > > > > rewriting the display code. > > > > Yeah you need to list everyone you want to Cc: explicitly. > > Also need the Bugzilla reference when fixing a bug. In this case: > > https://bugs.freedesktop.org/show_bug.cgi?id=91203 Added, thanks for supplying the link. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3c2425f..b4f7a4f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5046,6 +5046,9 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) ironlake_fdi_pll_disable(intel_crtc); } + + intel_crtc->active = false; + intel_update_watermarks(crtc); } static void haswell_crtc_disable(struct drm_crtc *crtc) @@ -5091,6 +5094,9 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder->post_disable) encoder->post_disable(encoder); + + intel_crtc->active = false; + intel_update_watermarks(crtc); } static void i9xx_pfit_enable(struct intel_crtc *crtc) @@ -6155,6 +6161,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) if (!IS_GEN2(dev)) intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); + + intel_crtc->active = false; + intel_update_watermarks(crtc); } static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
Watermark calculations depend on the intel_crtc->active flag to be set properly. Suspend/resume is broken on SKL and we also get DDB mismatches without this patch. The regression was introduced in: commit eddfcbcdc27fbecb33bff098967bbdd7ca75bfa6 Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Date: Mon Jun 15 12:33:53 2015 +0200 drm/i915: Update less state during modeset. No need to repeatedly call update_watermarks, or update_fbc. Down to a single call to update_watermarks in .crtc_enable Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> Tested-by(IVB): Matt Roper <matthew.d.roper@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> v2: Don't touch disable_shared_dpll() Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 9 +++++++++ 1 file changed, 9 insertions(+)