| Message ID | 20180718171943.3246-2-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > 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> Reviewed-by: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com> Tested-by: Nathan Ciobanu <nathan.d.ciobanu@linux.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 >
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
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@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));
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));
On Thu, 2018-07-19 at 16:37 -0700, Rodrigo Vivi wrote: > 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> Thanks for the review, I will push this with Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107281
On Wed, 2018-07-25 at 11:39 -0700, Dhinakaran Pandiyan wrote: > On Thu, 2018-07-19 at 16:37 -0700, Rodrigo Vivi wrote: > > > > 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> > Thanks for the review, I will push this with > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107281 Pushed after checking the logs of failures - none of them were related to the patch series.
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));
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. 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(-)