Re: [Intel-gfx] [PATCH 3/6] drm/i915: Use pipes instead crtc indices in PLL state tracking

["Kahola, Mika" <mika.kahola@intel.com>]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Wednesday, February 24, 2021 4:42 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 3/6] drm/i915: Use pipes instead crtc indices in PLL
> state tracking
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> All the other places we have use pipes instead of crtc indices when tracking
> resource usage. Life is easier when we do it the same way always, so switch
> the dpll mgr to using pipes as well. Looks like it was actually mixing these up
> in some cases so it would not even have worked correctly except when the
> device has a contiguous set of pipes starting from pipe A.
> Granted, that is the typical case but supposedly it may not always hold on
> modern hw.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c  | 40 ++++++++--------
> .../drm/i915/display/intel_display_debugfs.c  |  4 +-
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 48 ++++++++++---------
> drivers/gpu/drm/i915/display/intel_dpll_mgr.h |  8 ++--
>  4 files changed, 51 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index b34620545d3b..958c2a796bae 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -9653,7 +9653,7 @@ verify_single_dpll_state(struct drm_i915_private
> *dev_priv,
>  			 struct intel_crtc_state *new_crtc_state)  {
>  	struct intel_dpll_hw_state dpll_hw_state;
> -	unsigned int crtc_mask;
> +	u8 pipe_mask;
>  	bool active;
> 
>  	memset(&dpll_hw_state, 0, sizeof(dpll_hw_state)); @@ -9666,34
> +9666,34 @@ verify_single_dpll_state(struct drm_i915_private *dev_priv,
>  		I915_STATE_WARN(!pll->on && pll->active_mask,
>  		     "pll in active use but not on in sw tracking\n");
>  		I915_STATE_WARN(pll->on && !pll->active_mask,
> -		     "pll is on but not used by any active crtc\n");
> +		     "pll is on but not used by any active pipe\n");
>  		I915_STATE_WARN(pll->on != active,
>  		     "pll on state mismatch (expected %i, found %i)\n",
>  		     pll->on, active);
>  	}
> 
>  	if (!crtc) {
> -		I915_STATE_WARN(pll->active_mask & ~pll->state.crtc_mask,
> -				"more active pll users than references: %x vs
> %x\n",
> -				pll->active_mask, pll->state.crtc_mask);
> +		I915_STATE_WARN(pll->active_mask & ~pll-
> >state.pipe_mask,
> +				"more active pll users than references: 0x%x
> vs 0x%x\n",
> +				pll->active_mask, pll->state.pipe_mask);
> 
>  		return;
>  	}
> 
> -	crtc_mask = drm_crtc_mask(&crtc->base);
> +	pipe_mask = BIT(crtc->pipe);
> 
>  	if (new_crtc_state->hw.active)
> -		I915_STATE_WARN(!(pll->active_mask & crtc_mask),
> -				"pll active mismatch (expected pipe %c in
> active mask 0x%02x)\n",
> +		I915_STATE_WARN(!(pll->active_mask & pipe_mask),
> +				"pll active mismatch (expected pipe %c in
> active mask 0x%x)\n",
>  				pipe_name(crtc->pipe), pll->active_mask);
>  	else
> -		I915_STATE_WARN(pll->active_mask & crtc_mask,
> -				"pll active mismatch (didn't expect pipe %c in
> active mask 0x%02x)\n",
> +		I915_STATE_WARN(pll->active_mask & pipe_mask,
> +				"pll active mismatch (didn't expect pipe %c in
> active mask
> +0x%x)\n",
>  				pipe_name(crtc->pipe), pll->active_mask);
> 
> -	I915_STATE_WARN(!(pll->state.crtc_mask & crtc_mask),
> -			"pll enabled crtcs mismatch (expected 0x%x in
> 0x%02x)\n",
> -			crtc_mask, pll->state.crtc_mask);
> +	I915_STATE_WARN(!(pll->state.pipe_mask & pipe_mask),
> +			"pll enabled crtcs mismatch (expected 0x%x in
> 0x%x)\n",
> +			pipe_mask, pll->state.pipe_mask);
> 
>  	I915_STATE_WARN(pll->on && memcmp(&pll->state.hw_state,
>  					  &dpll_hw_state,
> @@ -9713,15 +9713,15 @@ verify_shared_dpll_state(struct intel_crtc *crtc,
> 
>  	if (old_crtc_state->shared_dpll &&
>  	    old_crtc_state->shared_dpll != new_crtc_state->shared_dpll) {
> -		unsigned int crtc_mask = drm_crtc_mask(&crtc->base);
> +		u8 pipe_mask = BIT(crtc->pipe);
>  		struct intel_shared_dpll *pll = old_crtc_state->shared_dpll;
> 
> -		I915_STATE_WARN(pll->active_mask & crtc_mask,
> -				"pll active mismatch (didn't expect pipe %c in
> active mask)\n",
> -				pipe_name(crtc->pipe));
> -		I915_STATE_WARN(pll->state.crtc_mask & crtc_mask,
> -				"pll enabled crtcs mismatch (found %x in
> enabled mask)\n",
> -				pipe_name(crtc->pipe));
> +		I915_STATE_WARN(pll->active_mask & pipe_mask,
> +				"pll active mismatch (didn't expect pipe %c in
> active mask (0x%x))\n",
> +				pipe_name(crtc->pipe), pll->active_mask);
> +		I915_STATE_WARN(pll->state.pipe_mask & pipe_mask,
> +				"pll enabled crtcs mismatch (found %x in
> enabled mask (0x%x))\n",
> +				pipe_name(crtc->pipe), pll-
> >state.pipe_mask);
>  	}
>  }
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 35f176ea8280..20194ccfec05 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -1094,8 +1094,8 @@ static int i915_shared_dplls_info(struct seq_file
> *m, void *unused)
> 
>  		seq_printf(m, "DPLL%i: %s, id: %i\n", i, pll->info->name,
>  			   pll->info->id);
> -		seq_printf(m, " crtc_mask: 0x%08x, active: 0x%x, on: %s\n",
> -			   pll->state.crtc_mask, pll->active_mask, yesno(pll-
> >on));
> +		seq_printf(m, " pipe_mask: 0x%x, active: 0x%x, on: %s\n",
> +			   pll->state.pipe_mask, pll->active_mask, yesno(pll-
> >on));
>  		seq_printf(m, " tracked hardware state:\n");
>  		seq_printf(m, " dpll:    0x%08x\n", pll->state.hw_state.dpll);
>  		seq_printf(m, " dpll_md: 0x%08x\n",
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index 529b1d569af2..a68ae90b07e3 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -176,7 +176,7 @@ void intel_prepare_shared_dpll(const struct
> intel_crtc_state *crtc_state)
>  		return;
> 
>  	mutex_lock(&dev_priv->dpll.lock);
> -	drm_WARN_ON(&dev_priv->drm, !pll->state.crtc_mask);
> +	drm_WARN_ON(&dev_priv->drm, !pll->state.pipe_mask);
>  	if (!pll->active_mask) {
>  		drm_dbg(&dev_priv->drm, "setting up %s\n", pll->info-
> >name);
>  		drm_WARN_ON(&dev_priv->drm, pll->on); @@ -198,7
> +198,7 @@ void intel_enable_shared_dpll(const struct intel_crtc_state
> *crtc_state)
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	struct intel_shared_dpll *pll = crtc_state->shared_dpll;
> -	unsigned int crtc_mask = drm_crtc_mask(&crtc->base);
> +	unsigned int pipe_mask = BIT(crtc->pipe);
>  	unsigned int old_mask;
> 
>  	if (drm_WARN_ON(&dev_priv->drm, pll == NULL)) @@ -207,16
> +207,16 @@ void intel_enable_shared_dpll(const struct intel_crtc_state
> *crtc_state)
>  	mutex_lock(&dev_priv->dpll.lock);
>  	old_mask = pll->active_mask;
> 
> -	if (drm_WARN_ON(&dev_priv->drm, !(pll->state.crtc_mask &
> crtc_mask)) ||
> -	    drm_WARN_ON(&dev_priv->drm, pll->active_mask & crtc_mask))
> +	if (drm_WARN_ON(&dev_priv->drm, !(pll->state.pipe_mask &
> pipe_mask)) ||
> +	    drm_WARN_ON(&dev_priv->drm, pll->active_mask & pipe_mask))
>  		goto out;
> 
> -	pll->active_mask |= crtc_mask;
> +	pll->active_mask |= pipe_mask;
> 
>  	drm_dbg_kms(&dev_priv->drm,
> -		    "enable %s (active %x, on? %d) for crtc %d\n",
> +		    "enable %s (active 0x%x, on? %d) for [CRTC:%d:%s]\n",
>  		    pll->info->name, pll->active_mask, pll->on,
> -		    crtc->base.base.id);
> +		    crtc->base.base.id, crtc->base.name);
> 
>  	if (old_mask) {
>  		drm_WARN_ON(&dev_priv->drm, !pll->on); @@ -244,7
> +244,7 @@ void intel_disable_shared_dpll(const struct intel_crtc_state
> *crtc_state)
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	struct intel_shared_dpll *pll = crtc_state->shared_dpll;
> -	unsigned int crtc_mask = drm_crtc_mask(&crtc->base);
> +	unsigned int pipe_mask = BIT(crtc->pipe);
> 
>  	/* PCH only available on ILK+ */
>  	if (INTEL_GEN(dev_priv) < 5)
> @@ -254,18 +254,20 @@ void intel_disable_shared_dpll(const struct
> intel_crtc_state *crtc_state)
>  		return;
> 
>  	mutex_lock(&dev_priv->dpll.lock);
> -	if (drm_WARN_ON(&dev_priv->drm, !(pll->active_mask &
> crtc_mask)))
> +	if (drm_WARN(&dev_priv->drm, !(pll->active_mask & pipe_mask),
> +		     "%s not used by [CRTC:%d:%s]\n", pll->info->name,
> +		     crtc->base.base.id, crtc->base.name))
>  		goto out;
> 
>  	drm_dbg_kms(&dev_priv->drm,
> -		    "disable %s (active %x, on? %d) for crtc %d\n",
> +		    "disable %s (active 0x%x, on? %d) for [CRTC:%d:%s]\n",
>  		    pll->info->name, pll->active_mask, pll->on,
> -		    crtc->base.base.id);
> +		    crtc->base.base.id, crtc->base.name);
> 
>  	assert_shared_dpll_enabled(dev_priv, pll);
>  	drm_WARN_ON(&dev_priv->drm, !pll->on);
> 
> -	pll->active_mask &= ~crtc_mask;
> +	pll->active_mask &= ~pipe_mask;
>  	if (pll->active_mask)
>  		goto out;
> 
> @@ -296,7 +298,7 @@ intel_find_shared_dpll(struct intel_atomic_state
> *state,
>  		pll = &dev_priv->dpll.shared_dplls[i];
> 
>  		/* Only want to check enabled timings first */
> -		if (shared_dpll[i].crtc_mask == 0) {
> +		if (shared_dpll[i].pipe_mask == 0) {
>  			if (!unused_pll)
>  				unused_pll = pll;
>  			continue;
> @@ -306,10 +308,10 @@ intel_find_shared_dpll(struct intel_atomic_state
> *state,
>  			   &shared_dpll[i].hw_state,
>  			   sizeof(*pll_state)) == 0) {
>  			drm_dbg_kms(&dev_priv->drm,
> -				    "[CRTC:%d:%s] sharing existing %s (crtc
> mask 0x%08x, active %x)\n",
> +				    "[CRTC:%d:%s] sharing existing %s (pipe
> mask 0x%x, active
> +0x%x)\n",
>  				    crtc->base.base.id, crtc->base.name,
>  				    pll->info->name,
> -				    shared_dpll[i].crtc_mask,
> +				    shared_dpll[i].pipe_mask,
>  				    pll->active_mask);
>  			return pll;
>  		}
> @@ -338,13 +340,13 @@ intel_reference_shared_dpll(struct
> intel_atomic_state *state,
> 
>  	shared_dpll = intel_atomic_get_shared_dpll_state(&state->base);
> 
> -	if (shared_dpll[id].crtc_mask == 0)
> +	if (shared_dpll[id].pipe_mask == 0)
>  		shared_dpll[id].hw_state = *pll_state;
> 
>  	drm_dbg(&i915->drm, "using %s for pipe %c\n", pll->info->name,
>  		pipe_name(crtc->pipe));
> 
> -	shared_dpll[id].crtc_mask |= 1 << crtc->pipe;
> +	shared_dpll[id].pipe_mask |= BIT(crtc->pipe);
>  }
> 
>  static void intel_unreference_shared_dpll(struct intel_atomic_state *state,
> @@ -354,7 +356,7 @@ static void intel_unreference_shared_dpll(struct
> intel_atomic_state *state,
>  	struct intel_shared_dpll_state *shared_dpll;
> 
>  	shared_dpll = intel_atomic_get_shared_dpll_state(&state->base);
> -	shared_dpll[pll->info->id].crtc_mask &= ~(1 << crtc->pipe);
> +	shared_dpll[pll->info->id].pipe_mask &= ~BIT(crtc->pipe);
>  }
> 
>  static void intel_put_dpll(struct intel_atomic_state *state, @@ -4597,19
> +4599,19 @@ static void readout_dpll_hw_state(struct drm_i915_private
> *i915,
> 
> POWER_DOMAIN_DPLL_DC_OFF);
>  	}
> 
> -	pll->state.crtc_mask = 0;
> +	pll->state.pipe_mask = 0;
>  	for_each_intel_crtc(&i915->drm, crtc) {
>  		struct intel_crtc_state *crtc_state =
>  			to_intel_crtc_state(crtc->base.state);
> 
>  		if (crtc_state->hw.active && crtc_state->shared_dpll == pll)
> -			pll->state.crtc_mask |= 1 << crtc->pipe;
> +			pll->state.pipe_mask |= BIT(crtc->pipe);
>  	}
> -	pll->active_mask = pll->state.crtc_mask;
> +	pll->active_mask = pll->state.pipe_mask;
> 
>  	drm_dbg_kms(&i915->drm,
> -		    "%s hw state readout: crtc_mask 0x%08x, on %i\n",
> -		    pll->info->name, pll->state.crtc_mask, pll->on);
> +		    "%s hw state readout: pipe_mask 0x%x, on %i\n",
> +		    pll->info->name, pll->state.pipe_mask, pll->on);
>  }
> 
>  void intel_dpll_readout_hw_state(struct drm_i915_private *i915) diff --git
> a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> index 2eb7618ef957..eb52e85022e2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> @@ -241,9 +241,9 @@ struct intel_dpll_hw_state {
>   */
>  struct intel_shared_dpll_state {
>  	/**
> -	 * @crtc_mask: mask of CRTC using this DPLL, active or not
> +	 * @pipe_mask: mask of pipes using this DPLL, active or not
>  	 */
> -	unsigned crtc_mask;
> +	u8 pipe_mask;
> 
>  	/**
>  	 * @hw_state: hardware configuration for the DPLL stored in @@ -
> 351,9 +351,9 @@ struct intel_shared_dpll {
>  	struct intel_shared_dpll_state state;
> 
>  	/**
> -	 * @active_mask: mask of active CRTCs (i.e. DPMS on) using this DPLL
> +	 * @active_mask: mask of active pipes (i.e. DPMS on) using this DPLL
>  	 */
> -	unsigned active_mask;
> +	u8 active_mask;
> 
>  	/**
>  	 * @on: is the PLL actually active? Disabled during modeset
> --
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx