| Message ID | 1503413631-18438-2-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 22, 2017 at 08:23:49PM +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. > > V2: addressed review comments from Imre, and added r-b > - use int instead of u8 for counter > - use for loop instead of do...while(); > V3: Rebase > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Imre Deak <imre.deak@intel.com> > > Reviewed-by: Imre Deak <imre.deak@intel.com> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > --- > 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..fc8c7ac 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; > + int retry; > 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, > - &data, sizeof(data)); > + for (retry = 5; ; retry--) { > + ret = drm_dp_dual_mode_read(adapter, > + DP_DUAL_MODE_LSPCON_CURRENT_MODE, > + &data, sizeof(data)); Why do we still need the retry here with patch 3/3? > + if (!ret || !retry) > + break; > + usleep_range(500, 1000); > + } > + > 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 >
Regards Shashank On 8/22/2017 8:24 PM, Imre Deak wrote: > On Tue, Aug 22, 2017 at 08:23:49PM +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. >> >> V2: addressed review comments from Imre, and added r-b >> - use int instead of u8 for counter >> - use for loop instead of do...while(); >> V3: Rebase >> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Imre Deak <imre.deak@intel.com> >> >> Reviewed-by: Imre Deak <imre.deak@intel.com> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> >> --- >> 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..fc8c7ac 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; >> + int retry; >> 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, >> - &data, sizeof(data)); >> + for (retry = 5; ; retry--) { >> + ret = drm_dp_dual_mode_read(adapter, >> + DP_DUAL_MODE_LSPCON_CURRENT_MODE, >> + &data, sizeof(data)); > Why do we still need the retry here with patch 3/3? The only reason I can think of would be I am paranoid :) ? I was thinking of going with this one first and see the CI results, and then later remove and optimize. But I guess you are right, this would be too many retries here, and I should do it other way around. - Shashank >> + if (!ret || !retry) >> + break; >> + usleep_range(500, 1000); >> + } >> + >> 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 >>
On Tue, 22 Aug 2017, Shashank Sharma <shashank.sharma@intel.com> 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. > > V2: addressed review comments from Imre, and added r-b > - use int instead of u8 for counter > - use for loop instead of do...while(); > V3: Rebase > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Imre Deak <imre.deak@intel.com> > > Reviewed-by: Imre Deak <imre.deak@intel.com> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > --- > 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..fc8c7ac 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; > + int retry; > 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, > - &data, sizeof(data)); > + for (retry = 5; ; retry--) { > + ret = drm_dp_dual_mode_read(adapter, > + DP_DUAL_MODE_LSPCON_CURRENT_MODE, > + &data, sizeof(data)); > + if (!ret || !retry) > + break; > + usleep_range(500, 1000); > + } Sorry, but that looks like a programming quiz to me. At most how many time does drm_dp_dual_mode_read() get called? With this, for example, the C programmer's spine tells you "six times" for the paradigm for loop before you even really stop to think about it: for (try = 0; try < 6; try++) { if (try) usleep_range(500, 1000); ret = drm_dp_dual_mode_read(); if (!ret) break; } BR, Jani. PS. I'm left wondering if "retry = 5" was there to emphasize that there's one try and five *retries*. > + > if (ret < 0) { > - DRM_ERROR("LSPCON read(0x80, 0x41) failed\n"); > + DRM_DEBUG_KMS("LSPCON read(0x80, 0x41) failed\n"); > return -EFAULT; > }
Regards Shashank On 8/22/2017 8:57 PM, Jani Nikula wrote: > On Tue, 22 Aug 2017, Shashank Sharma <shashank.sharma@intel.com> 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. >> >> V2: addressed review comments from Imre, and added r-b >> - use int instead of u8 for counter >> - use for loop instead of do...while(); >> V3: Rebase >> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Imre Deak <imre.deak@intel.com> >> >> Reviewed-by: Imre Deak <imre.deak@intel.com> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> >> --- >> 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..fc8c7ac 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; >> + int retry; >> 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, >> - &data, sizeof(data)); >> + for (retry = 5; ; retry--) { >> + ret = drm_dp_dual_mode_read(adapter, >> + DP_DUAL_MODE_LSPCON_CURRENT_MODE, >> + &data, sizeof(data)); >> + if (!ret || !retry) >> + break; >> + usleep_range(500, 1000); >> + } > Sorry, but that looks like a programming quiz to me. At most how many > time does drm_dp_dual_mode_read() get called? Yes, you are correct, the loop gets called 6 times, in the initial version of this code, I had the condition to be if (!ret || !--retry) break; which was serving the purpose of 6 calls to drm_dp_dual_mode_read() but only 5 delays. But later I moved some code, and messed with the logic. I will modify this. > > With this, for example, the C programmer's spine tells you "six times" > for the paradigm for loop before you even really stop to think about it: > > for (try = 0; try < 6; try++) { > if (try) > usleep_range(500, 1000); > ret = drm_dp_dual_mode_read(); > if (!ret) > break; > } > > BR, > Jani. > > > PS. I'm left wondering if "retry = 5" was there to emphasize that > there's one try and five *retries*. Actually, the aim was to do 6 call to drm_dp_dual_mode_read() but add only 5 delays, as I wanted the first read also in the loop. But as you rightly mention, the latest loop was not good enough. - Shashank > > >> + >> if (ret < 0) { >> - DRM_ERROR("LSPCON read(0x80, 0x41) failed\n"); >> + DRM_DEBUG_KMS("LSPCON read(0x80, 0x41) failed\n"); >> return -EFAULT; >> }
diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c index 80e62f6..fc8c7ac 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; + int retry; 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, - &data, sizeof(data)); + for (retry = 5; ; retry--) { + ret = drm_dp_dual_mode_read(adapter, + DP_DUAL_MODE_LSPCON_CURRENT_MODE, + &data, sizeof(data)); + if (!ret || !retry) + break; + usleep_range(500, 1000); + } + if (ret < 0) { - DRM_ERROR("LSPCON read(0x80, 0x41) failed\n"); + DRM_DEBUG_KMS("LSPCON read(0x80, 0x41) failed\n"); return -EFAULT; }
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. V2: addressed review comments from Imre, and added r-b - use int instead of u8 for counter - use for loop instead of do...while(); V3: Rebase Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Imre Deak <imre.deak@intel.com> Reviewed-by: Imre Deak <imre.deak@intel.com> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> --- drivers/gpu/drm/drm_dp_dual_mode_helper.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)