On Thu, 2016-10-20 at 21:20 +0300, Jani Nikula wrote:
> On Thu, 20 Oct 2016, Imre Deak <[email protected]> wrote:
> > On my APL the LSPCON firmware resumes in PCON mode as opposed to the
> > expected LS mode. It also appears to be in a state where AUX DPCD reads
> > will succeed but return garbage recovering only after a few hundreds of
> > milliseconds. After the recovery time DPCD reads will result in the
> > correct values and things will continue to work. If I2C over AUX is
> > attempted during this recovery time (implying an AUX write transaction)
> > the firmware won't recover and will stay in this broken state.
> >
> > As a workaround check if the firmware is in PCON state after resume and
> > if so wait until the correct DPCD values are returned. For this we
> > compare the branch descriptor with the one we cached during init time.
> > If the firmware was in the LS state, we skip the w/a and continue as
> > before.
> >
> > Cc: Shashank Sharma <[email protected]>
> > Cc: Ville Syrjälä <[email protected]>
> > Cc: Jani Nikula <[email protected]>
> > Signed-off-by: Imre Deak <[email protected]>
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 2 +-
> > drivers/gpu/drm/i915/intel_drv.h | 6 ++++-
> > drivers/gpu/drm/i915/intel_lspcon.c | 52
> > ++++++++++++++++++++++++++++++-------
> > 3 files changed, 48 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index e90211e..ec031db 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3487,7 +3487,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
> > intel_dp->DP = DP;
> > }
> >
> > -static bool
> > +bool
> > intel_dp_read_dpcd(struct intel_dp *intel_dp)
> > {
> > if (drm_dp_dpcd_read(&intel_dp->aux, 0x000, intel_dp->dpcd,
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index a35e241..9a2366e 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -972,7 +972,9 @@ struct intel_dp {
> > struct intel_lspcon {
> > bool active;
> > enum drm_lspcon_mode mode;
> > - struct drm_dp_aux *aux;
> > + struct intel_dp *intel_dp;
> > + bool desc_valid;
> > + struct intel_dp_desc desc;
>
> I guess we could cache the desc in intel_dp directly. Independent of
> this patch.
It's not used anywhere else, but I can move it to intel_dp.
>
> Also, I'm wondering if we could stick with just aux here, and read
> something else from dpcd instead.
Not sure either, I picked desc since we read it out anyway during init.
>
> > };
> >
> > struct intel_digital_port {
> > @@ -1469,6 +1471,8 @@ static inline unsigned int
> > intel_dp_unused_lane_mask(int lane_count)
> > }
> >
> > bool
> > +intel_dp_read_dpcd(struct intel_dp *intel_dp);
> > +bool
> > intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc);
> > void
> > intel_dp_print_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc);
> > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c
> > b/drivers/gpu/drm/i915/intel_lspcon.c
> > index d2c8cb2..54c6173 100644
> > --- a/drivers/gpu/drm/i915/intel_lspcon.c
> > +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> > @@ -30,7 +30,7 @@
> > static enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon
> > *lspcon)
> > {
> > enum drm_lspcon_mode current_mode = DRM_LSPCON_MODE_INVALID;
> > - struct i2c_adapter *adapter = &lspcon->aux->ddc;
> > + struct i2c_adapter *adapter = &lspcon->intel_dp->aux.ddc;
> >
> > if (drm_lspcon_get_mode(adapter, ¤t_mode))
> > DRM_ERROR("Error reading LSPCON mode\n");
> > @@ -45,7 +45,7 @@ static int lspcon_change_mode(struct intel_lspcon *lspcon,
> > {
> > int err;
> > enum drm_lspcon_mode current_mode;
> > - struct i2c_adapter *adapter = &lspcon->aux->ddc;
> > + struct i2c_adapter *adapter = &lspcon->intel_dp->aux.ddc;
> >
> > err = drm_lspcon_get_mode(adapter, ¤t_mode);
> > if (err) {
> > @@ -72,7 +72,7 @@ static int lspcon_change_mode(struct intel_lspcon *lspcon,
> > static bool lspcon_probe(struct intel_lspcon *lspcon)
> > {
> > enum drm_dp_dual_mode_type adaptor_type;
> > - struct i2c_adapter *adapter = &lspcon->aux->ddc;
> > + struct i2c_adapter *adapter = &lspcon->intel_dp->aux.ddc;
> >
> > /* Lets probe the adaptor and check its type */
> > adaptor_type = drm_dp_dual_mode_detect(adapter);
> > @@ -89,8 +89,42 @@ static bool lspcon_probe(struct intel_lspcon *lspcon)
> > return true;
> > }
> >
> > +static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
> > +{
> > + unsigned long start = jiffies;
> > +
> > + if (!lspcon->desc_valid)
> > + return;
> > +
> > + while (1) {
> > + struct intel_dp_desc desc;
> > +
> > + /*
> > + * The w/a only applies in PCON mode and we don't expect any
> > + * AUX errors.
> > + */
> > + if (!intel_dp_read_desc(lspcon->intel_dp, &desc))
> > + return;
> > +
> > + if (!memcmp(&lspcon->desc, &desc, sizeof(desc))) {
> > + DRM_DEBUG_KMS("LSPCON recovering in PCON mode after %u
> > ms\n",
> > + jiffies_to_msecs(jiffies - start));
> > + return;
> > + }
> > +
> > + if (time_after(jiffies, start + msecs_to_jiffies(1000)))
> > + break;
> > +
> > + msleep(10);
> > + }
> > +
> > + DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
> > +}
>
> I think we need to go through the LSPCON spec once more before we merge
> this. Maybe we don't reset the LSPCON properly. Maybe we don't handle
> the resume properly.
I haven't found at least any way to reset things. I also tried to
change to LS mode when suspending or switch here to LS mode first and
only then to PCON mode but these didn't help.
--Imre
> Maybe this isn't a "workaround" after all.
>
> BR,
> Jani.
>
> > +
> > void lspcon_resume(struct intel_lspcon *lspcon)
> > {
> > + lspcon_resume_in_pcon_wa(lspcon);
> > +
> > if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON, true))
> > DRM_ERROR("LSPCON resume failed\n");
> > else
> > @@ -111,7 +145,7 @@ bool lspcon_init(struct intel_digital_port
> > *intel_dig_port)
> >
> > lspcon->active = false;
> > lspcon->mode = DRM_LSPCON_MODE_INVALID;
> > - lspcon->aux = &dp->aux;
> > + lspcon->intel_dp = dp;
> >
> > if (!lspcon_probe(lspcon)) {
> > DRM_ERROR("Failed to probe lspcon\n");
> > @@ -131,12 +165,10 @@ bool lspcon_init(struct intel_digital_port
> > *intel_dig_port)
> > }
> > }
> >
> > - if (drm_debug & DRM_UT_KMS) {
> > - struct intel_dp_desc desc;
> > -
> > - if (intel_dp_read_desc(dp, &desc))
> > - intel_dp_print_desc(dp, &desc);
> > - }
> > + lspcon->desc_valid = intel_dp_read_dpcd(dp) &&
> > + intel_dp_read_desc(dp, &lspcon->desc);
> > + if ((drm_debug & DRM_UT_KMS) && lspcon->desc_valid)
> > + intel_dp_print_desc(dp, &lspcon->desc);
> >
> > DRM_DEBUG_KMS("Success: LSPCON init\n");
> > return true;
>
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
