| Message ID | 1451998226-21433-4-git-send-email-shubhangi.shrivastava@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: > When created originally intel_dp_check_link_status() > was supposed to handle only link training for short > pulse but has grown into handler for short pulse itself. > This patch cleans up this function by splitting it into > two halves. First intel_dp_short_pulse() is called, > which will be entry point and handle all logic for > short pulse handling while intel_dp_check_link_status() > will retain its original purpose of only doing link > status related work. > The link retraining part when EQ is not correct is > retained to intel_dp_check_link_status whereas other > operations are handled as part of intel_dp_short_pulse. > This change is required to avoid performing all DPCD > related operations on performing link retraining. > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++---------------- > - > 1 file changed, 33 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 137757b..842790e 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4289,6 +4289,33 @@ go_again: > return -EINVAL; > } > > +static void > +intel_dp_check_link_status(struct intel_dp *intel_dp) > +{ > + struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp) > ->base; > + u8 link_status[DP_LINK_STATUS_SIZE]; > + > + if (!intel_dp_get_link_status(intel_dp, link_status)) { > + DRM_ERROR("Failed to get link status\n"); > + return; > + } > + > + if (!intel_encoder->base.crtc) > + return; > + > + if (!to_intel_crtc(intel_encoder->base.crtc)->active) > + return; > + > + /* if link training is requested we should perform it always */ > + if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) || > + (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) { > + DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", > + intel_encoder->base.name); > + intel_dp_start_link_train(intel_dp); > + intel_dp_stop_link_train(intel_dp); > + } > +} > + > /* > * According to DP spec > * 5.1.2: > @@ -4298,15 +4325,12 @@ go_again: > * 4. Check link status on receipt of hot-plug interrupt > */ > static void > -intel_dp_check_link_status(struct intel_dp *intel_dp) > +intel_dp_short_pulse(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > - struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp) > ->base; > u8 sink_irq_vector; > u8 link_status[DP_LINK_STATUS_SIZE]; > > - WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); > - I think it's better to move this WARN to the new intel_dp_check_link_status(). > /* > * Clearing compliance test variables to allow capturing > * of values for next automated test request. > @@ -4315,12 +4339,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) > intel_dp->compliance_test_type = 0; > intel_dp->compliance_test_data = 0; > > - if (!intel_encoder->base.crtc) > - return; > - > - if (!to_intel_crtc(intel_encoder->base.crtc)->active) > - return; > - > /* Try to read receiver status if the link appears to be up */ > if (!intel_dp_get_link_status(intel_dp, link_status)) { > return; There is now two calls to intel_dp_get_link_status()and the value of link_status is not used in this function, so maybe just remove it from here. Looks good otherwise. Ander > @@ -4345,14 +4363,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) > DRM_DEBUG_DRIVER("CP or sink specific irq > unhandled\n"); > } > > - /* if link training is requested we should perform it always */ > - if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) || > - (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) { > - DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", > - intel_encoder->base.name); > - intel_dp_start_link_train(intel_dp); > - intel_dp_stop_link_train(intel_dp); > - } > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > + intel_dp_check_link_status(intel_dp); > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > } > > /* XXX this is probably wrong for multiple downstream ports */ > @@ -5080,11 +5093,8 @@ intel_dp_hpd_pulse(struct intel_digital_port > *intel_dig_port, bool long_hpd) > } > } > > - if (!intel_dp->is_mst) { > - drm_modeset_lock(&dev->mode_config.connection_mutex, > NULL); > - intel_dp_check_link_status(intel_dp); > - drm_modeset_unlock(&dev > ->mode_config.connection_mutex); > - } > + if (!intel_dp->is_mst) > + intel_dp_short_pulse(intel_dp); > } > > ret = IRQ_HANDLED;
On Wednesday 13 January 2016 08:34 PM, Ander Conselvan De Oliveira wrote: > On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: >> When created originally intel_dp_check_link_status() >> was supposed to handle only link training for short >> pulse but has grown into handler for short pulse itself. >> This patch cleans up this function by splitting it into >> two halves. First intel_dp_short_pulse() is called, >> which will be entry point and handle all logic for >> short pulse handling while intel_dp_check_link_status() >> will retain its original purpose of only doing link >> status related work. >> The link retraining part when EQ is not correct is >> retained to intel_dp_check_link_status whereas other >> operations are handled as part of intel_dp_short_pulse. >> This change is required to avoid performing all DPCD >> related operations on performing link retraining. >> >> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> >> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> >> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++---------------- >> - >> 1 file changed, 33 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 137757b..842790e 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4289,6 +4289,33 @@ go_again: >> return -EINVAL; >> } >> >> +static void >> +intel_dp_check_link_status(struct intel_dp *intel_dp) >> +{ >> + struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp) >> ->base; >> + u8 link_status[DP_LINK_STATUS_SIZE]; >> + >> + if (!intel_dp_get_link_status(intel_dp, link_status)) { >> + DRM_ERROR("Failed to get link status\n"); >> + return; >> + } >> + >> + if (!intel_encoder->base.crtc) >> + return; >> + >> + if (!to_intel_crtc(intel_encoder->base.crtc)->active) >> + return; >> + >> + /* if link training is requested we should perform it always */ >> + if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) || >> + (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) { >> + DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", >> + intel_encoder->base.name); >> + intel_dp_start_link_train(intel_dp); >> + intel_dp_stop_link_train(intel_dp); >> + } >> +} >> + >> /* >> * According to DP spec >> * 5.1.2: >> @@ -4298,15 +4325,12 @@ go_again: >> * 4. Check link status on receipt of hot-plug interrupt >> */ >> static void >> -intel_dp_check_link_status(struct intel_dp *intel_dp) >> +intel_dp_short_pulse(struct intel_dp *intel_dp) >> { >> struct drm_device *dev = intel_dp_to_dev(intel_dp); >> - struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp) >> ->base; >> u8 sink_irq_vector; >> u8 link_status[DP_LINK_STATUS_SIZE]; >> >> - WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); >> - > I think it's better to move this WARN to the new intel_dp_check_link_status(). Sure.. Done.. >> /* >> * Clearing compliance test variables to allow capturing >> * of values for next automated test request. >> @@ -4315,12 +4339,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) >> intel_dp->compliance_test_type = 0; >> intel_dp->compliance_test_data = 0; >> >> - if (!intel_encoder->base.crtc) >> - return; >> - >> - if (!to_intel_crtc(intel_encoder->base.crtc)->active) >> - return; >> - >> /* Try to read receiver status if the link appears to be up */ >> if (!intel_dp_get_link_status(intel_dp, link_status)) { >> return; > There is now two calls to intel_dp_get_link_status()and the value of link_status > is not used in this function, so maybe just remove it from here. Looks good > otherwise. > > Ander Sure.. Done.. >> @@ -4345,14 +4363,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) >> DRM_DEBUG_DRIVER("CP or sink specific irq >> unhandled\n"); >> } >> >> - /* if link training is requested we should perform it always */ >> - if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) || >> - (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) { >> - DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", >> - intel_encoder->base.name); >> - intel_dp_start_link_train(intel_dp); >> - intel_dp_stop_link_train(intel_dp); >> - } >> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >> + intel_dp_check_link_status(intel_dp); >> + drm_modeset_unlock(&dev->mode_config.connection_mutex); >> } >> >> /* XXX this is probably wrong for multiple downstream ports */ >> @@ -5080,11 +5093,8 @@ intel_dp_hpd_pulse(struct intel_digital_port >> *intel_dig_port, bool long_hpd) >> } >> } >> >> - if (!intel_dp->is_mst) { >> - drm_modeset_lock(&dev->mode_config.connection_mutex, >> NULL); >> - intel_dp_check_link_status(intel_dp); >> - drm_modeset_unlock(&dev >> ->mode_config.connection_mutex); >> - } >> + if (!intel_dp->is_mst) >> + intel_dp_short_pulse(intel_dp); >> } >> >> ret = IRQ_HANDLED;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 137757b..842790e 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4289,6 +4289,33 @@ go_again: return -EINVAL; } +static void +intel_dp_check_link_status(struct intel_dp *intel_dp) +{ + struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; + u8 link_status[DP_LINK_STATUS_SIZE]; + + if (!intel_dp_get_link_status(intel_dp, link_status)) { + DRM_ERROR("Failed to get link status\n"); + return; + } + + if (!intel_encoder->base.crtc) + return; + + if (!to_intel_crtc(intel_encoder->base.crtc)->active) + return; + + /* if link training is requested we should perform it always */ + if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) || + (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) { + DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", + intel_encoder->base.name); + intel_dp_start_link_train(intel_dp); + intel_dp_stop_link_train(intel_dp); + } +} + /* * According to DP spec * 5.1.2: @@ -4298,15 +4325,12 @@ go_again: * 4. Check link status on receipt of hot-plug interrupt */ static void -intel_dp_check_link_status(struct intel_dp *intel_dp) +intel_dp_short_pulse(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); - struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; u8 sink_irq_vector; u8 link_status[DP_LINK_STATUS_SIZE]; - WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); - /* * Clearing compliance test variables to allow capturing * of values for next automated test request. @@ -4315,12 +4339,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) intel_dp->compliance_test_type = 0; intel_dp->compliance_test_data = 0; - if (!intel_encoder->base.crtc) - return; - - if (!to_intel_crtc(intel_encoder->base.crtc)->active) - return; - /* Try to read receiver status if the link appears to be up */ if (!intel_dp_get_link_status(intel_dp, link_status)) { return; @@ -4345,14 +4363,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); } - /* if link training is requested we should perform it always */ - if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) || - (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) { - DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", - intel_encoder->base.name); - intel_dp_start_link_train(intel_dp); - intel_dp_stop_link_train(intel_dp); - } + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); + intel_dp_check_link_status(intel_dp); + drm_modeset_unlock(&dev->mode_config.connection_mutex); } /* XXX this is probably wrong for multiple downstream ports */ @@ -5080,11 +5093,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) } } - if (!intel_dp->is_mst) { - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); - intel_dp_check_link_status(intel_dp); - drm_modeset_unlock(&dev->mode_config.connection_mutex); - } + if (!intel_dp->is_mst) + intel_dp_short_pulse(intel_dp); } ret = IRQ_HANDLED;