[PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable

[PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable

[Patrik Jakobsson]

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

Re: [PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable

[shuang.he]

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

Re: [PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable

[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?

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

Re: [PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable

[Patrik Jakobsson]

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

Re: [PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable

[Maarten Lankhorst]

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

Re: [PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable

[Daniel Vetter]

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

Re: [PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable

[Damien Lespiau]

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

Re: [PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable

[Daniel Vetter]

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