R-B: Shashank Sharma <shashank.sharma@xxxxxxxxx>
Regards
Shashank
-----Original Message-----
From: Deak, Imre
Sent: Tuesday, November 22, 2016 9:47 PM
To: Sharma, Shashank <shashank.sharma@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>; Nikula, Jani <jani.nikula@xxxxxxxxx>
Subject: Re: [PATCH 3/4] drm/i915/lspcon: Wait for expected LSPCON mode to settle
On Tue, 2016-11-22 at 21:06 +0530, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 11/22/2016 12:45 AM, Imre Deak wrote:
> > Some LSPCON adaptors may return an incorrect LSPCON mode right after
> > waking from DP Sleep state. This is the case at least for the
> > ParadTech
> > PS175 adaptor, both when waking because of exiting the DP Sleep to
> > active state, or due to any other AUX CH transfer. We can determine
> > the current expected mode based on whether the DPCD area is
> > accessible, since according to the LSPCON spec this area is only
> > accesible in PCON mode.
> >
> > This wait will avoid us trying to change the mode, while the current
> > expected mode hasn't settled yet and start link training before the
> > adaptor thinks it's in PCON mode after waking from DP Sleep state.
> >
> > Cc: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 5 +++
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_lspcon.c | 70
> > ++++++++++++++++++++++++++++++++-----
> > 3 files changed, 68 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c index 16c19d78..8026a83 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2401,6 +2401,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
> > ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> > DP_SET_POWER_D3);
> > } else {
> > + struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
> > +
> > /*
> > * When turning on, we need to retry for 1ms to give the sink
> > * time to wake up.
> > @@ -2412,6 +2414,9 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
> > break;
> > msleep(1);
> > }
> > +
> > + if (ret == 1 && lspcon->active)
> > + lspcon_wait_pcon_mode(lspcon);
> > }
> >
> > if (ret != 1)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index cf47e8a..d165904 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1830,4 +1830,5 @@ void intel_color_load_luts(struct
> > drm_crtc_state *crtc_state);
> > /* intel_lspcon.c */
> > bool lspcon_init(struct intel_digital_port *intel_dig_port);
> > void lspcon_resume(struct intel_lspcon *lspcon);
> > +void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
> > #endif /* __INTEL_DRV_H__ */
> > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c
> > b/drivers/gpu/drm/i915/intel_lspcon.c
> > index 5013124..281127d 100644
> > --- a/drivers/gpu/drm/i915/intel_lspcon.c
> > +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> > @@ -35,16 +35,54 @@ static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
> > return &dig_port->dp;
> > }
> >
> > +static const char *lspcon_mode_name(enum drm_lspcon_mode mode) {
> > + switch (mode) {
> > + case DRM_LSPCON_MODE_PCON:
> > + return "PCON";
> > + case DRM_LSPCON_MODE_LS:
> > + return "LS";
> > + case DRM_LSPCON_MODE_INVALID:
> > + return "INVALID";
> > + default:
> > + MISSING_CASE(mode);
> > + return "INVALID";
> > + }
> > +}
> > +
> > static enum drm_lspcon_mode lspcon_get_current_mode(struct
> > intel_lspcon *lspcon)
> > {
> > - enum drm_lspcon_mode current_mode = DRM_LSPCON_MODE_INVALID;
> > + enum drm_lspcon_mode current_mode;
> > struct i2c_adapter *adapter =
> > &lspcon_to_intel_dp(lspcon)->aux.ddc;
> >
> > - if (drm_lspcon_get_mode(adapter, ¤t_mode))
> > + if (drm_lspcon_get_mode(adapter, ¤t_mode)) {
> > DRM_ERROR("Error reading LSPCON mode\n");
> > - else
> > - DRM_DEBUG_KMS("Current LSPCON mode %s\n",
> > - current_mode == DRM_LSPCON_MODE_PCON ? "PCON" : "LS");
> > + return DRM_LSPCON_MODE_INVALID;
> > + }
> > + return current_mode;
> > +}
> > +
> > +static enum drm_lspcon_mode lspcon_wait_mode(struct intel_lspcon *lspcon,
> > + enum drm_lspcon_mode mode) {
> > + enum drm_lspcon_mode current_mode;
> > +
> > + current_mode = lspcon_get_current_mode(lspcon);
> > + if (current_mode == mode || current_mode == DRM_LSPCON_MODE_INVALID)
> > + goto out;
> > +
> > + DRM_DEBUG_KMS("Waiting for LSPCON mode %s to settle\n",
> > + lspcon_mode_name(mode));
> > +
> Can we remove above lines, and start from below lines ? I guess
> wait_for checks the condition first and then executes wait ?
Yes wait_for() works like that, but I wanted to have a debug message about the fact that we had to wait.
> Also, a comment stating 100ms wait ?
Why? It's clear what's the timeout from the code itself.
> - Shashank
> > + wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode ||
> > + current_mode == DRM_LSPCON_MODE_INVALID, 100);
> > + if (current_mode != mode)
> > + DRM_DEBUG_KMS("LSPCON mode hasn't settled\n");
> > +
> > +out:
> > + DRM_DEBUG_KMS("Current LSPCON mode %s\n",
> > + lspcon_mode_name(current_mode));
> > +
> > return current_mode;
> > }
> >
> > @@ -97,8 +135,10 @@ static bool lspcon_probe(struct intel_lspcon
> > *lspcon)
> > {
> > enum drm_dp_dual_mode_type adaptor_type;
> > struct i2c_adapter *adapter =
> > &lspcon_to_intel_dp(lspcon)->aux.ddc;
> > + enum drm_lspcon_mode expected_mode;
> >
> > - lspcon_wake_native_aux_ch(lspcon);
> > + expected_mode = lspcon_wake_native_aux_ch(lspcon) ?
> > + DRM_LSPCON_MODE_PCON : DRM_LSPCON_MODE_LS;
> >
> > /* Lets probe the adaptor and check its type */
> > adaptor_type = drm_dp_dual_mode_detect(adapter); @@ -110,7 +150,7
> > @@ static bool lspcon_probe(struct intel_lspcon *lspcon)
> >
> > /* Yay ... got a LSPCON device */
> > DRM_DEBUG_KMS("LSPCON detected\n");
> > - lspcon->mode = lspcon_get_current_mode(lspcon);
> > + lspcon->mode = lspcon_wait_mode(lspcon, expected_mode);
> > lspcon->active = true;
> > return true;
> > }
> > @@ -150,8 +190,17 @@ static void lspcon_resume_in_pcon_wa(struct
> > intel_lspcon *lspcon)
> >
> > void lspcon_resume(struct intel_lspcon *lspcon)
> > {
> > - if (lspcon_wake_native_aux_ch(lspcon))
> > + enum drm_lspcon_mode expected_mode;
> > +
> > + if (lspcon_wake_native_aux_ch(lspcon)) {
> > + expected_mode = DRM_LSPCON_MODE_PCON;
> > lspcon_resume_in_pcon_wa(lspcon);
> > + } else {
> > + expected_mode = DRM_LSPCON_MODE_LS;
> > + }
> > +
> > + if (lspcon_wait_mode(lspcon, expected_mode) == DRM_LSPCON_MODE_PCON)
> > + return;
> >
> > if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON, true))
> > DRM_ERROR("LSPCON resume failed\n"); @@ -159,6 +208,11 @@ void
> > lspcon_resume(struct intel_lspcon *lspcon)
> > DRM_DEBUG_KMS("LSPCON resume success\n");
> > }
> >
> > +void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon) {
> > + lspcon_wait_mode(lspcon, DRM_LSPCON_MODE_PCON); }
> > +
> > bool lspcon_init(struct intel_digital_port *intel_dig_port)
> > {
> > struct intel_dp *dp = &intel_dig_port->dp;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
