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

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

On Thu, Jul 19, 2018 at 11:51:40AM -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2018-07-18 at 22:43 -0700, Rodrigo Vivi wrote:
> > 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...
> 
> But there is more payload update code in intel_mst_enable_dp(),

oh... the part2 indeed...

> that
> would get executed regardless of this diff below. We'll have to rewrite
> pre_enable, enable, disable and post_disable if the idea is avoid sink
> transactions after the first failure. It does make sense to do all of
> that as it avoids printing error messages in dmesg when we very well
> know the branch device is disconnected, but this should be a separate
> change.

fair enough...

> My idea was to bring pre_enable/enable in line with
> disable/post_disable.

makes sense... I just saw it is similar on payload update failure.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> 
> > 
> > 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.
> It is clearing the ACT status bit by writing a 1. Bspec has this
> under DP_TP_STATUS:24
> 
> -DK
> 
> > 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));
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx