Re: [PATCH 03/19] drm/i915: Don't clobber crtc_state->cpu_transcoder for inactive crtcs

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

On Tue, 2025-02-18 at 23:18 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Inactive crtcs are supposed to have their crtc_state completely
> cleared. Currently we are clobbering crtc_state->cpu_transcoder
> before determining whether it's actually enabled or not. Don't
> do that.
> 
> I want to rework the inherited flag handling for inactive crtcs
> a bit, and having a bogus cpu_transcoder in the crtc state can
> then cause confusing fastset mismatches even when the crtc never
> changes state during the commit.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 28 +++++++++-----------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 

Reviewed-by: Vinod Govindapillai <vinod.govindapillai@intel.com>

> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 8ee8c1df075a..75b184c933e7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -3109,25 +3109,24 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  	struct intel_display *display = to_intel_display(crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	enum intel_display_power_domain power_domain;
> +	enum transcoder cpu_transcoder = (enum transcoder)crtc->pipe;
>  	intel_wakeref_t wakeref;
> +	bool ret = false;
>  	u32 tmp;
> -	bool ret;
>  
>  	power_domain = POWER_DOMAIN_PIPE(crtc->pipe);
>  	wakeref = intel_display_power_get_if_enabled(display, power_domain);
>  	if (!wakeref)
>  		return false;
>  
> +	tmp = intel_de_read(dev_priv, TRANSCONF(dev_priv, cpu_transcoder));
> +	if (!(tmp & TRANSCONF_ENABLE))
> +		goto out;
> +
> +	pipe_config->cpu_transcoder = cpu_transcoder;
> +
>  	pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
>  	pipe_config->sink_format = pipe_config->output_format;
> -	pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
> -
> -	ret = false;
> -
> -	tmp = intel_de_read(dev_priv,
> -			    TRANSCONF(dev_priv, pipe_config->cpu_transcoder));
> -	if (!(tmp & TRANSCONF_ENABLE))
> -		goto out;
>  
>  	if (IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
>  	    IS_CHERRYVIEW(dev_priv)) {
> @@ -3492,23 +3491,22 @@ static bool ilk_get_pipe_config(struct intel_crtc *crtc,
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	enum intel_display_power_domain power_domain;
> +	enum transcoder cpu_transcoder = (enum transcoder)crtc->pipe;
>  	intel_wakeref_t wakeref;
> +	bool ret = false;
>  	u32 tmp;
> -	bool ret;
>  
>  	power_domain = POWER_DOMAIN_PIPE(crtc->pipe);
>  	wakeref = intel_display_power_get_if_enabled(display, power_domain);
>  	if (!wakeref)
>  		return false;
>  
> -	pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
> -
> -	ret = false;
> -	tmp = intel_de_read(dev_priv,
> -			    TRANSCONF(dev_priv, pipe_config->cpu_transcoder));
> +	tmp = intel_de_read(dev_priv, TRANSCONF(dev_priv, cpu_transcoder));
>  	if (!(tmp & TRANSCONF_ENABLE))
>  		goto out;
>  
> +	pipe_config->cpu_transcoder = cpu_transcoder;
> +
>  	switch (tmp & TRANSCONF_BPC_MASK) {
>  	case TRANSCONF_BPC_6:
>  		pipe_config->pipe_bpp = 18;