Re: [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization

From: Daniel Vetter <daniel@ffwll.ch>
To: Matt Roper <matthew.d.roper@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization
Date: Mon, 13 Jun 2016 16:49:49 +0200	[thread overview]
Message-ID: <20160613144949.GF1338@phenom.ffwll.local> (raw)
In-Reply-To: <1465510495-30302-2-git-send-email-matthew.d.roper@intel.com>

On Thu, Jun 09, 2016 at 03:14:53PM -0700, Matt Roper wrote:
> intel_state->active_crtcs is usually only initialized when doing a
> modeset.  During our first atomic commit after boot, we're effectively
> faking a modeset to sanitize the DDB/wm setup, so ensure that this field
> gets initialized before use.
> 
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 658a756..0cd38ca 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3896,9 +3896,18 @@ skl_compute_ddb(struct drm_atomic_state *state)
>  	 * pretend that all pipes switched active status so that we'll
>  	 * ensure a full DDB recompute.
>  	 */
> -	if (dev_priv->wm.distrust_bios_wm)
> +	if (dev_priv->wm.distrust_bios_wm) {
>  		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.
> +		 */
> +		intel_state->active_crtcs = dev_priv->active_crtcs;
> +	}

Can't we move input sanitization out of this? Imo mixing up atomic
check/compute config code with hw state restoring is way too fragile.

Also, why exactly do we have active_crtcs? Seems to just be duplicated
state that can get out of sync, we have too many of those already ...
-Daniel

> +
>  	/*
>  	 * If the modeset changes which CRTC's are active, we need to
>  	 * recompute the DDB allocation for *all* active pipes, even
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx