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 <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > > Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@xxxxxxxxx>
> > > ---
> > > 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,
> > > - &data, 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 <imre.deak@xxxxxxxxx>
> >
> > > + ret = drm_dp_dual_mode_read(adapter,
> > > + DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> > > + &data, 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
