Re: [PATCH 08/15] drm/i915: Add bigjoiner and uncompressed joiner hw readout sanity checks

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

On Wed, Sep 18, 2024 at 08:13:36PM +0530, Ankit Nautiyal wrote:
> From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> 
> Add sanity checks for primary and secondary bigjoiner/uncompressed
> bitmasks, should make it easier to spot possible issues.
> 
> v2:
> -Streamline the expected masks and add few more drm_WARNs. (Ville)
> -Use %#x format specifier for printing joiner masks. (Ville)
> -Use struct intel_display instead of struct drm_i915_private. (Ankit)
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com> (v1)
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 49 +++++++++++++++++++-
>  1 file changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 28447d4c8934..a17e89fb5eb9 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -3654,26 +3654,71 @@ static void enabled_bigjoiner_pipes(struct intel_display *display,
>  	}
>  }
>  
> +static u8 expected_secondary_pipes(u8 primary_pipes, int num_pipes)
> +{
> +	u8 secondary_pipes = 0;
> +
> +	for (int i = 1; i < num_pipes; i++)
> +		secondary_pipes |= primary_pipes << i;
> +
> +	return secondary_pipes;
> +}
> +
> +static u8 expected_uncompjoiner_secondary_pipes(u8 uncompjoiner_primary_pipes)

You use the full "uncompressed_joiner" everywhere else, so should
probably stick to that here as well for consistency.

> +{
> +	return expected_secondary_pipes(uncompjoiner_primary_pipes, 2);
> +}
> +
> +static u8 expected_bigjoiner_secondary_pipes(u8 bigjoiner_primary_pipes)
> +{
> +	return expected_secondary_pipes(bigjoiner_primary_pipes, 2);
> +}
> +
>  static void enabled_joiner_pipes(struct drm_i915_private *dev_priv,
>  				 u8 *primary_pipes, u8 *secondary_pipes)
>  {
>  	struct intel_display *display = to_intel_display(&dev_priv->drm);
>  	u8 primary_uncompressed_joiner_pipes, primary_bigjoiner_pipes;
>  	u8 secondary_uncompressed_joiner_pipes, secondary_bigjoiner_pipes;
> +	u8 uncompressed_joiner_pipes, bigjoiner_pipes;
>  
>  	enabled_uncompressed_joiner_pipes(display, &primary_uncompressed_joiner_pipes,
>  					  &secondary_uncompressed_joiner_pipes);
>  
> +	drm_WARN_ON(display->drm,
> +		    (primary_uncompressed_joiner_pipes & secondary_uncompressed_joiner_pipes) != 0);
> +
>  	enabled_bigjoiner_pipes(display, &primary_bigjoiner_pipes,
>  				&secondary_bigjoiner_pipes);
>  
> +	drm_WARN_ON(display->drm,
> +		    (primary_bigjoiner_pipes & secondary_bigjoiner_pipes) != 0);
> +
> +	uncompressed_joiner_pipes = primary_uncompressed_joiner_pipes |
> +				    secondary_uncompressed_joiner_pipes;
> +	bigjoiner_pipes = primary_bigjoiner_pipes | secondary_bigjoiner_pipes;
> +
> +	drm_WARN(display->drm, (uncompressed_joiner_pipes & bigjoiner_pipes) != 0,
> +		 "Uncomressed joiner pipes(%#x) and bigjoiner pipes(%#x) can't intersect\n",
> +		 uncompressed_joiner_pipes, bigjoiner_pipes);

Maybe add an empty line between all the WARNs because this is starting
to look pretty busy othwerwise.

Otherwise lgtm
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +	drm_WARN(display->drm, secondary_bigjoiner_pipes !=
> +		 expected_bigjoiner_secondary_pipes(primary_bigjoiner_pipes),
> +		 "Wrong secondary bigjoiner pipes(expected %#x, current %#x)\n",
> +		 expected_bigjoiner_secondary_pipes(primary_bigjoiner_pipes),
> +		 secondary_bigjoiner_pipes);
> +	drm_WARN(display->drm, secondary_uncompressed_joiner_pipes !=
> +		 expected_uncompjoiner_secondary_pipes(primary_uncompressed_joiner_pipes),
> +		 "Wrong secondary uncompressed joiner pipes(expected %#x, current %#x)\n",
> +		 expected_uncompjoiner_secondary_pipes(primary_uncompressed_joiner_pipes),
> +		 secondary_uncompressed_joiner_pipes);
> +
>  	*primary_pipes = primary_uncompressed_joiner_pipes | primary_bigjoiner_pipes;
>  
>  	*secondary_pipes = secondary_uncompressed_joiner_pipes | secondary_bigjoiner_pipes;
>  
>  	/* Joiner pipes should always be consecutive primary and secondary */
> -	drm_WARN(&dev_priv->drm, *secondary_pipes != *primary_pipes << 1,
> -		 "Joiner misconfigured (primary pipes 0x%x, secondary pipes 0x%x)\n",
> +	drm_WARN(display->drm, *secondary_pipes != *primary_pipes << 1,
> +		 "Joiner misconfigured (primary pipes %#x, secondary pipes %#x)\n",
>  		 *primary_pipes, *secondary_pipes);
>  }
>  
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel