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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx