Re: [PATCH v3 05/11] drm/i915/gen9+: Do not initialise active_crtcs for !modeset

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Nov 08, 2016 at 01:55:36PM +0100, Maarten Lankhorst wrote:
> This is a hack and not needed. Use the right mask by checking
> intel_state->modeset. This works for watermark sanitization too.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 38 +++++++++++++++-----------------------
>  1 file changed, 15 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 02f52b52a03d..d38a46efcfed 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3089,26 +3089,22 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_crtc *for_crtc = cstate->base.crtc;
> -	unsigned int pipe_size, ddb_size;
> +	unsigned int pipe_size, ddb_size, active_crtcs;
>  	int nth_active_pipe;
>  
> +	if (intel_state->modeset)
> +		active_crtcs = intel_state->active_crtcs;
> +	else
> +		active_crtcs = dev_priv->active_crtcs;

What's the story with the locking here?

> +
> +	*num_active = hweight32(active_crtcs);
> +
>  	if (WARN_ON(!state) || !cstate->base.active) {
>  		alloc->start = 0;
>  		alloc->end = 0;
> -		*num_active = hweight32(dev_priv->active_crtcs);
>  		return;
>  	}
>  
> -	if (intel_state->active_pipe_changes)
> -		*num_active = hweight32(intel_state->active_crtcs);
> -	else
> -		*num_active = hweight32(dev_priv->active_crtcs);
> -
> -	ddb_size = INTEL_INFO(dev_priv)->ddb_size;
> -	WARN_ON(ddb_size == 0);
> -
> -	ddb_size -= 4; /* 4 blocks for bypass path allocation */
> -
>  	/*
>  	 * If the state doesn't change the active CRTC's, then there's
>  	 * no need to recalculate; the existing pipe allocation limits
> @@ -3126,9 +3122,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  		return;
>  	}
>  
> -	nth_active_pipe = hweight32(intel_state->active_crtcs &
> +	ddb_size = INTEL_INFO(dev_priv)->ddb_size;
> +	WARN_ON(ddb_size == 0);
> +
> +	ddb_size -= 4; /* 4 blocks for bypass path allocation */
> +
> +	nth_active_pipe = hweight32(active_crtcs &
>  				    (drm_crtc_mask(for_crtc) - 1));
> -	pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
> +	pipe_size = ddb_size / *num_active;
>  	alloc->start = nth_active_pipe * ddb_size / *num_active;
>  	alloc->end = alloc->start + pipe_size;
>  }
> @@ -4021,15 +4022,6 @@ skl_compute_ddb(struct drm_atomic_state *state)
>  			return ret;
>  
>  		intel_state->active_pipe_changes = ~0;
> -
> -		/*
> -		 * We usually only initialize intel_state->active_crtcs if we
> -		 * we're doing a modeset; make sure this field is always
> -		 * initialized during the sanitization process that happens
> -		 * on the first commit too.
> -		 */
> -		if (!intel_state->modeset)
> -			intel_state->active_crtcs = dev_priv->active_crtcs;
>  	}
>  
>  	/*
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx