| Message ID | 1470954221-6474-1-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2016-08-11 at 15:23 -0700, Manasi Navare wrote: > Intel_dp_link_is_valid() function reads the Link status registers > and returns a boolean to indicate link is valid or not. > If the link has lost lock and is not valid any more, link > training is performed outside the function else previously trained link > is retained. > This gives us flexibility of checking whether link is valid and training > it independently. > > v2: > * Changed the function name from intel_dp_check_link_status() > to intel_dp_link_is_valid() (Lukas Wunner) > * Checks for CRTC and active CRTC are moved outside the > intel_dp_link_is_valid() function (Rodrigo Vivi) > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++++-------------- > 1 file changed, 37 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 364db90..891147d 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3881,36 +3881,33 @@ go_again: > return -EINVAL; > } > > -static void > -intel_dp_check_link_status(struct intel_dp *intel_dp) > +static bool > +intel_dp_link_is_valid(struct intel_dp *intel_dp) > { > - struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; > struct drm_device *dev = intel_dp_to_dev(intel_dp); > u8 link_status[DP_LINK_STATUS_SIZE]; > > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); > > if (!intel_dp_get_link_status(intel_dp, link_status)) { > - DRM_ERROR("Failed to get link status\n"); > - return; > + DRM_DEBUG_KMS("Failed to get link status\n"); > + return false; > } > > - if (!intel_encoder->base.crtc) > - return; > + /* Check if the link is valid by reading the bits of Link status > + * registers > + */ > + if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { > + DRM_DEBUG_KMS("Channel EQ or CR not ok, need to retrain\n"); drm_dp_channel_eq_ok() does not check for CR. Should we just say "Channel EQ not ok" to preempt ambiguity while debugging ? > + return false; > + } > > - if (!to_intel_crtc(intel_encoder->base.crtc)->active) > - return; > + DRM_DEBUG_KMS("Link is good, no need to retrain\n"); The caller does not expect us to link train anymore, I don't think we have to explicitly state "no need to retrain". Also, do we need debug messages if the link is good? > + return true; > > - /* 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: > @@ -3928,6 +3925,8 @@ static bool > intel_dp_short_pulse(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct intel_encoder *intel_encoder = &intel_dig_port->base; > u8 sink_irq_vector = 0; > u8 old_sink_count = intel_dp->sink_count; > bool ret; > @@ -3968,8 +3967,18 @@ intel_dp_short_pulse(struct intel_dp *intel_dp) > DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); > } > > + /* Do not train the link if there is no crtc */ > + if (!intel_encoder->base.crtc) > + return true; > + if (!to_intel_crtc(intel_encoder->base.crtc)->active) > + return true; > + I might be completely off base here. Shouldn't we keep the link valid irrespective of whether there is an active crtc? I thought that is what the refactoring is supposed to enable. Does intel_dp_short_pulse() get called when there is a link loss during upfront link training? And in that case, shouldn't we retrain even without a crtc? Besides that, how about using just one return? struct drm_crtc *crtc = intel_encoder->base.crtc; if (crtc == NULL || !to_intel_crtc(crtc)->active) return true; > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > - intel_dp_check_link_status(intel_dp); > + if (!intel_dp_link_is_valid(intel_dp) || > + intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) { > + intel_dp_start_link_train(intel_dp); > + intel_dp_stop_link_train(intel_dp); > + } > drm_modeset_unlock(&dev->mode_config.connection_mutex); > > return true; > @@ -4298,8 +4307,17 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) > * check links status, there has been known issues of > * link loss triggerring long pulse!!!! > */ > + /* Do not train the link if there is no crtc */ > + if (!intel_encoder->base.crtc) > + goto out; > + if (!to_intel_crtc(intel_encoder->base.crtc)->active) > + goto out; > + > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > - intel_dp_check_link_status(intel_dp); > + if (!intel_dp_link_is_valid(intel_dp)) { > + intel_dp_start_link_train(intel_dp); > + intel_dp_stop_link_train(intel_dp); > + } > drm_modeset_unlock(&dev->mode_config.connection_mutex); > goto out; > }
On Thu, Aug 11, 2016 at 08:18:54PM -0700, Pandiyan, Dhinakaran wrote: > On Thu, 2016-08-11 at 15:23 -0700, Manasi Navare wrote: > > Intel_dp_link_is_valid() function reads the Link status registers > > and returns a boolean to indicate link is valid or not. > > If the link has lost lock and is not valid any more, link > > training is performed outside the function else previously trained link > > is retained. > > This gives us flexibility of checking whether link is valid and training > > it independently. > > > > v2: > > * Changed the function name from intel_dp_check_link_status() > > to intel_dp_link_is_valid() (Lukas Wunner) > > * Checks for CRTC and active CRTC are moved outside the > > intel_dp_link_is_valid() function (Rodrigo Vivi) > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++++-------------- > > 1 file changed, 37 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 364db90..891147d 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -3881,36 +3881,33 @@ go_again: > > return -EINVAL; > > } > > > > -static void > > -intel_dp_check_link_status(struct intel_dp *intel_dp) > > +static bool > > +intel_dp_link_is_valid(struct intel_dp *intel_dp) > > { > > - struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; > > struct drm_device *dev = intel_dp_to_dev(intel_dp); > > u8 link_status[DP_LINK_STATUS_SIZE]; > > > > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); > > > > if (!intel_dp_get_link_status(intel_dp, link_status)) { > > - DRM_ERROR("Failed to get link status\n"); > > - return; > > + DRM_DEBUG_KMS("Failed to get link status\n"); > > + return false; > > } > > > > - if (!intel_encoder->base.crtc) > > - return; > > + /* Check if the link is valid by reading the bits of Link status > > + * registers > > + */ > > + if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { > > + DRM_DEBUG_KMS("Channel EQ or CR not ok, need to retrain\n"); > drm_dp_channel_eq_ok() does not check for CR. Should we just say > "Channel EQ not ok" to preempt ambiguity while debugging ? Actually this macro checks for DP_CHANNEL_EQ_BITS which is defined as: #define DP_CHANNEL_EQ_BITS (DP_LANE_CR_DONE | \ DP_LANE_CHANNEL_EQ_DONE | \ DP_LANE_SYMBOL_LOCKED) So it includes checking for Channel EQ and Clock Recovery CR bits > > > + return false; > > + } > > > > - if (!to_intel_crtc(intel_encoder->base.crtc)->active) > > - return; > > + DRM_DEBUG_KMS("Link is good, no need to retrain\n"); > The caller does not expect us to link train anymore, I don't think we > have to explicitly state "no need to retrain". Also, do we need debug > messages if the link is good? I agree , maybe this is not needed. I will remove this > > > + return true; > > > > - /* 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: > > @@ -3928,6 +3925,8 @@ static bool > > intel_dp_short_pulse(struct intel_dp *intel_dp) > > { > > struct drm_device *dev = intel_dp_to_dev(intel_dp); > > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > + struct intel_encoder *intel_encoder = &intel_dig_port->base; > > u8 sink_irq_vector = 0; > > u8 old_sink_count = intel_dp->sink_count; > > bool ret; > > @@ -3968,8 +3967,18 @@ intel_dp_short_pulse(struct intel_dp *intel_dp) > > DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); > > } > > > > + /* Do not train the link if there is no crtc */ > > + if (!intel_encoder->base.crtc) > > + return true; > > + if (!to_intel_crtc(intel_encoder->base.crtc)->active) > > + return true; > > + > I might be completely off base here. Shouldn't we keep the link valid > irrespective of whether there is an active crtc? I thought that is what > the refactoring is supposed to enable. Does intel_dp_short_pulse() get > called when there is a link loss during upfront link training? And in > that case, shouldn't we retrain even without a crtc? We cannot ever retrain without a CRTC. This check is more for making sure that the clocks are set up befofe we try to retrain else we will see AUX channel failures. If I track this back in the kernel tree, this check was added to avoid the lock up issues on some platforms. > > Besides that, how about using just one return? > > struct drm_crtc *crtc = intel_encoder->base.crtc; > > if (crtc == NULL || !to_intel_crtc(crtc)->active) > return true; > > The only problem with doing both these checks together is that if crtc is NULL then we are trying to dereference a NULL pointer in the second check. So it should be seuqential, check if crtc is active only if there is crtc available. Manasi > > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > - intel_dp_check_link_status(intel_dp); > > + if (!intel_dp_link_is_valid(intel_dp) || > > + intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) { > > + intel_dp_start_link_train(intel_dp); > > + intel_dp_stop_link_train(intel_dp); > > + } > > drm_modeset_unlock(&dev->mode_config.connection_mutex); > > > > return true; > > @@ -4298,8 +4307,17 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) > > * check links status, there has been known issues of > > * link loss triggerring long pulse!!!! > > */ > > + /* Do not train the link if there is no crtc */ > > + if (!intel_encoder->base.crtc) > > + goto out; > > + if (!to_intel_crtc(intel_encoder->base.crtc)->active) > > + goto out; > > + > > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > - intel_dp_check_link_status(intel_dp); > > + if (!intel_dp_link_is_valid(intel_dp)) { > > + intel_dp_start_link_train(intel_dp); > > + intel_dp_stop_link_train(intel_dp); > > + } > > drm_modeset_unlock(&dev->mode_config.connection_mutex); > > goto out; > > } >
On Fri, 2016-08-12 at 10:56 -0700, Manasi Navare wrote: > On Thu, Aug 11, 2016 at 08:18:54PM -0700, Pandiyan, Dhinakaran wrote: > > On Thu, 2016-08-11 at 15:23 -0700, Manasi Navare wrote: > > > Intel_dp_link_is_valid() function reads the Link status registers > > > and returns a boolean to indicate link is valid or not. > > > If the link has lost lock and is not valid any more, link > > > training is performed outside the function else previously trained link > > > is retained. > > > This gives us flexibility of checking whether link is valid and training > > > it independently. > > > > > > v2: > > > * Changed the function name from intel_dp_check_link_status() > > > to intel_dp_link_is_valid() (Lukas Wunner) > > > * Checks for CRTC and active CRTC are moved outside the > > > intel_dp_link_is_valid() function (Rodrigo Vivi) > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++++-------------- > > > 1 file changed, 37 insertions(+), 19 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index 364db90..891147d 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -3881,36 +3881,33 @@ go_again: > > > return -EINVAL; > > > } > > > > > > -static void > > > -intel_dp_check_link_status(struct intel_dp *intel_dp) > > > +static bool > > > +intel_dp_link_is_valid(struct intel_dp *intel_dp) > > > { > > > - struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; > > > struct drm_device *dev = intel_dp_to_dev(intel_dp); > > > u8 link_status[DP_LINK_STATUS_SIZE]; > > > > > > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); > > > > > > if (!intel_dp_get_link_status(intel_dp, link_status)) { > > > - DRM_ERROR("Failed to get link status\n"); > > > - return; > > > + DRM_DEBUG_KMS("Failed to get link status\n"); > > > + return false; > > > } > > > > > > - if (!intel_encoder->base.crtc) > > > - return; > > > + /* Check if the link is valid by reading the bits of Link status > > > + * registers > > > + */ > > > + if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { > > > + DRM_DEBUG_KMS("Channel EQ or CR not ok, need to retrain\n"); > > drm_dp_channel_eq_ok() does not check for CR. Should we just say > > "Channel EQ not ok" to preempt ambiguity while debugging ? > > Actually this macro checks for DP_CHANNEL_EQ_BITS which is defined as: > #define DP_CHANNEL_EQ_BITS (DP_LANE_CR_DONE | \ > DP_LANE_CHANNEL_EQ_DONE | \ > DP_LANE_SYMBOL_LOCKED) > So it includes checking for Channel EQ and Clock Recovery CR bits > > Thank you, I should have looked hard. I will leave this to you. > > > > > + return false; > > > + } > > > > > > - if (!to_intel_crtc(intel_encoder->base.crtc)->active) > > > - return; > > > + DRM_DEBUG_KMS("Link is good, no need to retrain\n"); > > The caller does not expect us to link train anymore, I don't think we > > have to explicitly state "no need to retrain". Also, do we need debug > > messages if the link is good? > > I agree , maybe this is not needed. I will remove this > > > > > > + return true; > > > > > > - /* 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: > > > @@ -3928,6 +3925,8 @@ static bool > > > intel_dp_short_pulse(struct intel_dp *intel_dp) > > > { > > > struct drm_device *dev = intel_dp_to_dev(intel_dp); > > > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > > + struct intel_encoder *intel_encoder = &intel_dig_port->base; > > > u8 sink_irq_vector = 0; > > > u8 old_sink_count = intel_dp->sink_count; > > > bool ret; > > > @@ -3968,8 +3967,18 @@ intel_dp_short_pulse(struct intel_dp *intel_dp) > > > DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); > > > } > > > > > > + /* Do not train the link if there is no crtc */ > > > + if (!intel_encoder->base.crtc) > > > + return true; > > > + if (!to_intel_crtc(intel_encoder->base.crtc)->active) > > > + return true; > > > + > > I might be completely off base here. Shouldn't we keep the link valid > > irrespective of whether there is an active crtc? I thought that is what > > the refactoring is supposed to enable. Does intel_dp_short_pulse() get > > called when there is a link loss during upfront link training? And in > > that case, shouldn't we retrain even without a crtc? > > We cannot ever retrain without a CRTC. This check is more for making sure that the clocks > are set up befofe we try to retrain else we will see AUX channel failures. > If I track this back in the kernel tree, this check was added to avoid the lock up issues on some > platforms. So, crtc will be active by the time we get short pulse for upfront link training failures ? > > > > Besides that, how about using just one return? > > > > struct drm_crtc *crtc = intel_encoder->base.crtc; > > > > if (crtc == NULL || !to_intel_crtc(crtc)->active) > > return true; > > > > > > The only problem with doing both these checks together is that if crtc is NULL > then we are trying to dereference a NULL pointer in the second check. > So it should be seuqential, check if crtc is active only if there is crtc available. > > Manasi > afaik the second check won't be evaluated if the first is True. > > > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > > - intel_dp_check_link_status(intel_dp); > > > + if (!intel_dp_link_is_valid(intel_dp) || > > > + intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) { > > > + intel_dp_start_link_train(intel_dp); > > > + intel_dp_stop_link_train(intel_dp); > > > + } > > > drm_modeset_unlock(&dev->mode_config.connection_mutex); > > > > > > return true; > > > @@ -4298,8 +4307,17 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) > > > * check links status, there has been known issues of > > > * link loss triggerring long pulse!!!! > > > */ > > > + /* Do not train the link if there is no crtc */ > > > + if (!intel_encoder->base.crtc) > > > + goto out; > > > + if (!to_intel_crtc(intel_encoder->base.crtc)->active) > > > + goto out; > > > + > > > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > > - intel_dp_check_link_status(intel_dp); > > > + if (!intel_dp_link_is_valid(intel_dp)) { > > > + intel_dp_start_link_train(intel_dp); > > > + intel_dp_stop_link_train(intel_dp); > > > + } > > > drm_modeset_unlock(&dev->mode_config.connection_mutex); > > > goto out; > > > } > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Aug 12, 2016 at 02:50:58PM -0700, Pandiyan, Dhinakaran wrote: > On Fri, 2016-08-12 at 10:56 -0700, Manasi Navare wrote: > > On Thu, Aug 11, 2016 at 08:18:54PM -0700, Pandiyan, Dhinakaran wrote: > > > On Thu, 2016-08-11 at 15:23 -0700, Manasi Navare wrote: > > > > Intel_dp_link_is_valid() function reads the Link status registers > > > > and returns a boolean to indicate link is valid or not. > > > > If the link has lost lock and is not valid any more, link > > > > training is performed outside the function else previously trained link > > > > is retained. > > > > This gives us flexibility of checking whether link is valid and training > > > > it independently. > > > > > > > > v2: > > > > * Changed the function name from intel_dp_check_link_status() > > > > to intel_dp_link_is_valid() (Lukas Wunner) > > > > * Checks for CRTC and active CRTC are moved outside the > > > > intel_dp_link_is_valid() function (Rodrigo Vivi) > > > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++++-------------- > > > > 1 file changed, 37 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > > index 364db90..891147d 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -3881,36 +3881,33 @@ go_again: > > > > return -EINVAL; > > > > } > > > > > > > > -static void > > > > -intel_dp_check_link_status(struct intel_dp *intel_dp) > > > > +static bool > > > > +intel_dp_link_is_valid(struct intel_dp *intel_dp) > > > > { > > > > - struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; > > > > struct drm_device *dev = intel_dp_to_dev(intel_dp); > > > > u8 link_status[DP_LINK_STATUS_SIZE]; > > > > > > > > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); > > > > > > > > if (!intel_dp_get_link_status(intel_dp, link_status)) { > > > > - DRM_ERROR("Failed to get link status\n"); > > > > - return; > > > > + DRM_DEBUG_KMS("Failed to get link status\n"); > > > > + return false; > > > > } > > > > > > > > - if (!intel_encoder->base.crtc) > > > > - return; > > > > + /* Check if the link is valid by reading the bits of Link status > > > > + * registers > > > > + */ > > > > + if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { > > > > + DRM_DEBUG_KMS("Channel EQ or CR not ok, need to retrain\n"); > > > drm_dp_channel_eq_ok() does not check for CR. Should we just say > > > "Channel EQ not ok" to preempt ambiguity while debugging ? > > > > Actually this macro checks for DP_CHANNEL_EQ_BITS which is defined as: > > #define DP_CHANNEL_EQ_BITS (DP_LANE_CR_DONE | \ > > DP_LANE_CHANNEL_EQ_DONE | \ > > DP_LANE_SYMBOL_LOCKED) > > So it includes checking for Channel EQ and Clock Recovery CR bits > > > > > > Thank you, I should have looked hard. I will leave this to you. > > > > > > > > + return false; > > > > + } > > > > > > > > - if (!to_intel_crtc(intel_encoder->base.crtc)->active) > > > > - return; > > > > + DRM_DEBUG_KMS("Link is good, no need to retrain\n"); > > > The caller does not expect us to link train anymore, I don't think we > > > have to explicitly state "no need to retrain". Also, do we need debug > > > messages if the link is good? > > > > I agree , maybe this is not needed. I will remove this > > > > > > > > > + return true; > > > > > > > > - /* 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: > > > > @@ -3928,6 +3925,8 @@ static bool > > > > intel_dp_short_pulse(struct intel_dp *intel_dp) > > > > { > > > > struct drm_device *dev = intel_dp_to_dev(intel_dp); > > > > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > > > + struct intel_encoder *intel_encoder = &intel_dig_port->base; > > > > u8 sink_irq_vector = 0; > > > > u8 old_sink_count = intel_dp->sink_count; > > > > bool ret; > > > > @@ -3968,8 +3967,18 @@ intel_dp_short_pulse(struct intel_dp *intel_dp) > > > > DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); > > > > } > > > > > > > > + /* Do not train the link if there is no crtc */ > > > > + if (!intel_encoder->base.crtc) > > > > + return true; > > > > + if (!to_intel_crtc(intel_encoder->base.crtc)->active) > > > > + return true; > > > > + > > > I might be completely off base here. Shouldn't we keep the link valid > > > irrespective of whether there is an active crtc? I thought that is what > > > the refactoring is supposed to enable. Does intel_dp_short_pulse() get > > > called when there is a link loss during upfront link training? And in > > > that case, shouldn't we retrain even without a crtc? > > > > We cannot ever retrain without a CRTC. This check is more for making sure that the clocks > > are set up befofe we try to retrain else we will see AUX channel failures. > > If I track this back in the kernel tree, this check was added to avoid the lock up issues on some > > platforms. > > So, crtc will be active by the time we get short pulse for upfront link > training failures ? So the way locks are taken by upfront link train, it would have enabled the crtc before it can handle link loss related short pulses. > > > > > > > Besides that, how about using just one return? > > > > > > struct drm_crtc *crtc = intel_encoder->base.crtc; > > > > > > if (crtc == NULL || !to_intel_crtc(crtc)->active) > > > return true; > > > > > > > > > > The only problem with doing both these checks together is that if crtc is NULL > > then we are trying to dereference a NULL pointer in the second check. > > So it should be seuqential, check if crtc is active only if there is crtc available. > > > > Manasi > > > > afaik the second check won't be evaluated if the first is True. > Yup, makes sense. I will change that > > > > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > > > - intel_dp_check_link_status(intel_dp); > > > > + if (!intel_dp_link_is_valid(intel_dp) || > > > > + intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) { > > > > + intel_dp_start_link_train(intel_dp); > > > > + intel_dp_stop_link_train(intel_dp); > > > > + } > > > > drm_modeset_unlock(&dev->mode_config.connection_mutex); > > > > > > > > return true; > > > > @@ -4298,8 +4307,17 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) > > > > * check links status, there has been known issues of > > > > * link loss triggerring long pulse!!!! > > > > */ > > > > + /* Do not train the link if there is no crtc */ > > > > + if (!intel_encoder->base.crtc) > > > > + goto out; > > > > + if (!to_intel_crtc(intel_encoder->base.crtc)->active) > > > > + goto out; > > > > + > > > > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > > > - intel_dp_check_link_status(intel_dp); > > > > + if (!intel_dp_link_is_valid(intel_dp)) { > > > > + intel_dp_start_link_train(intel_dp); > > > > + intel_dp_stop_link_train(intel_dp); > > > > + } > > > > drm_modeset_unlock(&dev->mode_config.connection_mutex); > > > > goto out; > > > > } > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 364db90..891147d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3881,36 +3881,33 @@ go_again: return -EINVAL; } -static void -intel_dp_check_link_status(struct intel_dp *intel_dp) +static bool +intel_dp_link_is_valid(struct intel_dp *intel_dp) { - struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; struct drm_device *dev = intel_dp_to_dev(intel_dp); u8 link_status[DP_LINK_STATUS_SIZE]; WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); if (!intel_dp_get_link_status(intel_dp, link_status)) { - DRM_ERROR("Failed to get link status\n"); - return; + DRM_DEBUG_KMS("Failed to get link status\n"); + return false; } - if (!intel_encoder->base.crtc) - return; + /* Check if the link is valid by reading the bits of Link status + * registers + */ + if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { + DRM_DEBUG_KMS("Channel EQ or CR not ok, need to retrain\n"); + return false; + } - if (!to_intel_crtc(intel_encoder->base.crtc)->active) - return; + DRM_DEBUG_KMS("Link is good, no need to retrain\n"); + return true; - /* 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: @@ -3928,6 +3925,8 @@ static bool intel_dp_short_pulse(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct intel_encoder *intel_encoder = &intel_dig_port->base; u8 sink_irq_vector = 0; u8 old_sink_count = intel_dp->sink_count; bool ret; @@ -3968,8 +3967,18 @@ intel_dp_short_pulse(struct intel_dp *intel_dp) DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); } + /* Do not train the link if there is no crtc */ + if (!intel_encoder->base.crtc) + return true; + if (!to_intel_crtc(intel_encoder->base.crtc)->active) + return true; + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); - intel_dp_check_link_status(intel_dp); + if (!intel_dp_link_is_valid(intel_dp) || + intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) { + intel_dp_start_link_train(intel_dp); + intel_dp_stop_link_train(intel_dp); + } drm_modeset_unlock(&dev->mode_config.connection_mutex); return true; @@ -4298,8 +4307,17 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) * check links status, there has been known issues of * link loss triggerring long pulse!!!! */ + /* Do not train the link if there is no crtc */ + if (!intel_encoder->base.crtc) + goto out; + if (!to_intel_crtc(intel_encoder->base.crtc)->active) + goto out; + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); - intel_dp_check_link_status(intel_dp); + if (!intel_dp_link_is_valid(intel_dp)) { + intel_dp_start_link_train(intel_dp); + intel_dp_stop_link_train(intel_dp); + } drm_modeset_unlock(&dev->mode_config.connection_mutex); goto out; }
Intel_dp_link_is_valid() function reads the Link status registers and returns a boolean to indicate link is valid or not. If the link has lost lock and is not valid any more, link training is performed outside the function else previously trained link is retained. This gives us flexibility of checking whether link is valid and training it independently. v2: * Changed the function name from intel_dp_check_link_status() to intel_dp_link_is_valid() (Lukas Wunner) * Checks for CRTC and active CRTC are moved outside the intel_dp_link_is_valid() function (Rodrigo Vivi) Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 19 deletions(-)