Re: [Intel-gfx] [PATCH 1/2] drm: add retries for lspcon status check

2017-08-16 Thread Sharma, Shashank 

Regards

Shashank


On 8/16/2017 9:42 PM, Imre Deak wrote:

On Wed, Aug 16, 2017 at 09:18:58PM +0530, Sharma, Shashank wrote:

Thanks for the review, Imre.

My comments, inline.

Regards
Shashank
On 8/16/2017 7:35 PM, Imre Deak wrote:

On Fri, Aug 11, 2017 at 06:58:26PM +0530, Shashank Sharma wrote:

It's an observation during some CI tests that few LSPCON chips
respond slow while system is under load, and need some delay
while reading current mode status using i2c-over-aux channel.

This patch:
- Adds few retries and delays before declaring a read
failure from LSPCON hardware.
- Changes the debug level of the print from ERROR->DEBUG
whereas another patch in I915 will add an ERROR message
from the caller when we have timed out all our limits.

Hm yea, I was wondering if this is the same issue we saw earlier due to
HPD not being asserted. But looking at the logs it's not the case. After
HPD is asserted it really takes this much time (~36ms) for the adaptor
to respond. This is against the DP 1.4 spec which specifies a 20ms
maximum wake up time, but what can you do.


Cc: Ville Syrjala 
Cc: Imre Deak 
Signed-off-by: Shashank Sharma 
Signed-off-by: Mahesh Kumar 
---
   drivers/gpu/drm/drm_dp_dual_mode_helper.c | 14 +++---
   1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c 
b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
index 80e62f6..c63eac8 100644
--- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
+++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
@@ -409,6 +409,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
enum drm_lspcon_mode *mode)
   {
u8 data;
+   u8 retry = 5;

Nitpick: no reason for a specific type so just int?

Actually, was trying to save 3 bytes, as I knew max value for retry would be
5 < 255, but might be too much optimization ?

Yes, imo it's odd to use u8 for simple loop counter. The compiler may
even generate some extra code to zero extend or truncate the result.

cool, so int it is.

int ret = 0;
if (!mode) {
@@ -417,10 +418,17 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
}
/* Read Status: i2c over aux */
-   ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
-   , sizeof(data));
+   do {

Nitpick: a for loop whenever possible is generally clearer.

Sorry, I dint understand this comment, little more help required :) ?

for (retry = 0; retry < 5; retry++) ...

would be clearer imo.

Sure, will do it.

--Imre


- Shashank

With the above optionally changed:
Reviewed-by: Imre Deak 


+   ret = drm_dp_dual_mode_read(adapter,
+   DP_DUAL_MODE_LSPCON_CURRENT_MODE,
+   , sizeof(data));
+   if (!ret || !retry--)
+   break;
+   usleep_range(500, 1000);
+   } while (1);
+
if (ret < 0) {
-   DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
+   DRM_DEBUG_KMS("LSPCON read(0x80, 0x41) failed\n");
return -EFAULT;
}
--
2.7.4



___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm: add retries for lspcon status check

2017-08-16 Thread Imre Deak 
On Wed, Aug 16, 2017 at 09:18:58PM +0530, Sharma, Shashank wrote:
> Thanks for the review, Imre.
> 
> My comments, inline.
> 
> Regards
> Shashank
> On 8/16/2017 7:35 PM, Imre Deak wrote:
> > On Fri, Aug 11, 2017 at 06:58:26PM +0530, Shashank Sharma wrote:
> > > It's an observation during some CI tests that few LSPCON chips
> > > respond slow while system is under load, and need some delay
> > > while reading current mode status using i2c-over-aux channel.
> > > 
> > > This patch:
> > > - Adds few retries and delays before declaring a read
> > >failure from LSPCON hardware.
> > > - Changes the debug level of the print from ERROR->DEBUG
> > >whereas another patch in I915 will add an ERROR message
> > >from the caller when we have timed out all our limits.
> > Hm yea, I was wondering if this is the same issue we saw earlier due to
> > HPD not being asserted. But looking at the logs it's not the case. After
> > HPD is asserted it really takes this much time (~36ms) for the adaptor
> > to respond. This is against the DP 1.4 spec which specifies a 20ms
> > maximum wake up time, but what can you do.
> > 
> > > Cc: Ville Syrjala 
> > > Cc: Imre Deak 
> > > Signed-off-by: Shashank Sharma 
> > > Signed-off-by: Mahesh Kumar 
> > > ---
> > >   drivers/gpu/drm/drm_dp_dual_mode_helper.c | 14 +++---
> > >   1 file changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c 
> > > b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> > > index 80e62f6..c63eac8 100644
> > > --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> > > @@ -409,6 +409,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
> > >   enum drm_lspcon_mode *mode)
> > >   {
> > >   u8 data;
> > > + u8 retry = 5;
> > Nitpick: no reason for a specific type so just int?
> Actually, was trying to save 3 bytes, as I knew max value for retry would be
> 5 < 255, but might be too much optimization ?

Yes, imo it's odd to use u8 for simple loop counter. The compiler may
even generate some extra code to zero extend or truncate the result.

> > >   int ret = 0;
> > >   if (!mode) {
> > > @@ -417,10 +418,17 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
> > >   }
> > >   /* Read Status: i2c over aux */
> > > - ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> > > - , sizeof(data));
> > > + do {
> > Nitpick: a for loop whenever possible is generally clearer.
> Sorry, I dint understand this comment, little more help required :) ?

for (retry = 0; retry < 5; retry++) ...

would be clearer imo.

--Imre

> - Shashank
> > 
> > With the above optionally changed:
> > Reviewed-by: Imre Deak 
> > 
> > > + ret = drm_dp_dual_mode_read(adapter,
> > > + DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> > > + , sizeof(data));
> > > + if (!ret || !retry--)
> > > + break;
> > > + usleep_range(500, 1000);
> > > + } while (1);
> > > +
> > >   if (ret < 0) {
> > > - DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
> > > + DRM_DEBUG_KMS("LSPCON read(0x80, 0x41) failed\n");
> > >   return -EFAULT;
> > >   }
> > > -- 
> > > 2.7.4
> > > 
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm: add retries for lspcon status check

2017-08-16 Thread Sharma, Shashank 

Thanks for the review, Imre.

My comments, inline.

Regards
Shashank
On 8/16/2017 7:35 PM, Imre Deak wrote:

On Fri, Aug 11, 2017 at 06:58:26PM +0530, Shashank Sharma wrote:

It's an observation during some CI tests that few LSPCON chips
respond slow while system is under load, and need some delay
while reading current mode status using i2c-over-aux channel.

This patch:
- Adds few retries and delays before declaring a read
   failure from LSPCON hardware.
- Changes the debug level of the print from ERROR->DEBUG
   whereas another patch in I915 will add an ERROR message
   from the caller when we have timed out all our limits.

Hm yea, I was wondering if this is the same issue we saw earlier due to
HPD not being asserted. But looking at the logs it's not the case. After
HPD is asserted it really takes this much time (~36ms) for the adaptor
to respond. This is against the DP 1.4 spec which specifies a 20ms
maximum wake up time, but what can you do.


Cc: Ville Syrjala 
Cc: Imre Deak 
Signed-off-by: Shashank Sharma 
Signed-off-by: Mahesh Kumar 
---
  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 14 +++---
  1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c 
b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
index 80e62f6..c63eac8 100644
--- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
+++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
@@ -409,6 +409,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
enum drm_lspcon_mode *mode)
  {
u8 data;
+   u8 retry = 5;

Nitpick: no reason for a specific type so just int?
Actually, was trying to save 3 bytes, as I knew max value for retry 
would be 5 < 255, but might be too much optimization ?

int ret = 0;
  
  	if (!mode) {

@@ -417,10 +418,17 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
}
  
  	/* Read Status: i2c over aux */

-   ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
-   , sizeof(data));
+   do {

Nitpick: a for loop whenever possible is generally clearer.

Sorry, I dint understand this comment, little more help required :) ?
- Shashank


With the above optionally changed:
Reviewed-by: Imre Deak 


+   ret = drm_dp_dual_mode_read(adapter,
+   DP_DUAL_MODE_LSPCON_CURRENT_MODE,
+   , sizeof(data));
+   if (!ret || !retry--)
+   break;
+   usleep_range(500, 1000);
+   } while (1);
+
if (ret < 0) {
-   DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
+   DRM_DEBUG_KMS("LSPCON read(0x80, 0x41) failed\n");
return -EFAULT;
}
  
--

2.7.4



___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm: add retries for lspcon status check

2017-08-14 Thread Pandiyan, Dhinakaran 
On Fri, 2017-08-11 at 18:58 +0530, Shashank Sharma wrote:
> It's an observation during some CI tests that few LSPCON chips
> respond slow while system is under load, and need some delay

Thanks for the patch. Can you please explain what you mean by load here?
I can't follow why an external chip would respond slow if the system is
under load.

> while reading current mode status using i2c-over-aux channel.
> 

Is there a fdo bug where I can see more CI logs?

The only log I saw has this pattern around the time where this error
occurs. I am not sure how GPU hangs affect AUX transactions. But, I
think we should investigate/fix the underlying problem instead of adding
delays.


[   78.758510] i915 :00:02.0: Resetting vcs0 after gpu hang
[   78.758922] [drm:i915_gem_reset_engine [i915]] context
kms_busy[1372]/0 marked guilty (score 19) banned? no
[   78.758941] [drm:i915_gem_reset_engine [i915]] resetting vcs0 to
restart from tail of request 0x62
[   78.758990] [drm:gen8_init_common_ring [i915]] Execlists enabled for
vcs0
[   78.759014] [drm:gen8_init_common_ring [i915]] Restarting vcs0:0 from
0x62
[   78.759482] [drm:intel_power_well_enable [i915]] enabling always-on
[   78.759499] [drm:intel_power_well_enable [i915]] enabling DC off
[   78.759908] [drm:gen9_set_dc_state [i915]] Setting DC state from 02
to 00
[   78.759932] [drm:intel_power_well_enable [i915]] enabling power well
2
[   78.759993] [drm:intel_atomic_commit_tail [i915]] [ENCODER:57:DDI B]
[   78.760021] [drm:intel_atomic_commit_tail [i915]] [ENCODER:59:DP-MST
A]
[   78.760079] [drm:intel_atomic_commit_tail [i915]] [ENCODER:60:DP-MST
B]
[   78.760107] [drm:intel_atomic_commit_tail [i915]] [ENCODER:61:DP-MST
C]
[   78.760136] [drm:intel_atomic_commit_tail [i915]] [ENCODER:64:DDI C]
[   78.760163] [drm:intel_atomic_commit_tail [i915]] [ENCODER:66:DP-MST
A]
[   78.760191] [drm:intel_atomic_commit_tail [i915]] [ENCODER:67:DP-MST
B]
[   78.760218] [drm:intel_atomic_commit_tail [i915]] [ENCODER:68:DP-MST
C]
[   78.760246] [drm:verify_single_dpll_state.isra.70 [i915]] DPLL 0
[   78.760275] [drm:verify_single_dpll_state.isra.70 [i915]] DPLL 1
[   78.760304] [drm:verify_single_dpll_state.isra.70 [i915]] DPLL 2
[   78.760332] [drm:verify_single_dpll_state.isra.70 [i915]] DPLL 3
[   78.760362] [drm:intel_enable_shared_dpll [i915]] enable DPLL 1
(active 1, on? 0) for crtc 36
[   78.760390] [drm:intel_enable_shared_dpll [i915]] enabling DPLL 1
[   78.762529] [drm:intel_power_well_enable [i915]] enabling DDI B IO
power well
[   78.763146] [drm:drm_dp_i2c_do_msg] native defer
[   78.764268] [drm:drm_dp_i2c_do_msg] native defer
[   78.765735] [drm:drm_dp_i2c_do_msg] native defer
[   78.766794] [drm:drm_dp_i2c_do_msg] native defer
[   78.767853] [drm:drm_dp_i2c_do_msg] native defer
[   78.768982] [drm:drm_dp_i2c_do_msg] native defer
[   78.770112] [drm:drm_dp_i2c_do_msg] native defer
[   78.771240] [drm:drm_dp_i2c_do_msg] native defer
[   78.772371] [drm:drm_dp_i2c_do_msg] native defer
[   78.773068] [drm:drm_dp_i2c_do_msg] too many retries, giving up
[   78.773507] [drm:drm_dp_i2c_do_msg] native defer
[   78.774531] [drm:drm_dp_i2c_do_msg] native defer
[   78.775629] [drm:drm_dp_i2c_do_msg] native defer
[   78.776668] [drm:drm_dp_i2c_do_msg] native defer
[   78.10] [drm:drm_dp_i2c_do_msg] native defer
[   78.778810] [drm:drm_dp_i2c_do_msg] native defer
[   78.779907] [drm:drm_dp_i2c_do_msg] native defer
[   78.780585] [drm:drm_dp_i2c_do_msg] too many retries, giving up
[   78.780589] [drm:drm_lspcon_get_mode] *ERROR* LSPCON read(0x80, 0x41)
failed
[   78.780621] [drm:lspcon_wait_mode [i915]] *ERROR* Error reading
LSPCON mode
[   78.780646] [drm:lspcon_wait_mode [i915]] Current LSPCON mode INVALID



> This patch:
> - Adds few retries and delays before declaring a read
>   failure from LSPCON hardware.
> - Changes the debug level of the print from ERROR->DEBUG
>   whereas another patch in I915 will add an ERROR message
>   from the caller when we have timed out all our limits.
> 
> Cc: Ville Syrjala 
> Cc: Imre Deak 
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Mahesh Kumar 
> ---
>  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c 
> b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> index 80e62f6..c63eac8 100644
> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> @@ -409,6 +409,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>   enum drm_lspcon_mode *mode)
>  {
>   u8 data;
> + u8 retry = 5;
>   int ret = 0;
>  
>   if (!mode) {
> @@ -417,10 +418,17 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>   }
>  
>   /* Read Status: i2c over aux */
> - ret = drm_dp_dual_mode_read(adapter,