Re: [PATCH 5/6] drm/i915: Sanitize MBUS joining

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 31 Oct 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> If the system boots with MBUS joining enabled but we disable
> the relevant pipe during sanitaion we later get into trouble

*sanitation

> as the rest of the code doesn't expect MBUS joining to be
> enabled unless the set of active pipes is in agreement.
>
> We could relax some of the MBUS joining related checks during
> normal atomic commits to let this slide, but that might also
> let some real bugs through. So let's sanitize the MBUS joining
> instead. And in order to keep things more or less in sync we'll
> do the related credit, cdclk/mdclk ratio, etc. updates as well.

Okay, I'm a bit lacking on confidence with this part of the driver, but
it looks sensible. Fingers crossed.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

And the same nitpick about i915 usage.

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/skl_watermark.c | 29 ++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 2eefeff6693a..98f9e01b2a1c 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -3567,6 +3567,29 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
>  	gen9_dbuf_slices_update(i915, new_slices);
>  }
>  
> +static void skl_mbus_sanitize(struct drm_i915_private *i915)
> +{
> +	struct intel_dbuf_state *dbuf_state =
> +		to_intel_dbuf_state(i915->display.dbuf.obj.state);
> +
> +	if (!HAS_MBUS_JOINING(i915))
> +		return;
> +
> +	if (!dbuf_state->joined_mbus ||
> +	    adlp_check_mbus_joined(dbuf_state->active_pipes))
> +		return;
> +
> +	drm_dbg_kms(&i915->drm, "Disabling redundant MBUS joining (active pipes 0x%x)\n",
> +		    dbuf_state->active_pipes);
> +
> +	dbuf_state->joined_mbus = false;
> +	intel_dbuf_mdclk_cdclk_ratio_update(i915,
> +					    dbuf_state->mdclk_cdclk_ratio,
> +					    dbuf_state->joined_mbus);
> +	pipe_mbus_dbox_ctl_update(i915, dbuf_state);
> +	mbus_ctl_join_update(i915, dbuf_state, INVALID_PIPE);
> +}
> +
>  static bool skl_dbuf_is_misconfigured(struct drm_i915_private *i915)
>  {
>  	const struct intel_dbuf_state *dbuf_state =
> @@ -3599,7 +3622,7 @@ static bool skl_dbuf_is_misconfigured(struct drm_i915_private *i915)
>  	return false;
>  }
>  
> -static void skl_wm_sanitize(struct drm_i915_private *i915)
> +static void skl_dbuf_sanitize(struct drm_i915_private *i915)
>  {
>  	struct intel_crtc *crtc;
>  
> @@ -3638,7 +3661,9 @@ static void skl_wm_sanitize(struct drm_i915_private *i915)
>  static void skl_wm_get_hw_state_and_sanitize(struct drm_i915_private *i915)
>  {
>  	skl_wm_get_hw_state(i915);
> -	skl_wm_sanitize(i915);
> +
> +	skl_mbus_sanitize(i915);
> +	skl_dbuf_sanitize(i915);
>  }
>  
>  void intel_wm_state_verify(struct intel_atomic_state *state,

-- 
Jani Nikula, Intel