Re: [PATCH 2/3] drm/i915/bigjoiner: Refactor bigjoiner state readout

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

On Mon, Jan 08, 2024 at 02:07:24PM +0200, Stanislav Lisovskiy wrote:
> Don't call enabled_bigjoiner_pipes twice, lets just move
> intel_get_bigjoiner_config earlier, because it is anyway
> calling same function.
> Also cleanup hsw_enabled_transcoders from irrelevant bigjoiner code.

It's not irrelevant. The function is supposed to return the set of
enabled transcoders associated with the pipe. With this change the
function no longer does what it says on the tin.

> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 22 ++++++++++----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 927d124457b61..eec76ec167069 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -3525,7 +3525,6 @@ static u8 hsw_enabled_transcoders(struct intel_crtc *crtc)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	u8 panel_transcoder_mask = hsw_panel_transcoders(dev_priv);
>  	enum transcoder cpu_transcoder;
> -	u8 master_pipes, slave_pipes;
>  	u8 enabled_transcoders = 0;
>  
>  	/*
> @@ -3576,15 +3575,6 @@ static u8 hsw_enabled_transcoders(struct intel_crtc *crtc)
>  	if (transcoder_ddi_func_is_enabled(dev_priv, cpu_transcoder))
>  		enabled_transcoders |= BIT(cpu_transcoder);
>  
> -	/* bigjoiner slave -> consider the master pipe's transcoder as well */
> -	enabled_bigjoiner_pipes(dev_priv, &master_pipes, &slave_pipes);
> -	if (slave_pipes & BIT(crtc->pipe)) {
> -		cpu_transcoder = (enum transcoder)
> -			get_bigjoiner_master_pipe(crtc->pipe, master_pipes, slave_pipes);
> -		if (transcoder_ddi_func_is_enabled(dev_priv, cpu_transcoder))
> -			enabled_transcoders |= BIT(cpu_transcoder);
> -	}
> -
>  	return enabled_transcoders;
>  }
>  
> @@ -3631,6 +3621,15 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc,
>  	u32 tmp;
>  
>  	enabled_transcoders = hsw_enabled_transcoders(crtc);
> +
> +	/* bigjoiner slave -> consider the master pipe's transcoder as well */
> +	if (intel_crtc_is_bigjoiner_slave(pipe_config)) {
> +		unsigned long cpu_transcoder = (enum transcoder)
> +			bigjoiner_master_pipe(pipe_config);
> +		if (transcoder_ddi_func_is_enabled(dev_priv, cpu_transcoder))
> +			enabled_transcoders |= BIT(cpu_transcoder);
> +	}
> +
>  	if (!enabled_transcoders)
>  		return false;
>  
> @@ -3735,6 +3734,8 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
>  
>  	pipe_config->shared_dpll = NULL;
>  
> +	intel_bigjoiner_get_config(pipe_config);

So this is what avoids the "call it twice" part, but it also makes the
state potentially inconsistent as in all other cases we leave everything
zeroed if the transcoder is not enabled. So I'm not sure this is
entirely safe or whether we could end up with some weird state
mismatches due to the inconsistency.

Why do you think calling it twice is problematic? It's supposed to be 
idempotent (ignoring the actual register reads/etc.).

> +
>  	active = hsw_get_transcoder_state(crtc, pipe_config, &crtc->hw_readout_power_domains);
>  
>  	if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
> @@ -3746,7 +3747,6 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
>  	if (!active)
>  		goto out;
>  
> -	intel_bigjoiner_get_config(pipe_config);
>  	intel_dsc_get_config(pipe_config);
>  
>  	if (!transcoder_is_dsi(pipe_config->cpu_transcoder) ||
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel