[2/4] drm/i915: Don't check for wm changes until we've compute the wms fully
diff mbox series

Message ID 20200228203552.30273-2-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • [1/4] drm/i915: Don't check uv_wm in skl_plane_wm_equals()
Related show

Commit Message

Ville Syrjälä Feb. 28, 2020, 8:35 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we're comparing the watermarks between the old and new states
before we've fully computed the new watermarks. In particular
skl_build_pipe_wm() will not account for the amount of ddb space we'll
have. That information is only available during skl_compute_ddb()
which will proceed to zero out any watermark level exceeding the
ddb allocation. If we're short on ddb space this will end up
adding the plane to the state due erronously determining that the
watermarks have changed. Fix the problem by deferring
skl_wm_add_affected_planes() until we have the final watermarks
computed.

Noticed this when trying enable transition watermarks on glk.
We now computed the trans_wm as 28, but we only had 14 blocks
of ddb, and thus skl_compute_ddb() ended up disabling the cursor
trans_wm every time. Thus we ended up adding the cursor to every
commit that didn't actually affect the cursor at all.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Comments

Souza, Jose March 4, 2020, 12:21 a.m. UTC | #1
On Fri, 2020-02-28 at 22:35 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we're comparing the watermarks between the old and new
> states
> before we've fully computed the new watermarks. In particular
> skl_build_pipe_wm() will not account for the amount of ddb space
> we'll
> have. That information is only available during skl_compute_ddb()
> which will proceed to zero out any watermark level exceeding the
> ddb allocation. If we're short on ddb space this will end up
> adding the plane to the state due erronously determining that the
> watermarks have changed. Fix the problem by deferring
> skl_wm_add_affected_planes() until we have the final watermarks
> computed.
> 
> Noticed this when trying enable transition watermarks on glk.
> We now computed the trans_wm as 28, but we only had 14 blocks
> of ddb, and thus skl_compute_ddb() ended up disabling the cursor
> trans_wm every time. Thus we ended up adding the cursor to every
> commit that didn't actually affect the cursor at all.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 39299811b650..a3d76e69caae 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5762,16 +5762,24 @@ skl_compute_wm(struct intel_atomic_state
> *state)
>  		ret = skl_build_pipe_wm(new_crtc_state);
>  		if (ret)
>  			return ret;
> -
> -		ret = skl_wm_add_affected_planes(state, crtc);
> -		if (ret)
> -			return ret;
>  	}
>  
>  	ret = skl_compute_ddb(state);
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * skl_compute_ddb() will have adjusted the final watermarks
> +	 * based on how much ddb is available. Now we can actually
> +	 * check if the final watermarks changed.
> +	 */
> +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> old_crtc_state,
> +					    new_crtc_state, i) {
> +		ret = skl_wm_add_affected_planes(state, crtc);
> +		if (ret)
> +			return ret;
> +	}

skl_compute_ddb() is already calling skl_wm_add_affected_planes() after
do the ddb allocation for each pipe, so we could remove this chunk,
with that:

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> +
>  	skl_print_wm_changes(state);
>  
>  	return 0;
Ville Syrjälä March 4, 2020, 11:46 a.m. UTC | #2
On Wed, Mar 04, 2020 at 12:21:01AM +0000, Souza, Jose wrote:
> On Fri, 2020-02-28 at 22:35 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently we're comparing the watermarks between the old and new
> > states
> > before we've fully computed the new watermarks. In particular
> > skl_build_pipe_wm() will not account for the amount of ddb space
> > we'll
> > have. That information is only available during skl_compute_ddb()
> > which will proceed to zero out any watermark level exceeding the
> > ddb allocation. If we're short on ddb space this will end up
> > adding the plane to the state due erronously determining that the
> > watermarks have changed. Fix the problem by deferring
> > skl_wm_add_affected_planes() until we have the final watermarks
> > computed.
> > 
> > Noticed this when trying enable transition watermarks on glk.
> > We now computed the trans_wm as 28, but we only had 14 blocks
> > of ddb, and thus skl_compute_ddb() ended up disabling the cursor
> > trans_wm every time. Thus we ended up adding the cursor to every
> > commit that didn't actually affect the cursor at all.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 39299811b650..a3d76e69caae 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5762,16 +5762,24 @@ skl_compute_wm(struct intel_atomic_state
> > *state)
> >  		ret = skl_build_pipe_wm(new_crtc_state);
> >  		if (ret)
> >  			return ret;
> > -
> > -		ret = skl_wm_add_affected_planes(state, crtc);
> > -		if (ret)
> > -			return ret;
> >  	}
> >  
> >  	ret = skl_compute_ddb(state);
> >  	if (ret)
> >  		return ret;
> >  
> > +	/*
> > +	 * skl_compute_ddb() will have adjusted the final watermarks
> > +	 * based on how much ddb is available. Now we can actually
> > +	 * check if the final watermarks changed.
> > +	 */
> > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > +					    new_crtc_state, i) {
> > +		ret = skl_wm_add_affected_planes(state, crtc);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> skl_compute_ddb() is already calling skl_wm_add_affected_planes() after
> do the ddb allocation for each pipe, so we could remove this chunk,

skl_compute_ddb() calls skl_*ddb*_add_affected_planes(), which is a
different thing.

> with that:
> 
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> 
> > +
> >  	skl_print_wm_changes(state);
> >  
> >  	return 0;
Souza, Jose March 4, 2020, 11:25 p.m. UTC | #3
On Wed, 2020-03-04 at 13:46 +0200, Ville Syrjälä wrote:
> On Wed, Mar 04, 2020 at 12:21:01AM +0000, Souza, Jose wrote:
> > On Fri, 2020-02-28 at 22:35 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Currently we're comparing the watermarks between the old and new
> > > states
> > > before we've fully computed the new watermarks. In particular
> > > skl_build_pipe_wm() will not account for the amount of ddb space
> > > we'll
> > > have. That information is only available during skl_compute_ddb()
> > > which will proceed to zero out any watermark level exceeding the
> > > ddb allocation. If we're short on ddb space this will end up
> > > adding the plane to the state due erronously determining that the
> > > watermarks have changed. Fix the problem by deferring
> > > skl_wm_add_affected_planes() until we have the final watermarks
> > > computed.
> > > 
> > > Noticed this when trying enable transition watermarks on glk.
> > > We now computed the trans_wm as 28, but we only had 14 blocks
> > > of ddb, and thus skl_compute_ddb() ended up disabling the cursor
> > > trans_wm every time. Thus we ended up adding the cursor to every
> > > commit that didn't actually affect the cursor at all.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++++----
> > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 39299811b650..a3d76e69caae 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -5762,16 +5762,24 @@ skl_compute_wm(struct intel_atomic_state
> > > *state)
> > >  		ret = skl_build_pipe_wm(new_crtc_state);
> > >  		if (ret)
> > >  			return ret;
> > > -
> > > -		ret = skl_wm_add_affected_planes(state, crtc);
> > > -		if (ret)
> > > -			return ret;
> > >  	}
> > >  
> > >  	ret = skl_compute_ddb(state);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	/*
> > > +	 * skl_compute_ddb() will have adjusted the final watermarks
> > > +	 * based on how much ddb is available. Now we can actually
> > > +	 * check if the final watermarks changed.
> > > +	 */
> > > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > old_crtc_state,
> > > +					    new_crtc_state, i) {
> > > +		ret = skl_wm_add_affected_planes(state, crtc);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > 
> > skl_compute_ddb() is already calling skl_wm_add_affected_planes()
> > after
> > do the ddb allocation for each pipe, so we could remove this chunk,
> 
> skl_compute_ddb() calls skl_*ddb*_add_affected_planes(), which is a
> different thing..

Thanks

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> > with that:
> > 
> > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> > 
> > > +
> > >  	skl_print_wm_changes(state);
> > >  
> > >  	return 0;
Ville Syrjälä March 5, 2020, 1:55 p.m. UTC | #4
On Wed, Mar 04, 2020 at 11:25:42PM +0000, Souza, Jose wrote:
> On Wed, 2020-03-04 at 13:46 +0200, Ville Syrjälä wrote:
> > On Wed, Mar 04, 2020 at 12:21:01AM +0000, Souza, Jose wrote:
> > > On Fri, 2020-02-28 at 22:35 +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Currently we're comparing the watermarks between the old and new
> > > > states
> > > > before we've fully computed the new watermarks. In particular
> > > > skl_build_pipe_wm() will not account for the amount of ddb space
> > > > we'll
> > > > have. That information is only available during skl_compute_ddb()
> > > > which will proceed to zero out any watermark level exceeding the
> > > > ddb allocation. If we're short on ddb space this will end up
> > > > adding the plane to the state due erronously determining that the
> > > > watermarks have changed. Fix the problem by deferring
> > > > skl_wm_add_affected_planes() until we have the final watermarks
> > > > computed.
> > > > 
> > > > Noticed this when trying enable transition watermarks on glk.
> > > > We now computed the trans_wm as 28, but we only had 14 blocks
> > > > of ddb, and thus skl_compute_ddb() ended up disabling the cursor
> > > > trans_wm every time. Thus we ended up adding the cursor to every
> > > > commit that didn't actually affect the cursor at all.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++++----
> > > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 39299811b650..a3d76e69caae 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -5762,16 +5762,24 @@ skl_compute_wm(struct intel_atomic_state
> > > > *state)
> > > >  		ret = skl_build_pipe_wm(new_crtc_state);
> > > >  		if (ret)
> > > >  			return ret;
> > > > -
> > > > -		ret = skl_wm_add_affected_planes(state, crtc);
> > > > -		if (ret)
> > > > -			return ret;
> > > >  	}
> > > >  
> > > >  	ret = skl_compute_ddb(state);
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > +	/*
> > > > +	 * skl_compute_ddb() will have adjusted the final watermarks
> > > > +	 * based on how much ddb is available. Now we can actually
> > > > +	 * check if the final watermarks changed.
> > > > +	 */
> > > > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > > old_crtc_state,
> > > > +					    new_crtc_state, i) {
> > > > +		ret = skl_wm_add_affected_planes(state, crtc);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > 
> > > skl_compute_ddb() is already calling skl_wm_add_affected_planes()
> > > after
> > > do the ddb allocation for each pipe, so we could remove this chunk,
> > 
> > skl_compute_ddb() calls skl_*ddb*_add_affected_planes(), which is a
> > different thing..
> 
> Thanks

No, thank you for the review. Series pushed to dinq.

> 
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> 
> > 
> > > with that:
> > > 
> > > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> > > 
> > > > +
> > > >  	skl_print_wm_changes(state);
> > > >  
> > > >  	return 0;

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 39299811b650..a3d76e69caae 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5762,16 +5762,24 @@  skl_compute_wm(struct intel_atomic_state *state)
 		ret = skl_build_pipe_wm(new_crtc_state);
 		if (ret)
 			return ret;
-
-		ret = skl_wm_add_affected_planes(state, crtc);
-		if (ret)
-			return ret;
 	}
 
 	ret = skl_compute_ddb(state);
 	if (ret)
 		return ret;
 
+	/*
+	 * skl_compute_ddb() will have adjusted the final watermarks
+	 * based on how much ddb is available. Now we can actually
+	 * check if the final watermarks changed.
+	 */
+	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+					    new_crtc_state, i) {
+		ret = skl_wm_add_affected_planes(state, crtc);
+		if (ret)
+			return ret;
+	}
+
 	skl_print_wm_changes(state);
 
 	return 0;