Re: [Intel-gfx] [PATCH v4 5/6] drm/i915/display: Check if pipe fastset is allowed by external dependencies

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

On Wed, Dec 18, 2019 at 10:59:09AM -0800, José Roberto de Souza wrote:
> Check if fastset is allowed by external dependencies like other pipes
> and transcoders.
> 
> Right now this patch only forces a fullmodeset in MST slaves of MST
> masters that needs a fullmodeset but it will be needed for port sync
> as well.
> 
> v3:
> - moved handling to intel_atomic_check() this way is guarantee that
> all pipes will have its state computed
> 
> v4:
> - added a function to return if MST master neeeds modeset to simply
> code in intel_atomic_check()
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 53 +++++++++++++++-----
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 14 ++++++
>  drivers/gpu/drm/i915/display/intel_dp_mst.h  |  3 ++
>  3 files changed, 57 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index a4f252d05b37..2a406891567b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13903,19 +13903,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
>  
>  	new_crtc_state->uapi.mode_changed = false;
>  	new_crtc_state->update_pipe = true;
> -
> -	/*
> -	 * If we're not doing the full modeset we want to
> -	 * keep the current M/N values as they may be
> -	 * sufficiently different to the computed values
> -	 * to cause problems.
> -	 *
> -	 * FIXME: should really copy more fuzzy state here
> -	 */
> -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
>  }
>  
>  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> @@ -14083,6 +14070,46 @@ static int intel_atomic_check(struct drm_device *dev,
>  			any_ms = true;
>  	}
>  
> +	/**
> +	 * Check if fastset is allowed by external dependencies like other
> +	 * pipes and transcoders.
> +	 *
> +	 * Right now it only forces a fullmodeset when the MST master
> +	 * transcoder did not changed but the pipe of the master transcoder
> +	 * needs a fullmodeset so all slaves also needs to do a fullmodeset.
> +	 */
> +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		enum transcoder master = new_crtc_state->mst_master_transcoder;
> +
> +		if (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
> +		    needs_modeset(new_crtc_state))
> +			continue;
> +
> +		if (intel_dp_mst_master_trans_needs_modeset(state, master)) {

I think this has the loops the opposite way of what I was thinking,
but should work fine I think... OK. I'm convinced your way is in fact
better.

> +			new_crtc_state->uapi.mode_changed = true;
> +			new_crtc_state->update_pipe = false;
> +		}
> +	}
> +
> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +		if (needs_modeset(new_crtc_state))
> +			continue;

I suppose there isn't any way we should have crtcs in the state that
neither have update_pipe or needs_modeset flagged here. Could maybe
WARN_ON(!update_pipe) here if we're being paranoid.

> +
> +		/*
> +		 * If we're not doing the full modeset we want to
> +		 * keep the current M/N values as they may be
> +		 * sufficiently different to the computed values
> +		 * to cause problems.
> +		 *
> +		 * FIXME: should really copy more fuzzy state here
> +		 */
> +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;

Still a bit unhappy having this state copy inlined in intel_atomic_check().

> +	}
> +
>  	if (any_ms && !check_digital_port_conflicts(state)) {
>  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
>  		ret = EINVAL;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index efd14b0b507b..4aba1d702a83 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -854,3 +854,17 @@ bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
>  	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
>  	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
>  }
> +
> +bool intel_dp_mst_master_trans_needs_modeset(struct intel_atomic_state *state,
> +					     enum transcoder master)

Are we going to need this elsewhere? Or be static in intel_display.c?
Not that people are 100% happy of stuffing everything in there.

> +{
> +	struct intel_crtc_state *new_crtc_state;
> +	struct intel_crtc *crtc;
> +	int i;
> +
> +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
> +		if (new_crtc_state->mst_master_transcoder == master)

So we need to modeset everything when either the master or any
other slave wants to modeset? That is, we can't just modeset slaves
independently and only force the modesets for all slaves if the master
needs a modeset?

> +			return drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi);
> +
> +	return false;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> index 854724f68f09..72cb486f32ab 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> @@ -8,6 +8,7 @@
>  
>  #include <linux/types.h>
>  
> +struct intel_atomic_state;
>  struct intel_digital_port;
>  struct intel_crtc_state;
>  
> @@ -16,5 +17,7 @@ void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
>  int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port);
>  bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
>  bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
> +bool intel_dp_mst_master_trans_needs_modeset(struct intel_atomic_state *state,
> +					     enum transcoder master);
>  
>  #endif /* __INTEL_DP_MST_H__ */
> -- 
> 2.24.1

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