Re: [Intel-gfx] [PATCH] drm/i915: intel_dp_check_link_status should only return status of link

2016-07-11 Thread Manasi Navare
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 
> > ---
> >  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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: intel_dp_check_link_status should only return status of link

2016-07-11 Thread Manasi Navare
On Fri, Jul 01, 2016 at 05:35:25PM -0700, Rodrigo Vivi wrote:
> On Fri, Jul 1, 2016 at 3:47 PM, 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 
> > ---
> >  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 = 
> > _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(>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(>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(>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(>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(>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
___
Intel-gfx mailing list

Re: [Intel-gfx] [PATCH] drm/i915: intel_dp_check_link_status should only return status of link

2016-07-02 Thread Lukas Wunner
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 
> ---
>  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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: intel_dp_check_link_status should only return status of link

2016-07-02 Thread kbuild test robot
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 = _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(_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 = _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(>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 
:: CC: Ander Conselvan de Oliveira 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: intel_dp_check_link_status should only return status of link

2016-07-02 Thread kbuild test robot
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 = _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 = _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 
:: CC: Ander Conselvan de Oliveira 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: intel_dp_check_link_status should only return status of link

2016-07-01 Thread Rodrigo Vivi
On Fri, Jul 1, 2016 at 3:47 PM, 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 
> ---
>  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 = _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(>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(>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(>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(>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(>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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [PATCH] drm/i915: intel_dp_check_link_status should only return status of link

2016-07-01 Thread Manasi Navare
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 
---
 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 = _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(>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(>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(>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(>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(>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