Re: [Intel-gfx] [PATCH v3 11/25] drm/i915/fdi: Recompute state for affected CRTCs on FDI links

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

On Thu, Sep 14, 2023 at 10:26:45PM +0300, Imre Deak wrote:
> Recompute the state of all CRTCs on an FDI link during a modeset that
> may be affected by the modeset of other CRTCs on the same link. This
> ensures that each CRTC on the link maximizes its BW use (after another
> CRTC is disabled).
> 
> In practice this means recomputing pipe B's config on IVB if pipe C gets
> disabled.
> 
> v2:
> - Add the change recomputing affected CRTC states in a separate patch.
>   (Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  4 ++
>  drivers/gpu/drm/i915/display/intel_fdi.c     | 53 ++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_fdi.h     |  1 +
>  3 files changed, 58 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index aad16dcceb788..31297a333f50e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6210,6 +6210,10 @@ int intel_atomic_check_config(struct intel_atomic_state *state,
>  	if (ret)
>  		return ret;
>  
> +	ret = intel_fdi_add_affected_crtcs(state);
> +	if (ret)
> +		return ret;
> +
>  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>  		if (!intel_crtc_needs_modeset(new_crtc_state)) {
>  			if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> diff --git a/drivers/gpu/drm/i915/display/intel_fdi.c b/drivers/gpu/drm/i915/display/intel_fdi.c
> index ad01915a4a39b..d723ae7e10d71 100644
> --- a/drivers/gpu/drm/i915/display/intel_fdi.c
> +++ b/drivers/gpu/drm/i915/display/intel_fdi.c
> @@ -120,6 +120,59 @@ void intel_fdi_link_train(struct intel_crtc *crtc,
>  	dev_priv->display.funcs.fdi->fdi_link_train(crtc, crtc_state);
>  }
>  
> +/**
> + * intel_fdi_add_affected_crtcs - add CRTCs on FDI affected by other modeset CRTCs
> + * @state: intel atomic state
> + *
> + * Add a CRTC using FDI to @state if changing another CRTC's FDI BW usage is
> + * known to affect the available FDI BW for the former CRTC. In practice this
> + * means adding CRTC B on IVYBRIDGE if its use of FDI lanes is limited (by
> + * CRTC C) and CRTC C is getting disabled.
> + *
> + * Returns 0 in case of success, or a negative error code otherwise.
> + */
> +int intel_fdi_add_affected_crtcs(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> +	struct intel_crtc_state *old_crtc_state;
> +	struct intel_crtc_state *new_crtc_state;

Both look like they can be const.

> +	struct intel_crtc *crtc;
> +
> +	if (!IS_IVYBRIDGE(i915))

Should also check num_pipes==3 since pipe C can (at least in theory)
be fused off.

> +		return 0;
> +
> +	crtc = intel_crtc_for_pipe(i915, PIPE_C);
> +	new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> +
> +	if (!new_crtc_state)
> +		return 0;
> +
> +	old_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);

old vs. new mixup

> +
> +	if (!old_crtc_state->fdi_lanes)
> +		return 0;
> +
> +	if (!intel_crtc_needs_modeset(new_crtc_state))
> +		return 0;
> +
> +	if (new_crtc_state->uapi.enable)
> +		return 0;

We could be switching pipe C from driving a PCH port to driving
the CPU eDP port. So this check seems a bit wrong.

> +
> +	crtc = intel_crtc_for_pipe(i915, PIPE_B);
> +	new_crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> +
> +	if (IS_ERR(new_crtc_state))
> +		return PTR_ERR(old_crtc_state);
> +
> +	old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
> +	if (!old_crtc_state->fdi_lanes)
> +		return 0;
> +
> +	return intel_modeset_pipes_in_mask_early(state,
> +						 "FDI link BW decrease on pipe C",
> +						 BIT(PIPE_B));
> +}
> +
>  /* units of 100MHz */
>  static int pipe_required_fdi_lanes(struct intel_crtc_state *crtc_state)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_fdi.h b/drivers/gpu/drm/i915/display/intel_fdi.h
> index 129444c580f27..eb02b967bb440 100644
> --- a/drivers/gpu/drm/i915/display/intel_fdi.h
> +++ b/drivers/gpu/drm/i915/display/intel_fdi.h
> @@ -14,6 +14,7 @@ struct intel_crtc_state;
>  struct intel_encoder;
>  struct intel_link_bw_limits;
>  
> +int intel_fdi_add_affected_crtcs(struct intel_atomic_state *state);
>  int intel_fdi_link_freq(struct drm_i915_private *i915,
>  			const struct intel_crtc_state *pipe_config);
>  int ilk_fdi_compute_config(struct intel_crtc *intel_crtc,
> -- 
> 2.37.2

-- 
Ville Syrjälä
Intel