On Wed, 2017-09-20 at 13:02 -0700, Ausmus, James wrote:
> On Wed, Sep 20, 2017 at 12:55 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@xxxxxxxxx> wrote:
> > On Wed, 2017-09-20 at 12:11 -0700, Ausmus, James wrote:
> >> On Mon, Sep 18, 2017 at 3:21 PM, Dhinakaran Pandiyan
> >> <dhinakaran.pandiyan@xxxxxxxxx> wrote:
> >> > Rewriting this code without the goto, I believe, makes it more readable.
> >> > One functional change that has been included is the handling of failed ESI
> >> > register reads. Instead of disabling MST only for the first failed read, we
> >> > now disable MST on subsequent failed reads too. A failed ESI read is
> >> > problematic irrespective of whether it is the first or not.
> >> >
> >> > Cc: James Ausmus <james.ausmus@xxxxxxxxx>
> >> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> >> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> >> > ---
> >> > drivers/gpu/drm/i915/intel_dp.c | 75 +++++++++++++++++------------------------
> >> > 1 file changed, 31 insertions(+), 44 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> > index 98e7b96ca826..cc129aa444ac 100644
> >> > --- a/drivers/gpu/drm/i915/intel_dp.c
> >> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> > @@ -4191,57 +4191,44 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> >> > static int
> >> > intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >> > {
> >> > - bool bret;
> >> > + u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> >> > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> >> >
> >> > - if (intel_dp->is_mst) {
> >> > - u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> >> > - int ret = 0;
> >> > - int retry;
> >> > + if (!intel_dp->is_mst)
> >> > + return -EINVAL;
> >> > +
> >> > + while (intel_dp_get_sink_irq_esi(intel_dp, esi)) {
> >>
> >> It looks like if the underlying drm_dp_dpcd_read fails and returns
> >> -EIO, for instance, you'll get true back from
> >> intel_dp_get_sink_irq_esi,
> >
> > Wait, anything other than 14 from that dpcd read is a false, isn't it?
>
> D'oh! You're right - I completely glossed over the whole " ==
> DP_DPRX_ESI_LEN" bit - sorry for the noise...
>
> >
> >> and you'll still go in to the while, but
> >> with a potentially invalid esi. Granted, this is a problem in the
> >> original code as well, but it seems like something that should be
> >> fixed during the refactoring.
> >>
> >>
> >> > + int ret, retry;
> >> > bool handled;
> >> > - bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> >> > -go_again:
> >> > - if (bret == true) {
> >> > -
> >> > - /* check link status - esi[10] = 0x200c */
> >> > - if (intel_dp->active_mst_links &&
> >> > - !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> >> > - DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> >> > - intel_dp_start_link_train(intel_dp);
> >> > - intel_dp_stop_link_train(intel_dp);
> >> > - }
> >> >
> >> > - DRM_DEBUG_KMS("got esi %3ph\n", esi);
> >> > - ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
> >> > -
> >> > - if (handled) {
> >> > - for (retry = 0; retry < 3; retry++) {
> >> > - int wret;
> >> > - wret = drm_dp_dpcd_write(&intel_dp->aux,
> >> > - DP_SINK_COUNT_ESI+1,
> >> > - &esi[1], 3);
> >> > - if (wret == 3) {
> >> > - break;
> >> > - }
> >> > - }
> >> > + DRM_DEBUG_KMS("ESI %3ph\n", esi);
> >> >
> >> > - bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> >> > - if (bret == true) {
> >> > - DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
> >> > - goto go_again;
> >> > - }
> >> > - } else
> >> > - ret = 0;
> >> > + /* check link status - esi[10] = 0x200c */
> >> > + if (intel_dp->active_mst_links &&
> >> > + !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> >> > + intel_dp_start_link_train(intel_dp);
> >> > + intel_dp_stop_link_train(intel_dp);
> >> > + }
> >> >
> >> > - return ret;
> >> > - } else {
> >> > - struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> >> > - DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
> >> > - intel_dp->is_mst = false;
> >> > - drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> >> > - /* send a hotplug event */
> >> > - drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
> >> > + ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
> >>
> >> You're no longer using the value returned by drm_dp_mst_hpd_irq
> >
> > The way the code was originally written, the return from
> > drm_dp_mst_hpd_irq() was
> > a) changed to 0 when handled == false
> > b) discarded and a new return value was obtained if handled == true and
> > intel_dp_get_sink_irq_esi() is true the second time.
> >
> >
> > So the only case when the return value was returned back to the caller
> > is when handled == true and the second intel_dp_get_sink_irq_esi()
> > returned false.
> >
> > But this does not make sense. If the second intel_dp_get_sink_irq_esi()
> > is false, then we should still have to disable MST. This is the
> > functional change I noted in the commit message.
> >
>
> Certainly, but you aren't actually using ret for anything anymore, so
> the variable can be dropped
>
>
> >
> >>
> >> > + if (!handled)
> >> > + return 0;
> >> > +
if (ret)
DRM_DEBUG_KMS("MST irq_hpd handling failed %d\n", ret);
I was thinking of adding something like this, but then the drm helper
functions _handle_down_rep() and _handle_up_req() only ever return 0. I
also don't want to get rid of 'ret' because that may make it seem like
the underlying helpers don't return anything. So, we should either
change the helpers to return something useful or modify their
signatures. Both options need a bit more thought.
For now, I guess we could add just the debug message. Let me know what
you think.
-DK
> >> > + for (retry = 0; retry < 3; retry++) {
> >> > + int wret;
> >> > +
> >> > + wret = drm_dp_dpcd_write(&intel_dp->aux,
> >> > + DP_SINK_COUNT_ESI + 1, &esi[1],
> >> > + 3);
> >> > + if (wret == 3)
> >> > + break;
> >> > }
> >> > }
> >> > +
> >> > + DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
> >> > + intel_dp->is_mst = false;
> >> > + drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> >> > + drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
> >> > return -EINVAL;
> >> > }
> >> >
> >> > --
> >> > 2.11.0
> >> >
> >>
> >>
> >>
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
