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

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

On Fri, Dec 20, 2019 at 07:29:53AM -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()
> 
> v5:
> - fixed and moved code to check if MST master needs a modeset
> 
> 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>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 70 ++++++++++++++++----
>  1 file 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 24841dde490b..11f2c13ec23e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13909,19 +13909,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,
> @@ -14032,6 +14019,20 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
>  	return 0;
>  }
>  
> +static bool intel_cpu_transcoder_needs_modeset(struct intel_atomic_state *state,
> +					       enum transcoder transcoder)
> +{
> +	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->cpu_transcoder == transcoder)
> +			return needs_modeset(new_crtc_state);
> +
> +	return false;
> +}

I would highly recommend splitting this patch into two before pushing
so that the check vs. copy split goes in first, then the MST thing on
top. That way if we have to revert either MST or port sync then at
least we keep this part and won't break the other feature in the
process.

> +
>  /**
>   * intel_atomic_check - validate state object
>   * @dev: drm device
> @@ -14089,6 +14090,49 @@ static int intel_atomic_check(struct drm_device *dev,
>  			any_ms = true;
>  	}

Was going to say this any_ms thing has to move, but I guess not really
since the below loop won't do anything unless we still have at least
one crtc needing a modeset. Probably should move it, but can be done
later.

>  
> +	/**
> +	 * 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 (!new_crtc_state->uapi.enable ||

These uapi.enable checks seem fishy. Should at least be hw.enable since
we're past .compute_config() stage already.

> +		    !intel_dp_mst_is_slave_trans(new_crtc_state) ||
> +		    needs_modeset(new_crtc_state))
> +			continue;
> +
> +		if (intel_cpu_transcoder_needs_modeset(state, master)) {
> +			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 (!new_crtc_state->uapi.enable ||
> +		    needs_modeset(new_crtc_state) ||
> +		    !new_crtc_state->update_pipe)
> +			continue;
> +
> +		/*
> +		 * 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 sad.

> +	}
> +
>  	if (any_ms && !check_digital_port_conflicts(state)) {
>  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
>  		ret = EINVAL;
> -- 
> 2.24.1

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