Re: [PATCH v2 2/2] drm/i915/mst: Continue state updates even if AUX writes fail.

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Jul 18, 2018 at 10:19:43AM -0700, Dhinakaran Pandiyan wrote:
> We are too late in the enabling sequence to back out cleanly, not updating
> state tracking variables, like intel_dp->active_mst_links in this
> instance, results in incorrect behaviour further along.

I agree with you, although I'm not fully convinced that we need to call the
update payload if vcpi allocation failed...

so imho this entire function should be reworked to be like this:

+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -217,7 +217,6 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
        enum port port = intel_dig_port->base.port;
        struct intel_connector *connector =
                to_intel_connector(conn_state->connector);
-       int ret;
        uint32_t temp;
 
        /* MST encoders are bound to a crtc, not to a connector,
@@ -233,25 +232,15 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 
        drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true);
 
-       if (intel_dp->active_mst_links == 0)
+       if (intel_dp->active_mst_links++ == 0)
                intel_dig_port->base.pre_enable(&intel_dig_port->base,
                                                pipe_config, NULL);
 
-       ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
-                                      connector->port,
-                                      pipe_config->pbn,
-                                      pipe_config->dp_m_n.tu);
-       if (ret == false) {
+       if (drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr, connector->port,
+                                    pipe_config->pbn, pipe_config->dp_m_n.tu))
+               drm_dp_update_payload_part1(&intel_dp->mst_mgr);
+       else
                DRM_ERROR("failed to allocate vcpi\n");
-               return;
-       }
-
-
-       intel_dp->active_mst_links++;
-       temp = I915_READ(DP_TP_STATUS(port));
-       I915_WRITE(DP_TP_STATUS(port), temp);
-
-       ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);


probably with
-       temp = I915_READ(DP_TP_STATUS(port));
-       I915_WRITE(DP_TP_STATUS(port), temp);

in a separated patch. No idea why we read this staus reg and write back.
I didn't see on spec any indication that this cleans it or that we need that
for cleaning or anything else.

> 
> v2: Fixed int v/s bool comparison
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 7e3e01607643..110e7ff22ef7 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -241,11 +241,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  				       connector->port,
>  				       pipe_config->pbn,
>  				       pipe_config->dp_m_n.tu);
> -	if (ret == false) {
> +	if (!ret)
>  		DRM_ERROR("failed to allocate vcpi\n");
> -		return;
> -	}
> -
>  
>  	intel_dp->active_mst_links++;
>  	temp = I915_READ(DP_TP_STATUS(port));
> -- 
> 2.17.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx