Re: [Intel-gfx] [PATCH v2 06/22] drm/i915: Add helper to modeset a set of pipes

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

On Thu, Aug 24, 2023 at 11:05:01AM +0300, Imre Deak wrote:
> Add intel_modeset_pipes_in_mask() to modeset a provided set of pipes,
> used in a follow-up patch.
> 
> While at it add DocBook descriptions for the exported functions.
> 
> v2:
> - Add a flag controlling if active planes are force updated as well.
> - Add DockBook descriptions.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 43 ++++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_display.h |  3 ++
>  2 files changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index db3c26e013e3b..a1956b89fd75d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5420,8 +5420,24 @@ intel_verify_planes(struct intel_atomic_state *state)
>  			     plane_state->uapi.visible);
>  }
>  
> -int intel_modeset_all_pipes(struct intel_atomic_state *state,
> -			    const char *reason)
> +/**
> + * intel_modeset_pipes_in_mask - force a full modeset on a set of pipes
> + * @state: intel atomic state
> + * @reason: the reason for the full modeset
> + * @mask: mask of pipes to modeset
> + * @update_active_planes: force updating all active planes
> + *
> + * Force a full modeset on CRTCs in @mask due to the description in @reason.
> + * Also force updating all active planes in each modeset CRTC if
> + * @update_active_planes is %true. This flag must be set to %true if the
> + * function is called after new plane states are computed already and
> + * set to %false otherwise.
> + *
> + * Returns 0 in case of success, negative error code otherwise.
> + */
> +int intel_modeset_pipes_in_mask(struct intel_atomic_state *state,
> +				const char *reason, u8 mask,
> +				bool update_active_planes)

Not really a fan of this parametrized behaviour. Also pretty sure we
have several other places that trigger modesets early in the atomic
check and all those just hand roll currently. So if we want a helper
then it might make more sense to try to combine all those early cases
into a new function. We could rename the current thing _late() or
something to make it clearer when it should be used.

>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>  	struct intel_crtc *crtc;
> @@ -5430,7 +5446,7 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state,
>  	 * Add all pipes to the state, and force
>  	 * a modeset on all the active ones.
>  	 */
> -	for_each_intel_crtc(&dev_priv->drm, crtc) {
> +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, crtc, mask) {
>  		struct intel_crtc_state *crtc_state;
>  		int ret;
>  
> @@ -5461,7 +5477,9 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state,
>  		if (ret)
>  			return ret;
>  
> -		crtc_state->update_planes |= crtc_state->active_planes;
> +		if (update_active_planes)
> +			crtc_state->update_planes |= crtc_state->active_planes;
> +
>  		crtc_state->async_flip_planes = 0;
>  		crtc_state->do_async_flip = false;
>  	}
> @@ -5469,6 +5487,23 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state,
>  	return 0;
>  }
>  
> +/**
> + * intel_modeset_all_pipes - force a full modeset on all pipes
> + * @state: intel atomic state
> + * @reason: the reason for the full modeset
> + *
> + * Force a full modeset on all CRTCs due to the description in @reason.
> + * This function can be called only after new plane states are computed
> + * already.
> + *
> + * Returns 0 in case of success, negative error code otherwise.
> + */
> +int intel_modeset_all_pipes(struct intel_atomic_state *state,
> +			    const char *reason)
> +{
> +	return intel_modeset_pipes_in_mask(state, reason, -1, true);
> +}
> +
>  /*
>   * This implements the workaround described in the "notes" section of the mode
>   * set sequence documentation. When going from no pipes or single pipe to
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index 49ac8473b988b..d9a54610d9d5e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -515,6 +515,9 @@ void intel_update_watermarks(struct drm_i915_private *i915);
>  /* modesetting */
>  int intel_modeset_all_pipes(struct intel_atomic_state *state,
>  			    const char *reason);
> +int intel_modeset_pipes_in_mask(struct intel_atomic_state *state,
> +				const char *reason, u8 pipe_mask,
> +				bool update_active_planes);
>  void intel_modeset_get_crtc_power_domains(struct intel_crtc_state *crtc_state,
>  					  struct intel_power_domain_mask *old_domains);
>  void intel_modeset_put_crtc_power_domains(struct intel_crtc *crtc,
> -- 
> 2.37.2

-- 
Ville Syrjälä
Intel