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@xxxxxxxxx>
>
>
> >
> > 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@xxxxxxxxxxxxxxx>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > Cc: Nathan Ciobanu <nathan.d.ciobanu@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
