* [PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable
@ 2015-07-08 13:31 Patrik Jakobsson
2015-07-08 17:24 ` shuang.he
2015-07-10 11:22 ` Damien Lespiau
0 siblings, 2 replies; 8+ messages in thread
From: Patrik Jakobsson @ 2015-07-08 13:31 UTC (permalink / raw
To: intel-gfx
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(+)
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable
2015-07-08 13:31 [PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable Patrik Jakobsson
@ 2015-07-08 17:24 ` shuang.he
2015-07-10 11:22 ` Damien Lespiau
1 sibling, 0 replies; 8+ messages in thread
From: shuang.he @ 2015-07-08 17:24 UTC (permalink / raw
To: shuang.he, lei.a.liu, intel-gfx, patrik.jakobsson
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 '*'
_______________________________________________
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 v2] drm/i915: Don't forget to mark crtc as inactive after disable
2015-07-08 13:31 [PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable Patrik Jakobsson
2015-07-08 17:24 ` shuang.he
@ 2015-07-10 11:22 ` Damien Lespiau
2015-07-10 11:56 ` Patrik Jakobsson
2015-07-13 9:10 ` Maarten Lankhorst
1 sibling, 2 replies; 8+ messages in thread
From: Damien Lespiau @ 2015-07-10 11:22 UTC (permalink / raw
To: Patrik Jakobsson; +Cc: intel-gfx
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,
--
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
_______________________________________________
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 v2] drm/i915: Don't forget to mark crtc as inactive after disable
2015-07-10 11:22 ` Damien Lespiau
@ 2015-07-10 11:56 ` Patrik Jakobsson
2015-07-13 9:10 ` Maarten Lankhorst
1 sibling, 0 replies; 8+ messages in thread
From: Patrik Jakobsson @ 2015-07-10 11:56 UTC (permalink / raw
To: Damien Lespiau; +Cc: intel-gfx
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
_______________________________________________
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 v2] drm/i915: Don't forget to mark crtc as inactive after disable
2015-07-10 11:22 ` Damien Lespiau
2015-07-10 11:56 ` Patrik Jakobsson
@ 2015-07-13 9:10 ` Maarten Lankhorst
2015-07-13 9:49 ` Daniel Vetter
1 sibling, 1 reply; 8+ messages in thread
From: Maarten Lankhorst @ 2015-07-13 9:10 UTC (permalink / raw
To: Damien Lespiau, Patrik Jakobsson; +Cc: 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>
_______________________________________________
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 v2] drm/i915: Don't forget to mark crtc as inactive after disable
2015-07-13 9:10 ` Maarten Lankhorst
@ 2015-07-13 9:49 ` Daniel Vetter
2015-07-13 10:42 ` Damien Lespiau
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2015-07-13 9:49 UTC (permalink / raw
To: Maarten Lankhorst; +Cc: intel-gfx
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
--
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 v2] drm/i915: Don't forget to mark crtc as inactive after disable
2015-07-13 9:49 ` Daniel Vetter
@ 2015-07-13 10:42 ` Damien Lespiau
2015-07-13 14:53 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Damien Lespiau @ 2015-07-13 10:42 UTC (permalink / raw
To: Daniel Vetter; +Cc: intel-gfx
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
--
Damien
_______________________________________________
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 v2] drm/i915: Don't forget to mark crtc as inactive after disable
2015-07-13 10:42 ` Damien Lespiau
@ 2015-07-13 14:53 ` Daniel Vetter
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2015-07-13 14:53 UTC (permalink / raw
To: Damien Lespiau; +Cc: intel-gfx
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
--
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-07-13 14:50 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-08 13:31 [PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable Patrik Jakobsson
2015-07-08 17:24 ` shuang.he
2015-07-10 11:22 ` Damien Lespiau
2015-07-10 11:56 ` Patrik Jakobsson
2015-07-13 9:10 ` Maarten Lankhorst
2015-07-13 9:49 ` Daniel Vetter
2015-07-13 10:42 ` Damien Lespiau
2015-07-13 14:53 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).