| Message ID | 1467413276-27024-1-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 1, 2016 at 3:47 PM, Manasi Navare <manasi.d.navare@intel.com> wrote: > Intel_dp_check_link_status() function reads the Link status registers > and returns a boolean to indicate link is good or bad. > If the link is bad, it is retrained outside the function based > on the return value. > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 41 ++++++++++++++++++++++++----------------- > 1 file changed, 24 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 6d586b7..c795c9e 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3863,7 +3863,7 @@ go_again: > return -EINVAL; > } > > -static void > +static bool > intel_dp_check_link_status(struct intel_dp *intel_dp) > { > struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; > @@ -3873,26 +3873,25 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) > 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; where did this check go? why don't we need this anymore? > + /* 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_dp_clock_recovery_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; what happens if it is not active now? are we going to attemtp the retraing in this case? in other words: we really don't need this check anymore as the one above? Thanks, Rodrigo. > + 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: > @@ -3950,7 +3949,11 @@ intel_dp_short_pulse(struct intel_dp *intel_dp) > } > > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > - intel_dp_check_link_status(intel_dp); > + if (!intel_dp_check_link_status(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; > @@ -4271,7 +4274,11 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) > * link loss triggerring long pulse!!!! > */ > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > - intel_dp_check_link_status(intel_dp); > + if (!intel_dp_check_link_status(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); > goto out; > } > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi,
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.7-rc5 next-20160701]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Manasi-Navare/drm-i915-intel_dp_check_link_status-should-only-return-status-of-link/20160702-124153
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-i0-201626 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_dp.c: In function 'intel_dp_check_link_status':
>> drivers/gpu/drm/i915/intel_dp.c:3872:24: error: unused variable 'intel_encoder' [-Werror=unused-variable]
struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
^~~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +/intel_encoder +3872 drivers/gpu/drm/i915/intel_dp.c
0e32b39c Dave Airlie 2014-05-02 3866 return -EINVAL;
0e32b39c Dave Airlie 2014-05-02 3867 }
0e32b39c Dave Airlie 2014-05-02 3868
84fa171b Manasi Navare 2016-07-01 3869 static bool
5c9114d0 Shubhangi Shrivastava 2016-03-30 3870 intel_dp_check_link_status(struct intel_dp *intel_dp)
5c9114d0 Shubhangi Shrivastava 2016-03-30 3871 {
5c9114d0 Shubhangi Shrivastava 2016-03-30 @3872 struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
5c9114d0 Shubhangi Shrivastava 2016-03-30 3873 struct drm_device *dev = intel_dp_to_dev(intel_dp);
5c9114d0 Shubhangi Shrivastava 2016-03-30 3874 u8 link_status[DP_LINK_STATUS_SIZE];
5c9114d0 Shubhangi Shrivastava 2016-03-30 3875
:::::: The code at line 3872 was first introduced by commit
:::::: 5c9114d0ced2f16d1bfeda650b4acf95159f4759 drm/i915: Reorganizing intel_dp_check_link_status
:::::: TO: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
:::::: CC: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi,
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v4.7-rc5 next-20160701]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Manasi-Navare/drm-i915-intel_dp_check_link_status-should-only-return-status-of-link/20160702-124153
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-rhel (attached as .config)
compiler: gcc-4.9 (Debian 4.9.3-14) 4.9.3
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_dp.c: In function 'intel_dp_check_link_status':
>> drivers/gpu/drm/i915/intel_dp.c:3872:24: warning: unused variable 'intel_encoder' [-Wunused-variable]
struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
^
vim +/intel_encoder +3872 drivers/gpu/drm/i915/intel_dp.c
0e32b39c Dave Airlie 2014-05-02 3856 return ret;
0e32b39c Dave Airlie 2014-05-02 3857 } else {
0e32b39c Dave Airlie 2014-05-02 3858 struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
0e32b39c Dave Airlie 2014-05-02 3859 DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
0e32b39c Dave Airlie 2014-05-02 3860 intel_dp->is_mst = false;
0e32b39c Dave Airlie 2014-05-02 3861 drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
0e32b39c Dave Airlie 2014-05-02 3862 /* send a hotplug event */
0e32b39c Dave Airlie 2014-05-02 3863 drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
0e32b39c Dave Airlie 2014-05-02 3864 }
0e32b39c Dave Airlie 2014-05-02 3865 }
0e32b39c Dave Airlie 2014-05-02 3866 return -EINVAL;
0e32b39c Dave Airlie 2014-05-02 3867 }
0e32b39c Dave Airlie 2014-05-02 3868
84fa171b Manasi Navare 2016-07-01 3869 static bool
5c9114d0 Shubhangi Shrivastava 2016-03-30 3870 intel_dp_check_link_status(struct intel_dp *intel_dp)
5c9114d0 Shubhangi Shrivastava 2016-03-30 3871 {
5c9114d0 Shubhangi Shrivastava 2016-03-30 @3872 struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
5c9114d0 Shubhangi Shrivastava 2016-03-30 3873 struct drm_device *dev = intel_dp_to_dev(intel_dp);
5c9114d0 Shubhangi Shrivastava 2016-03-30 3874 u8 link_status[DP_LINK_STATUS_SIZE];
5c9114d0 Shubhangi Shrivastava 2016-03-30 3875
5c9114d0 Shubhangi Shrivastava 2016-03-30 3876 WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
5c9114d0 Shubhangi Shrivastava 2016-03-30 3877
5c9114d0 Shubhangi Shrivastava 2016-03-30 3878 if (!intel_dp_get_link_status(intel_dp, link_status)) {
84fa171b Manasi Navare 2016-07-01 3879 DRM_DEBUG_KMS("Failed to get link status\n");
84fa171b Manasi Navare 2016-07-01 3880 return false;
:::::: The code at line 3872 was first introduced by commit
:::::: 5c9114d0ced2f16d1bfeda650b4acf95159f4759 drm/i915: Reorganizing intel_dp_check_link_status
:::::: TO: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
:::::: CC: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Fri, Jul 01, 2016 at 03:47:56PM -0700, Manasi Navare wrote: > Intel_dp_check_link_status() function reads the Link status registers > and returns a boolean to indicate link is good or bad. > If the link is bad, it is retrained outside the function based > on the return value. > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 41 ++++++++++++++++++++++++----------------- > 1 file changed, 24 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 6d586b7..c795c9e 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3863,7 +3863,7 @@ go_again: > return -EINVAL; > } > > -static void > +static bool > intel_dp_check_link_status(struct intel_dp *intel_dp) > { A common pattern is to name bool functions like a yes/no question, so you might want to adjust the name to something like intel_dp_link_status_ok() or intel_dp_link_is_good() here. Best regards, Lukas
On Fri, Jul 01, 2016 at 05:35:25PM -0700, Rodrigo Vivi wrote: > On Fri, Jul 1, 2016 at 3:47 PM, Manasi Navare <manasi.d.navare@intel.com> wrote: > > Intel_dp_check_link_status() function reads the Link status registers > > and returns a boolean to indicate link is good or bad. > > If the link is bad, it is retrained outside the function based > > on the return value. > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 41 ++++++++++++++++++++++++----------------- > > 1 file changed, 24 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 6d586b7..c795c9e 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -3863,7 +3863,7 @@ go_again: > > return -EINVAL; > > } > > > > -static void > > +static bool > > intel_dp_check_link_status(struct intel_dp *intel_dp) > > { > > struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; > > @@ -3873,26 +3873,25 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) > > 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; > > where did this check go? why don't we need this anymore? > This function is being refactored to handle upront link training independent of modeset and according to the spec, this function should only read the DPCD registers to check if the link is valid. In case of upfront link training, we do not care about the crtc or if the crtc is active hence this and the next check for crtc acrive are not needed. > > + /* 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_dp_clock_recovery_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; > > what happens if it is not active now? are we going to attemtp the > retraing in this case? > in other words: we really don't need this check anymore as the one above? > > Thanks, > Rodrigo. > > > + 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: > > @@ -3950,7 +3949,11 @@ intel_dp_short_pulse(struct intel_dp *intel_dp) > > } > > > > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > - intel_dp_check_link_status(intel_dp); > > + if (!intel_dp_check_link_status(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; > > @@ -4271,7 +4274,11 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) > > * link loss triggerring long pulse!!!! > > */ > > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > - intel_dp_check_link_status(intel_dp); > > + if (!intel_dp_check_link_status(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); > > goto out; > > } > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br
On Sat, Jul 02, 2016 at 11:29:05AM +0200, Lukas Wunner wrote: > On Fri, Jul 01, 2016 at 03:47:56PM -0700, Manasi Navare wrote: > > Intel_dp_check_link_status() function reads the Link status registers > > and returns a boolean to indicate link is good or bad. > > If the link is bad, it is retrained outside the function based > > on the return value. > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 41 ++++++++++++++++++++++++----------------- > > 1 file changed, 24 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 6d586b7..c795c9e 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -3863,7 +3863,7 @@ go_again: > > return -EINVAL; > > } > > > > -static void > > +static bool > > intel_dp_check_link_status(struct intel_dp *intel_dp) > > { > > A common pattern is to name bool functions like a yes/no question, > so you might want to adjust the name to something like > intel_dp_link_status_ok() or intel_dp_link_is_good() here. > > Best regards, > > Lukas I will change the name to follow this naming pattern. Regards Manasi
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 6d586b7..c795c9e 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3863,7 +3863,7 @@ go_again: return -EINVAL; } -static void +static bool intel_dp_check_link_status(struct intel_dp *intel_dp) { struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; @@ -3873,26 +3873,25 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) 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_dp_clock_recovery_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: @@ -3950,7 +3949,11 @@ intel_dp_short_pulse(struct intel_dp *intel_dp) } drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); - intel_dp_check_link_status(intel_dp); + if (!intel_dp_check_link_status(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; @@ -4271,7 +4274,11 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) * link loss triggerring long pulse!!!! */ drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); - intel_dp_check_link_status(intel_dp); + if (!intel_dp_check_link_status(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); goto out; }
Intel_dp_check_link_status() function reads the Link status registers and returns a boolean to indicate link is good or bad. If the link is bad, it is retrained outside the function based on the return value. Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-)