Re: [Intel-gfx] [PATCH v3 2/5] drm/i915: Force recalculate min_cdclk if planes config changed

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

On Mon, Mar 30, 2020 at 03:23:51PM +0300, Stanislav Lisovskiy wrote:
> In Gen11+ whenever we might exceed DBuf bandwidth we might need to
> recalculate CDCLK which DBuf bandwidth is scaled with.
> Total Dbuf bw used might change based on particular plane needs.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 17d83f37f49f..9fd32d61ebfe 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14623,7 +14623,7 @@ static bool active_planes_affects_min_cdclk(struct drm_i915_private *dev_priv)
>  	/* See {hsw,vlv,ivb}_plane_ratio() */
>  	return IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv) ||
>  		IS_CHERRYVIEW(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
> -		IS_IVYBRIDGE(dev_priv);
> +		IS_IVYBRIDGE(dev_priv) || (INTEL_GEN(dev_priv) >= 11);
>  }
>  
>  static int intel_atomic_check_planes(struct intel_atomic_state *state,
> @@ -14669,7 +14669,13 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state,
>  		old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
>  		new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
>  
> -		if (hweight8(old_active_planes) == hweight8(new_active_planes))
> +		/*
> +		 * Not only the number of planes, but if the plane configuration had
> +		 * changed might already mean we need to recompute min CDCLK,
> +		 * because different planes might consume different amount of Dbuf bandwidth
> +		 * according to formula: Bw per plane = Pixel rate * bpp * pipe/plane scale factor
> +		 */

The set of of active planes doesn't dictate the bandwidth since it
doesn't consider most of the parameters you listed above. Ie. doesn't
seem to be the right place for this stuff.

The decision to bump the cdclk should really come from the dbuf code
not from some totally distinct piece of code. But to get this right
I have a feeling we'll need the dbuf state and totally decouple cdclk
from any_ms.

> +		if (old_active_planes == new_active_planes)
>  			continue;
>  
>  		ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes);
> -- 
> 2.24.1.485.gad05a3d8e5

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