[v4,7/8] drm/i915: Do not compute watermarks on a noop.

Submitted by Maarten Lankhorst on Feb. 10, 2016, 12:49 p.m.

Details

Message ID 1455108583-29227-8-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show
Series "Kill off intel_crtc->atomic!" ( rev: 10 9 8 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Maarten Lankhorst Feb. 10, 2016, 12:49 p.m.
No atomic state should be included after all validation when nothing has
changed. During modeset all active planes will be added to the state,
while disabled planes won't change their state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  3 +-
 drivers/gpu/drm/i915/intel_drv.h     | 13 ++++++++
 drivers/gpu/drm/i915/intel_pm.c      | 61 +++++++++++++++++++++---------------
 3 files changed, 51 insertions(+), 26 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 00cb261c6787..6bb1f5dbc7a0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11910,7 +11910,8 @@  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 	}
 
 	ret = 0;
-	if (dev_priv->display.compute_pipe_wm) {
+	if (dev_priv->display.compute_pipe_wm &&
+	    (mode_changed || pipe_config->update_pipe || crtc_state->planes_changed)) {
 		ret = dev_priv->display.compute_pipe_wm(intel_crtc, state);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8effb9ece21e..144597ac74e3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1583,6 +1583,19 @@  intel_atomic_get_crtc_state(struct drm_atomic_state *state,
 
 	return to_intel_crtc_state(crtc_state);
 }
+
+static inline struct intel_plane_state *
+intel_atomic_get_existing_plane_state(struct drm_atomic_state *state,
+				      struct intel_plane *plane)
+{
+	struct drm_plane_state *plane_state;
+
+	plane_state = drm_atomic_get_existing_plane_state(state, &plane->base);
+
+	return to_intel_plane_state(plane_state);
+}
+
+
 int intel_atomic_setup_scalers(struct drm_device *dev,
 	struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 379eabe093cb..8fb8c6891ae6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2010,11 +2010,18 @@  static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
 		cur_latency *= 5;
 	}
 
-	result->pri_val = ilk_compute_pri_wm(cstate, pristate,
-					     pri_latency, level);
-	result->spr_val = ilk_compute_spr_wm(cstate, sprstate, spr_latency);
-	result->cur_val = ilk_compute_cur_wm(cstate, curstate, cur_latency);
-	result->fbc_val = ilk_compute_fbc_wm(cstate, pristate, result->pri_val);
+	if (pristate) {
+		result->pri_val = ilk_compute_pri_wm(cstate, pristate,
+						     pri_latency, level);
+		result->fbc_val = ilk_compute_fbc_wm(cstate, pristate, result->pri_val);
+	}
+
+	if (sprstate)
+		result->spr_val = ilk_compute_spr_wm(cstate, sprstate, spr_latency);
+
+	if (curstate)
+		result->cur_val = ilk_compute_cur_wm(cstate, curstate, cur_latency);
+
 	result->enable = true;
 }
 
@@ -2287,7 +2294,6 @@  static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 	const struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc_state *cstate = NULL;
 	struct intel_plane *intel_plane;
-	struct drm_plane_state *ps;
 	struct intel_plane_state *pristate = NULL;
 	struct intel_plane_state *sprstate = NULL;
 	struct intel_plane_state *curstate = NULL;
@@ -2306,30 +2312,37 @@  static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 	memset(pipe_wm, 0, sizeof(*pipe_wm));
 
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-		ps = drm_atomic_get_plane_state(state,
-						&intel_plane->base);
-		if (IS_ERR(ps))
-			return PTR_ERR(ps);
+		struct intel_plane_state *ps;
+
+		ps = intel_atomic_get_existing_plane_state(state,
+							   intel_plane);
+		if (!ps)
+			continue;
 
 		if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
-			pristate = to_intel_plane_state(ps);
-		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
-			sprstate = to_intel_plane_state(ps);
-		else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
-			curstate = to_intel_plane_state(ps);
+			pristate = ps;
+		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY) {
+			sprstate = ps;
+
+			if (ps) {
+				pipe_wm->sprites_enabled = sprstate->visible;
+				pipe_wm->sprites_scaled = sprstate->visible &&
+					(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
+					drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
+			}
+		} else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
+			curstate = ps;
 	}
 
-	config.sprites_enabled = sprstate->visible;
-	config.sprites_scaled = sprstate->visible &&
-		(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
-		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
+	config.sprites_enabled = pipe_wm->sprites_enabled;
+	config.sprites_scaled = pipe_wm->sprites_scaled;
 
 	pipe_wm->pipe_enabled = cstate->base.active;
 	pipe_wm->sprites_enabled = config.sprites_enabled;
 	pipe_wm->sprites_scaled = config.sprites_scaled;
 
 	/* ILK/SNB: LP2+ watermarks only w/o sprites */
-	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
+	if (INTEL_INFO(dev)->gen <= 6 && pipe_wm->sprites_enabled)
 		max_level = 1;
 
 	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
@@ -2352,20 +2365,18 @@  static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 	ilk_compute_wm_reg_maximums(dev, 1, &max);
 
 	for (level = 1; level <= max_level; level++) {
-		struct intel_wm_level wm = {};
+		struct intel_wm_level *wm = &pipe_wm->wm[level];
 
 		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
-				     pristate, sprstate, curstate, &wm);
+				     pristate, sprstate, curstate, wm);
 
 		/*
 		 * Disable any watermark level that exceeds the
 		 * register maximums since such watermarks are
 		 * always invalid.
 		 */
-		if (!ilk_validate_wm_level(level, &max, &wm))
+		if (!ilk_validate_wm_level(level, &max, wm))
 			break;
-
-		pipe_wm->wm[level] = wm;
 	}
 
 	return 0;

Comments

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> No atomic state should be included after all validation when nothing

> has

> changed. During modeset all active planes will be added to the state,

> while disabled planes won't change their state.


As someone who is also not super familiar with the new watermarks code,
I really really wish I had a more detailed commit message to allow me
to understand your train of thought. I'll ask some questions below to
validate my understanding.

> 

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

> Cc: Matt Roper <matthew.d.roper@intel.com>

> ---

>  drivers/gpu/drm/i915/intel_display.c |  3 +-

>  drivers/gpu/drm/i915/intel_drv.h     | 13 ++++++++

>  drivers/gpu/drm/i915/intel_pm.c      | 61 +++++++++++++++++++++-----

> ----------

>  3 files changed, 51 insertions(+), 26 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/intel_display.c

> b/drivers/gpu/drm/i915/intel_display.c

> index 00cb261c6787..6bb1f5dbc7a0 100644

> --- a/drivers/gpu/drm/i915/intel_display.c

> +++ b/drivers/gpu/drm/i915/intel_display.c

> @@ -11910,7 +11910,8 @@ static int intel_crtc_atomic_check(struct

> drm_crtc *crtc,

>  	}

>  

>  	ret = 0;

> -	if (dev_priv->display.compute_pipe_wm) {

> +	if (dev_priv->display.compute_pipe_wm &&

> +	    (mode_changed || pipe_config->update_pipe || crtc_state-

> >planes_changed)) {

>  		ret = dev_priv->display.compute_pipe_wm(intel_crtc,

> state);

>  		if (ret)

>  			return ret;


Can't this chunk be on its own separate commit? I'm not sure why the
rest of the commit is related to this change.

It seems the rest of the commit is aimed reducing the number of planes
we have to lock, not about not computing WMs if nothing in the pipe has
changed.

> diff --git a/drivers/gpu/drm/i915/intel_drv.h

> b/drivers/gpu/drm/i915/intel_drv.h

> index 8effb9ece21e..144597ac74e3 100644

> --- a/drivers/gpu/drm/i915/intel_drv.h

> +++ b/drivers/gpu/drm/i915/intel_drv.h

> @@ -1583,6 +1583,19 @@ intel_atomic_get_crtc_state(struct

> drm_atomic_state *state,

>  

>  	return to_intel_crtc_state(crtc_state);

>  }

> +

> +static inline struct intel_plane_state *

> +intel_atomic_get_existing_plane_state(struct drm_atomic_state

> *state,

> +				      struct intel_plane *plane)

> +{

> +	struct drm_plane_state *plane_state;

> +

> +	plane_state = drm_atomic_get_existing_plane_state(state,

> &plane->base);

> +

> +	return to_intel_plane_state(plane_state);

> +}

> +

> +


Two newlines above.

It seems you'll be able to simplify a lot of stuff with this new
function. I'm looking forward to review that patch :)


>  int intel_atomic_setup_scalers(struct drm_device *dev,

>  	struct intel_crtc *intel_crtc,

>  	struct intel_crtc_state *crtc_state);

> diff --git a/drivers/gpu/drm/i915/intel_pm.c

> b/drivers/gpu/drm/i915/intel_pm.c

> index 379eabe093cb..8fb8c6891ae6 100644

> --- a/drivers/gpu/drm/i915/intel_pm.c

> +++ b/drivers/gpu/drm/i915/intel_pm.c

> @@ -2010,11 +2010,18 @@ static void ilk_compute_wm_level(const struct

> drm_i915_private *dev_priv,

>  		cur_latency *= 5;

>  	}

>  

> -	result->pri_val = ilk_compute_pri_wm(cstate, pristate,

> -					     pri_latency, level);

> -	result->spr_val = ilk_compute_spr_wm(cstate, sprstate,

> spr_latency);

> -	result->cur_val = ilk_compute_cur_wm(cstate, curstate,

> cur_latency);

> -	result->fbc_val = ilk_compute_fbc_wm(cstate, pristate,

> result->pri_val);

> +	if (pristate) {

> +		result->pri_val = ilk_compute_pri_wm(cstate,

> pristate,

> +						     pri_latency,

> level);

> +		result->fbc_val = ilk_compute_fbc_wm(cstate,

> pristate, result->pri_val);

> +	}

> +

> +	if (sprstate)

> +		result->spr_val = ilk_compute_spr_wm(cstate,

> sprstate, spr_latency);

> +

> +	if (curstate)

> +		result->cur_val = ilk_compute_cur_wm(cstate,

> curstate, cur_latency);

> +

>  	result->enable = true;

>  }

>  

> @@ -2287,7 +2294,6 @@ static int ilk_compute_pipe_wm(struct

> intel_crtc *intel_crtc,

>  	const struct drm_i915_private *dev_priv = dev->dev_private;

>  	struct intel_crtc_state *cstate = NULL;

>  	struct intel_plane *intel_plane;

> -	struct drm_plane_state *ps;

>  	struct intel_plane_state *pristate = NULL;

>  	struct intel_plane_state *sprstate = NULL;

>  	struct intel_plane_state *curstate = NULL;

> @@ -2306,30 +2312,37 @@ static int ilk_compute_pipe_wm(struct

> intel_crtc *intel_crtc,

>  	memset(pipe_wm, 0, sizeof(*pipe_wm));

>  

>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {

> -		ps = drm_atomic_get_plane_state(state,

> -						&intel_plane->base);

> -		if (IS_ERR(ps))

> -			return PTR_ERR(ps);

> +		struct intel_plane_state *ps;

> +

> +		ps = intel_atomic_get_existing_plane_state(state,

> +							   intel_pla

> ne);

> +		if (!ps)

> +			continue;


Ok, let me see if I understood: if some of these planes is not part of
the atomic commit, you're not going to include it in the calculations
since its value is not going to change. This would allow us lock less
planes, which is the main advantage. Is this correct?

If yes, how much do we really gain by saving some plane locking?

So by not assigning values to result->x_val at ilk_compute_wm_level()
when NULL is passed you're going to preserve whatever correct values
were already set earlier, so you don't need to recompute them. Is this
correct?

If the answer to the above is "yes", then don't we need to remove the
memset(pipe_wm, 0) at the beginning of ilk_compute_wm_level? Otherwise,
nothing will be preserved.

There's also the zero-initialization of "config" to consider.

Or maybe instead of all of the above, we're working under the
assumption that if some of the planes is not part of the atomic commit,
then all its relevant WM values will be zero?

>  

>  		if (intel_plane->base.type ==

> DRM_PLANE_TYPE_PRIMARY)

> -			pristate = to_intel_plane_state(ps);

> -		else if (intel_plane->base.type ==

> DRM_PLANE_TYPE_OVERLAY)

> -			sprstate = to_intel_plane_state(ps);

> -		else if (intel_plane->base.type ==

> DRM_PLANE_TYPE_CURSOR)

> -			curstate = to_intel_plane_state(ps);

> +			pristate = ps;

> +		else if (intel_plane->base.type ==

> DRM_PLANE_TYPE_OVERLAY) {

> +			sprstate = ps;

> +

> +			if (ps) {

> +				pipe_wm->sprites_enabled = sprstate-

> >visible;

> +				pipe_wm->sprites_scaled = sprstate-

> >visible &&


You're setting these values here...

> +					(drm_rect_width(&sprstate-

> >dst) != drm_rect_width(&sprstate->src) >> 16 ||

> +					drm_rect_height(&sprstate-

> >dst) != drm_rect_height(&sprstate->src) >> 16);

> +			}

> +		} else if (intel_plane->base.type ==

> DRM_PLANE_TYPE_CURSOR)


(missing brackets here, but if you follow my suggestion below you won't
need them)

> +			curstate = ps;

>  	}

>  

> -	config.sprites_enabled = sprstate->visible;

> -	config.sprites_scaled = sprstate->visible &&

> -		(drm_rect_width(&sprstate->dst) !=

> drm_rect_width(&sprstate->src) >> 16 ||

> -		drm_rect_height(&sprstate->dst) !=

> drm_rect_height(&sprstate->src) >> 16);

> +	config.sprites_enabled = pipe_wm->sprites_enabled;

> +	config.sprites_scaled = pipe_wm->sprites_scaled;

>  

>  	pipe_wm->pipe_enabled = cstate->base.active;

>  	pipe_wm->sprites_enabled = config.sprites_enabled;

>  	pipe_wm->sprites_scaled = config.sprites_scaled;


...but then we re-set them here.

Either you could remove these assignments here, or you could move the
"if (ps)" from the loop to outside it, becoming "if (sprstate)", making
the post-patch code similar to the pre-patch code. Since both pipe_wm
and config are zero-initialized you don't even need to think about the
!sprstate case.

>  

>  	/* ILK/SNB: LP2+ watermarks only w/o sprites */

> -	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)

> +	if (INTEL_INFO(dev)->gen <= 6 && pipe_wm->sprites_enabled)

>  		max_level = 1;

>  

>  	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */

> @@ -2352,20 +2365,18 @@ static int ilk_compute_pipe_wm(struct

> intel_crtc *intel_crtc,

>  	ilk_compute_wm_reg_maximums(dev, 1, &max);

>  

>  	for (level = 1; level <= max_level; level++) {

> -		struct intel_wm_level wm = {};

> +		struct intel_wm_level *wm = &pipe_wm->wm[level];

>  

>  		ilk_compute_wm_level(dev_priv, intel_crtc, level,

> cstate,

> -				     pristate, sprstate, curstate,

> &wm);

> +				     pristate, sprstate, curstate,

> wm);

>  

>  		/*

>  		 * Disable any watermark level that exceeds the

>  		 * register maximums since such watermarks are

>  		 * always invalid.

>  		 */

> -		if (!ilk_validate_wm_level(level, &max, &wm))

> +		if (!ilk_validate_wm_level(level, &max, wm))

>  			break;

> -

> -		pipe_wm->wm[level] = wm;


The change in the chunk above is that we're now leaving with non-zero
wm->{pri,spr,cur}_val if some level is invalid. Given the current
complexity of the code, it's not trivial verify whether nothing later
is assuming that invalid levels are all-zero, but it looks like we're
safe. Anyway, could you please move this to a separate patch that comes
before the other changes? It seems this change would be safe alone, and
we're seeing problems with WMs these days, so having nice bisectability
is a huge plus.

Thanks,
Paulo

>  	}

>  

>  	return 0;
Op 18-02-16 om 21:51 schreef Zanoni, Paulo R:
> Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
>> No atomic state should be included after all validation when nothing
>> has
>> changed. During modeset all active planes will be added to the state,
>> while disabled planes won't change their state.
> As someone who is also not super familiar with the new watermarks code,
> I really really wish I had a more detailed commit message to allow me
> to understand your train of thought. I'll ask some questions below to
> validate my understanding.
>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c |  3 +-
>>  drivers/gpu/drm/i915/intel_drv.h     | 13 ++++++++
>>  drivers/gpu/drm/i915/intel_pm.c      | 61 +++++++++++++++++++++-----
>> ----------
>>  3 files changed, 51 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 00cb261c6787..6bb1f5dbc7a0 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11910,7 +11910,8 @@ static int intel_crtc_atomic_check(struct
>> drm_crtc *crtc,
>>  	}
>>  
>>  	ret = 0;
>> -	if (dev_priv->display.compute_pipe_wm) {
>> +	if (dev_priv->display.compute_pipe_wm &&
>> +	    (mode_changed || pipe_config->update_pipe || crtc_state-
>>> planes_changed)) {
>>  		ret = dev_priv->display.compute_pipe_wm(intel_crtc,
>> state);
>>  		if (ret)
>>  			return ret;
> Can't this chunk be on its own separate commit? I'm not sure why the
> rest of the commit is related to this change.
>
> It seems the rest of the commit is aimed reducing the number of planes
> we have to lock, not about not computing WMs if nothing in the pipe has
> changed.
>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 8effb9ece21e..144597ac74e3 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1583,6 +1583,19 @@ intel_atomic_get_crtc_state(struct
>> drm_atomic_state *state,
>>  
>>  	return to_intel_crtc_state(crtc_state);
>>  }
>> +
>> +static inline struct intel_plane_state *
>> +intel_atomic_get_existing_plane_state(struct drm_atomic_state
>> *state,
>> +				      struct intel_plane *plane)
>> +{
>> +	struct drm_plane_state *plane_state;
>> +
>> +	plane_state = drm_atomic_get_existing_plane_state(state,
>> &plane->base);
>> +
>> +	return to_intel_plane_state(plane_state);
>> +}
>> +
>> +
> Two newlines above.
>
> It seems you'll be able to simplify a lot of stuff with this new
> function. I'm looking forward to review that patch :)
>
>
>>  int intel_atomic_setup_scalers(struct drm_device *dev,
>>  	struct intel_crtc *intel_crtc,
>>  	struct intel_crtc_state *crtc_state);
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index 379eabe093cb..8fb8c6891ae6 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2010,11 +2010,18 @@ static void ilk_compute_wm_level(const struct
>> drm_i915_private *dev_priv,
>>  		cur_latency *= 5;
>>  	}
>>  
>> -	result->pri_val = ilk_compute_pri_wm(cstate, pristate,
>> -					     pri_latency, level);
>> -	result->spr_val = ilk_compute_spr_wm(cstate, sprstate,
>> spr_latency);
>> -	result->cur_val = ilk_compute_cur_wm(cstate, curstate,
>> cur_latency);
>> -	result->fbc_val = ilk_compute_fbc_wm(cstate, pristate,
>> result->pri_val);
>> +	if (pristate) {
>> +		result->pri_val = ilk_compute_pri_wm(cstate,
>> pristate,
>> +						     pri_latency,
>> level);
>> +		result->fbc_val = ilk_compute_fbc_wm(cstate,
>> pristate, result->pri_val);
>> +	}
>> +
>> +	if (sprstate)
>> +		result->spr_val = ilk_compute_spr_wm(cstate,
>> sprstate, spr_latency);
>> +
>> +	if (curstate)
>> +		result->cur_val = ilk_compute_cur_wm(cstate,
>> curstate, cur_latency);
>> +
>>  	result->enable = true;
>>  }
>>  
>> @@ -2287,7 +2294,6 @@ static int ilk_compute_pipe_wm(struct
>> intel_crtc *intel_crtc,
>>  	const struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_crtc_state *cstate = NULL;
>>  	struct intel_plane *intel_plane;
>> -	struct drm_plane_state *ps;
>>  	struct intel_plane_state *pristate = NULL;
>>  	struct intel_plane_state *sprstate = NULL;
>>  	struct intel_plane_state *curstate = NULL;
>> @@ -2306,30 +2312,37 @@ static int ilk_compute_pipe_wm(struct
>> intel_crtc *intel_crtc,
>>  	memset(pipe_wm, 0, sizeof(*pipe_wm));
>>  
>>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>> -		ps = drm_atomic_get_plane_state(state,
>> -						&intel_plane->base);
>> -		if (IS_ERR(ps))
>> -			return PTR_ERR(ps);
>> +		struct intel_plane_state *ps;
>> +
>> +		ps = intel_atomic_get_existing_plane_state(state,
>> +							   intel_pla
>> ne);
>> +		if (!ps)
>> +			continue;
> Ok, let me see if I understood: if some of these planes is not part of
> the atomic commit, you're not going to include it in the calculations
> since its value is not going to change. This would allow us lock less
> planes, which is the main advantage. Is this correct?
>
> If yes, how much do we really gain by saving some plane locking?
>
> So by not assigning values to result->x_val at ilk_compute_wm_level()
> when NULL is passed you're going to preserve whatever correct values
> were already set earlier, so you don't need to recompute them. Is this
> correct?
>
> If the answer to the above is "yes", then don't we need to remove the
> memset(pipe_wm, 0) at the beginning of ilk_compute_wm_level? Otherwise,
> nothing will be preserved.
>
> There's also the zero-initialization of "config" to consider.
>
> Or maybe instead of all of the above, we're working under the
> assumption that if some of the planes is not part of the atomic commit,
> then all its relevant WM values will be zero?
>
>>  
>>  		if (intel_plane->base.type ==
>> DRM_PLANE_TYPE_PRIMARY)
>> -			pristate = to_intel_plane_state(ps);
>> -		else if (intel_plane->base.type ==
>> DRM_PLANE_TYPE_OVERLAY)
>> -			sprstate = to_intel_plane_state(ps);
>> -		else if (intel_plane->base.type ==
>> DRM_PLANE_TYPE_CURSOR)
>> -			curstate = to_intel_plane_state(ps);
>> +			pristate = ps;
>> +		else if (intel_plane->base.type ==
>> DRM_PLANE_TYPE_OVERLAY) {
>> +			sprstate = ps;
>> +
>> +			if (ps) {
>> +				pipe_wm->sprites_enabled = sprstate-
>>> visible;
>> +				pipe_wm->sprites_scaled = sprstate-
>>> visible &&
> You're setting these values here...
>
>> +					(drm_rect_width(&sprstate-
>>> dst) != drm_rect_width(&sprstate->src) >> 16 ||
>> +					drm_rect_height(&sprstate-
>>> dst) != drm_rect_height(&sprstate->src) >> 16);
>> +			}
>> +		} else if (intel_plane->base.type ==
>> DRM_PLANE_TYPE_CURSOR)
> (missing brackets here, but if you follow my suggestion below you won't
> need them)
>
>> +			curstate = ps;
>>  	}
>>  
>> -	config.sprites_enabled = sprstate->visible;
>> -	config.sprites_scaled = sprstate->visible &&
>> -		(drm_rect_width(&sprstate->dst) !=
>> drm_rect_width(&sprstate->src) >> 16 ||
>> -		drm_rect_height(&sprstate->dst) !=
>> drm_rect_height(&sprstate->src) >> 16);
>> +	config.sprites_enabled = pipe_wm->sprites_enabled;
>> +	config.sprites_scaled = pipe_wm->sprites_scaled;
>>  
>>  	pipe_wm->pipe_enabled = cstate->base.active;
>>  	pipe_wm->sprites_enabled = config.sprites_enabled;
>>  	pipe_wm->sprites_scaled = config.sprites_scaled;
> ...but then we re-set them here.
>
> Either you could remove these assignments here, or you could move the
> "if (ps)" from the loop to outside it, becoming "if (sprstate)", making
> the post-patch code similar to the pre-patch code. Since both pipe_wm
> and config are zero-initialized you don't even need to think about the
> !sprstate case.
Makes sense, will do so.
>>  
>>  	/* ILK/SNB: LP2+ watermarks only w/o sprites */
>> -	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
>> +	if (INTEL_INFO(dev)->gen <= 6 && pipe_wm->sprites_enabled)
>>  		max_level = 1;
>>  
>>  	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
>> @@ -2352,20 +2365,18 @@ static int ilk_compute_pipe_wm(struct
>> intel_crtc *intel_crtc,
>>  	ilk_compute_wm_reg_maximums(dev, 1, &max);
>>  
>>  	for (level = 1; level <= max_level; level++) {
>> -		struct intel_wm_level wm = {};
>> +		struct intel_wm_level *wm = &pipe_wm->wm[level];
>>  
>>  		ilk_compute_wm_level(dev_priv, intel_crtc, level,
>> cstate,
>> -				     pristate, sprstate, curstate,
>> &wm);
>> +				     pristate, sprstate, curstate,
>> wm);
>>  
>>  		/*
>>  		 * Disable any watermark level that exceeds the
>>  		 * register maximums since such watermarks are
>>  		 * always invalid.
>>  		 */
>> -		if (!ilk_validate_wm_level(level, &max, &wm))
>> +		if (!ilk_validate_wm_level(level, &max, wm))
>>  			break;
>> -
>> -		pipe_wm->wm[level] = wm;
> The change in the chunk above is that we're now leaving with non-zero
> wm->{pri,spr,cur}_val if some level is invalid. Given the current
> complexity of the code, it's not trivial verify whether nothing later
> is assuming that invalid levels are all-zero, but it looks like we're
> safe. Anyway, could you please move this to a separate patch that comes
> before the other changes? It seems this change would be safe alone, and
> we're seeing problems with WMs these days, so having nice bisectability
> is a huge plus.
>
Well caught, I think we need to calculate even invalid values, but set ->enable = false in that case.
That is the only way we can ensure that the wm levels are calculated correctly.
I've sent those as separate patches, so I get a full CI run from them.

[PATCH 1/2] drm/i915: Allow preservation of watermarks.
[PATCH 2/2] drm/i915: Only recalculate wm's for planes part of the state, v2.