Re: [Intel-gfx] [PATCH 03/10] drm/i915: Remove weird code from intel_atomic_check_bigjoiner()

["Navare, Manasi" <manasi.d.navare@intel.com>]

On Thu, Feb 03, 2022 at 08:38:16PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> There's some weird junk in intel_atomic_check_bigjoiner()
> that's trying to look at the old crtc state's bigjoiner
> usage for some reason. That code is totally unnecessary,
> and maybe even actively harmful. Not entirely sure which
> since it's such a mess that I can't actually wrap my brain
> around what it ends up doing.
> 
> Either way, thanks to intel_bigjoiner_add_affected_crtcs()
> all of the old bigjoiner crtcs are guaranteed to be in the
> state already if any one of them is in the state. Also if
> any one of those crtcs got flagged for a modeset, then all
> of them will have been flagged, and the bigjoiner links
> will have been detached via kill_bigjoiner_slave().
> 
> So there is no need to look examing any old bigjoiner
> usage in intel_atomic_check_bigjoiner(). All we have to care
> about is whether bigjoiner is needed for the new state,
> and whether we can get the slave crtc we need.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Completely agree with this cleanup, makes it so much easier to add new future code

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 33 +++++++-------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 2006eec6e166..b5701ca57889 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7584,38 +7584,28 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state,
>  }
>  
>  static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state,
> -					struct intel_crtc *crtc,
> -					struct intel_crtc_state *old_crtc_state,
> -					struct intel_crtc_state *new_crtc_state)
> +					struct intel_crtc *master_crtc)
>  {
>  	struct drm_i915_private *i915 = to_i915(state->base.dev);
> -	struct intel_crtc_state *slave_crtc_state, *master_crtc_state;
> -	struct intel_crtc *slave_crtc, *master_crtc;
> +	struct intel_crtc_state *master_crtc_state =
> +		intel_atomic_get_new_crtc_state(state, master_crtc);
> +	struct intel_crtc_state *slave_crtc_state;
> +	struct intel_crtc *slave_crtc;
>  
> -	/* slave being enabled, is master is still claiming this crtc? */
> -	if (old_crtc_state->bigjoiner_slave) {
> -		slave_crtc = crtc;
> -		master_crtc = old_crtc_state->bigjoiner_linked_crtc;
> -		master_crtc_state = intel_atomic_get_new_crtc_state(state, master_crtc);
> -		if (!master_crtc_state || !intel_crtc_needs_modeset(master_crtc_state))
> -			goto claimed;
> -	}
> -
> -	if (!new_crtc_state->bigjoiner)
> +	if (!master_crtc_state->bigjoiner)
>  		return 0;
>  
> -	slave_crtc = intel_dsc_get_bigjoiner_secondary(crtc);
> +	slave_crtc = intel_dsc_get_bigjoiner_secondary(master_crtc);
>  	if (!slave_crtc) {
>  		drm_dbg_kms(&i915->drm,
>  			    "[CRTC:%d:%s] Big joiner configuration requires "
>  			    "CRTC + 1 to be used, doesn't exist\n",
> -			    crtc->base.base.id, crtc->base.name);
> +			    master_crtc->base.base.id, master_crtc->base.name);
>  		return -EINVAL;
>  	}
>  
> -	new_crtc_state->bigjoiner_linked_crtc = slave_crtc;
> +	master_crtc_state->bigjoiner_linked_crtc = slave_crtc;
>  	slave_crtc_state = intel_atomic_get_crtc_state(&state->base, slave_crtc);
> -	master_crtc = crtc;
>  	if (IS_ERR(slave_crtc_state))
>  		return PTR_ERR(slave_crtc_state);
>  
> @@ -7627,7 +7617,7 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state,
>  		    "[CRTC:%d:%s] Used as slave for big joiner\n",
>  		    slave_crtc->base.base.id, slave_crtc->base.name);
>  
> -	return copy_bigjoiner_crtc_state(slave_crtc_state, new_crtc_state);
> +	return copy_bigjoiner_crtc_state(slave_crtc_state, master_crtc_state);
>  
>  claimed:
>  	drm_dbg_kms(&i915->drm,
> @@ -7899,8 +7889,7 @@ static int intel_atomic_check(struct drm_device *dev,
>  		if (ret)
>  			goto fail;
>  
> -		ret = intel_atomic_check_bigjoiner(state, crtc, old_crtc_state,
> -						   new_crtc_state);
> +		ret = intel_atomic_check_bigjoiner(state, crtc);
>  		if (ret)
>  			goto fail;
>  	}
> -- 
> 2.34.1
>