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(), 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. My idea was to bring pre_enable/enable in line with
disable/post_disable.
>
> 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
