Re: [Intel-gfx] [PATCH v3] drm/i915: Fix inconsistance between pfit.enable and scaler freeing

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

On Fri, Jan 24, 2020 at 07:23:01PM +0200, Stanislav Lisovskiy wrote:
> Despite that during hw readout we seem to have scalers assigned
> to pipes, then call atomic_setup_scalers, at the commit stage in
> skl_update_scaler there is a check, that if we have fb src and
> dest of same size, we stage freeing of that scaler.
> 
> However we don't update pfit.enabled flag then, which makes
> the state inconsistent, which in turn triggers a WARN_ON
> in skl_pfit_enable, because we have pfit enabled,
> but no assigned scaler.

And the reason for not having updates pfit.enabled is that the
the modeset was forced by a cdclk change and thus the full state
recomputation never happened and we're left with the inherited
pfit.enabled.

> 
> To me this looks weird that we kind of do the decision
> to use or not use the scaler at skl_update_scaler stage
> but not in intel_atomic_setup_scalers, moreover
> not updating the whole state consistently.
> 
> This fix is to not free the scaler if we have pfit.enabled
> flag set, so that the state is now consistent
> and the warnings are gone.
> 
> v2: - Put pfit.enable check into crtc specific place
>       (Ville Syrjälä)
> 
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/577

Closes: ...


> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 5768cfcf71c4..cd242d91a924 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6037,7 +6037,8 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
>  	const struct drm_display_mode *adjusted_mode = &state->hw.adjusted_mode;
>  	bool need_scaler = false;
>  
> -	if (state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> +	if (state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
> +	    state->pch_pfit.enabled)

Hmm, no hw.enable check here for the existing case either. Shouldn't
matter now that I made the crtc disable clear the entire state as well.
Oh, it was handled via that odd force_detach stuff it seems. Whatever.

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

>  		need_scaler = true;
>  
>  	return skl_update_scaler(state, !state->hw.active, SKL_CRTC_INDEX,
> -- 
> 2.24.1.485.gad05a3d8e5

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx