Re: [Intel-gfx] [PATCH 01/11] drm/i915: Check pipe active state in {planes, vrr}_{enabling, disabling}()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 06 Nov 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> {planes,vrr}_{enabling,disabling}() are supposed to indicate
> whether the specific hardware feature is supposed to be enabling
> or disabling. That can only makes sense if the pipe is active
> overall. So check for that before we go poking at the hardware.
>
> I think we're semi-safe currently on due to:
> - intel_pre_plane_update() doesn't get called when the pipe
>   was not-active prior to the commit, but this is actually a bug.
>   This saves vrr_disabling(), and vrr_enabling() is called from
>   deeper down where we have already checked hw.active.
> - active_planes mirrors the crtc's hw.active
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 9e9c03287869..f24c410cbd8f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -902,12 +902,18 @@ static bool needs_async_flip_vtd_wa(const struct intel_crtc_state *crtc_state)
>  static bool planes_enabling(const struct intel_crtc_state *old_crtc_state,
>  			    const struct intel_crtc_state *new_crtc_state)
>  {
> +	if (!new_crtc_state->hw.active)
> +		return false;
> +
>  	return is_enabling(active_planes, old_crtc_state, new_crtc_state);
>  }
>  
>  static bool planes_disabling(const struct intel_crtc_state *old_crtc_state,
>  			     const struct intel_crtc_state *new_crtc_state)
>  {
> +	if (!old_crtc_state->hw.active)
> +		return false;
> +
>  	return is_disabling(active_planes, old_crtc_state, new_crtc_state);
>  }
>  
> @@ -924,6 +930,9 @@ static bool vrr_params_changed(const struct intel_crtc_state *old_crtc_state,
>  static bool vrr_enabling(const struct intel_crtc_state *old_crtc_state,
>  			 const struct intel_crtc_state *new_crtc_state)
>  {
> +	if (!new_crtc_state->hw.active)
> +		return false;
> +
>  	return is_enabling(vrr.enable, old_crtc_state, new_crtc_state) ||
>  		(new_crtc_state->vrr.enable &&
>  		 (new_crtc_state->update_m_n || new_crtc_state->update_lrr ||
> @@ -933,6 +942,9 @@ static bool vrr_enabling(const struct intel_crtc_state *old_crtc_state,
>  static bool vrr_disabling(const struct intel_crtc_state *old_crtc_state,
>  			  const struct intel_crtc_state *new_crtc_state)
>  {
> +	if (!old_crtc_state->hw.active)
> +		return false;
> +
>  	return is_disabling(vrr.enable, old_crtc_state, new_crtc_state) ||
>  		(old_crtc_state->vrr.enable &&
>  		 (new_crtc_state->update_m_n || new_crtc_state->update_lrr ||

-- 
Jani Nikula, Intel