* [PATCH 1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs
@ 2016-10-20 17:06 Imre Deak
2016-10-20 17:06 ` [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode Imre Deak
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Imre Deak @ 2016-10-20 17:06 UTC (permalink / raw)
To: intel-gfx
Extend the branch/sink descriptor info with the missing device ID
field and print this info for eDP and LSPCON connectors too.
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 83 +++++++++++++++----------------------
drivers/gpu/drm/i915/intel_drv.h | 13 ++++++
drivers/gpu/drm/i915/intel_lspcon.c | 7 ++++
3 files changed, 53 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 88f3b74..e90211e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1442,42 +1442,34 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
DRM_DEBUG_KMS("common rates: %s\n", str);
}
-static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
+static bool intel_dp_is_branch(struct intel_dp *intel_dp)
{
- uint8_t rev;
- int len;
-
- if ((drm_debug & DRM_UT_KMS) == 0)
- return;
-
- if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
- DP_DWN_STRM_PORT_PRESENT))
- return;
-
- len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
- if (len < 0)
- return;
-
- DRM_DEBUG_KMS("sink hw revision: %d.%d\n", (rev & 0xf0) >> 4, rev & 0xf);
+ return intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
+ DP_DWN_STRM_PORT_PRESENT;
}
-static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
+bool
+intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
{
- uint8_t rev[2];
- int len;
+ u32 base = intel_dp_is_branch(intel_dp) ? DP_BRANCH_OUI : DP_SINK_OUI;
- if ((drm_debug & DRM_UT_KMS) == 0)
- return;
+ return drm_dp_dpcd_read(&intel_dp->aux, base, desc, sizeof(*desc)) ==
+ sizeof(*desc);
+}
- if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
- DP_DWN_STRM_PORT_PRESENT))
- return;
+void
+intel_dp_print_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
+{
+ const char *dev_type = intel_dp_is_branch(intel_dp) ? "branch" : "sink";
+ bool oui_sup = intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
+ DP_OUI_SUPPORT;
- len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
- if (len < 0)
- return;
-
- DRM_DEBUG_KMS("sink sw revision: %d.%d\n", rev[0], rev[1]);
+ DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %.*s HW-rev %d.%d SW-rev %d.%d\n",
+ dev_type,
+ (int)sizeof(desc->oui), desc->oui, oui_sup ? "" : "(NS)",
+ (int)sizeof(desc->device_id), desc->device_id,
+ (desc->hw_rev & 0xf0) >> 4, (desc->hw_rev & 0xf),
+ desc->sw_major_rev, desc->sw_minor_rev);
}
static int rate_to_index(int find, const int *rates)
@@ -3519,6 +3511,13 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
if (!intel_dp_read_dpcd(intel_dp))
return false;
+ if (drm_debug & DRM_UT_KMS) {
+ struct intel_dp_desc desc;
+
+ if (intel_dp_read_desc(intel_dp, &desc))
+ intel_dp_print_desc(intel_dp, &desc);
+ }
+
if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11)
dev_priv->no_aux_handshake = intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
DP_NO_AUX_HANDSHAKE_LINK_TRAINING;
@@ -3621,23 +3620,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
return true;
}
-static void
-intel_dp_probe_oui(struct intel_dp *intel_dp)
-{
- u8 buf[3];
-
- if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
- return;
-
- if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
- DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
- buf[0], buf[1], buf[2]);
-
- if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
- DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
- buf[0], buf[1], buf[2]);
-}
-
static bool
intel_dp_can_mst(struct intel_dp *intel_dp)
{
@@ -4410,11 +4392,12 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
yesno(drm_dp_tps3_supported(intel_dp->dpcd)));
intel_dp_print_rates(intel_dp);
+ if (drm_debug & DRM_UT_KMS) {
+ struct intel_dp_desc desc;
- intel_dp_probe_oui(intel_dp);
-
- intel_dp_print_hw_revision(intel_dp);
- intel_dp_print_sw_revision(intel_dp);
+ if (intel_dp_read_desc(intel_dp, &desc))
+ intel_dp_print_desc(intel_dp, &desc);
+ }
intel_dp_configure_mst(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c06a33e..a35e241 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -883,6 +883,14 @@ enum link_m_n_set {
M2_N2
};
+struct intel_dp_desc {
+ u8 oui[3];
+ u8 device_id[6];
+ u8 hw_rev;
+ u8 sw_major_rev;
+ u8 sw_minor_rev;
+} __packed;
+
struct intel_dp {
i915_reg_t output_reg;
i915_reg_t aux_ch_ctl_reg;
@@ -1460,6 +1468,11 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
return ~((1 << lane_count) - 1) & 0xf;
}
+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);
+
/* intel_dp_aux_backlight.c */
int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 632149c..d2c8cb2 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -131,6 +131,13 @@ 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);
+ }
+
DRM_DEBUG_KMS("Success: LSPCON init\n");
return true;
}
--
2.5.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode
2016-10-20 17:06 [PATCH 1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs Imre Deak
@ 2016-10-20 17:06 ` Imre Deak
2016-10-20 18:20 ` Jani Nikula
2016-10-20 17:47 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs Patchwork
2016-10-20 18:06 ` [PATCH 1/2] " Jani Nikula
2 siblings, 1 reply; 18+ messages in thread
From: Imre Deak @ 2016-10-20 17:06 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula
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 <shashank.sharma@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
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;
};
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");
+}
+
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;
--
2.5.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs
2016-10-20 17:06 [PATCH 1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs Imre Deak
2016-10-20 17:06 ` [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode Imre Deak
@ 2016-10-20 17:47 ` Patchwork
2016-10-20 18:06 ` [PATCH 1/2] " Jani Nikula
2 siblings, 0 replies; 18+ messages in thread
From: Patchwork @ 2016-10-20 17:47 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs
URL : https://patchwork.freedesktop.org/series/14123/
State : success
== Summary ==
Series 14123v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/14123/revisions/1/mbox/
Test drv_module_reload_basic:
dmesg-warn -> PASS (fi-skl-6700hq)
fi-bdw-5557u total:246 pass:231 dwarn:0 dfail:0 fail:0 skip:15
fi-bsw-n3050 total:246 pass:204 dwarn:0 dfail:0 fail:0 skip:42
fi-bxt-t5700 total:246 pass:216 dwarn:0 dfail:0 fail:0 skip:30
fi-byt-j1900 total:246 pass:215 dwarn:0 dfail:0 fail:0 skip:31
fi-byt-n2820 total:246 pass:211 dwarn:0 dfail:0 fail:0 skip:35
fi-hsw-4770 total:246 pass:224 dwarn:0 dfail:0 fail:0 skip:22
fi-hsw-4770r total:246 pass:224 dwarn:0 dfail:0 fail:0 skip:22
fi-ilk-650 total:246 pass:185 dwarn:0 dfail:0 fail:1 skip:60
fi-ivb-3520m total:246 pass:221 dwarn:0 dfail:0 fail:0 skip:25
fi-ivb-3770 total:246 pass:221 dwarn:0 dfail:0 fail:0 skip:25
fi-kbl-7200u total:246 pass:222 dwarn:0 dfail:0 fail:0 skip:24
fi-skl-6260u total:246 pass:232 dwarn:0 dfail:0 fail:0 skip:14
fi-skl-6700hq total:246 pass:223 dwarn:0 dfail:0 fail:0 skip:23
fi-skl-6700k total:246 pass:221 dwarn:1 dfail:0 fail:0 skip:24
fi-skl-6770hq total:246 pass:231 dwarn:0 dfail:0 fail:0 skip:15
fi-snb-2520m total:246 pass:210 dwarn:0 dfail:0 fail:0 skip:36
fi-snb-2600 total:246 pass:209 dwarn:0 dfail:0 fail:0 skip:37
Results at /archive/results/CI_IGT_test/Patchwork_2778/
5113d7495dab3ea4d14a7698368c6be80f6c045c drm-intel-nightly: 2016y-10m-20d-13h-31m-16s UTC integration manifest
c3e5a2c drm/i915/lspcon: Add workaround for resuming in PCON mode
e8e46ae drm/i915/dp: Print full branch/sink descriptor for all outputs
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs
2016-10-20 17:06 [PATCH 1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs Imre Deak
2016-10-20 17:06 ` [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode Imre Deak
2016-10-20 17:47 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs Patchwork
@ 2016-10-20 18:06 ` Jani Nikula
2016-10-20 18:58 ` Imre Deak
2016-10-24 8:36 ` Daniel Vetter
2 siblings, 2 replies; 18+ messages in thread
From: Jani Nikula @ 2016-10-20 18:06 UTC (permalink / raw)
To: Imre Deak, intel-gfx
On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> Extend the branch/sink descriptor info with the missing device ID
> field and print this info for eDP and LSPCON connectors too.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 83 +++++++++++++++----------------------
> drivers/gpu/drm/i915/intel_drv.h | 13 ++++++
> drivers/gpu/drm/i915/intel_lspcon.c | 7 ++++
> 3 files changed, 53 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 88f3b74..e90211e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1442,42 +1442,34 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
> DRM_DEBUG_KMS("common rates: %s\n", str);
> }
>
> -static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
> +static bool intel_dp_is_branch(struct intel_dp *intel_dp)
Belongs in drm dp helpers.
> {
> - uint8_t rev;
> - int len;
> -
> - if ((drm_debug & DRM_UT_KMS) == 0)
> - return;
> -
> - if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> - DP_DWN_STRM_PORT_PRESENT))
> - return;
> -
> - len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
> - if (len < 0)
> - return;
> -
> - DRM_DEBUG_KMS("sink hw revision: %d.%d\n", (rev & 0xf0) >> 4, rev & 0xf);
> + return intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> + DP_DWN_STRM_PORT_PRESENT;
> }
>
> -static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
> +bool
> +intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
> {
> - uint8_t rev[2];
> - int len;
> + u32 base = intel_dp_is_branch(intel_dp) ? DP_BRANCH_OUI : DP_SINK_OUI;
>
> - if ((drm_debug & DRM_UT_KMS) == 0)
> - return;
> + return drm_dp_dpcd_read(&intel_dp->aux, base, desc, sizeof(*desc)) ==
> + sizeof(*desc);
Starting to read either branch or sink oui should be a standalone prep
change. I guess this should be done, although I've seen crappy devices
that report oui in wrong place...
> +}
>
> - if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> - DP_DWN_STRM_PORT_PRESENT))
> - return;
> +void
> +intel_dp_print_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
> +{
> + const char *dev_type = intel_dp_is_branch(intel_dp) ? "branch" : "sink";
> + bool oui_sup = intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> + DP_OUI_SUPPORT;
>
> - len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
> - if (len < 0)
> - return;
> -
> - DRM_DEBUG_KMS("sink sw revision: %d.%d\n", rev[0], rev[1]);
> + DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %.*s HW-rev %d.%d SW-rev %d.%d\n",
> + dev_type,
> + (int)sizeof(desc->oui), desc->oui, oui_sup ? "" : "(NS)",
> + (int)sizeof(desc->device_id), desc->device_id,
> + (desc->hw_rev & 0xf0) >> 4, (desc->hw_rev & 0xf),
> + desc->sw_major_rev, desc->sw_minor_rev);
I've been thinking about starting to print this stuff too. But again,
could be a standalone change.
> }
>
> static int rate_to_index(int find, const int *rates)
> @@ -3519,6 +3511,13 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
> if (!intel_dp_read_dpcd(intel_dp))
> return false;
>
> + if (drm_debug & DRM_UT_KMS) {
> + struct intel_dp_desc desc;
> +
> + if (intel_dp_read_desc(intel_dp, &desc))
> + intel_dp_print_desc(intel_dp, &desc);
> + }
I *really* don't think we should do dpcd access conditional to drm
debugs. I smell heisenbugs just thinking about it.
> +
> if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11)
> dev_priv->no_aux_handshake = intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
> DP_NO_AUX_HANDSHAKE_LINK_TRAINING;
> @@ -3621,23 +3620,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> return true;
> }
>
> -static void
> -intel_dp_probe_oui(struct intel_dp *intel_dp)
> -{
> - u8 buf[3];
> -
> - if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
> - return;
> -
> - if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
> - DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
> - buf[0], buf[1], buf[2]);
> -
> - if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
> - DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
> - buf[0], buf[1], buf[2]);
> -}
> -
> static bool
> intel_dp_can_mst(struct intel_dp *intel_dp)
> {
> @@ -4410,11 +4392,12 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> yesno(drm_dp_tps3_supported(intel_dp->dpcd)));
>
> intel_dp_print_rates(intel_dp);
> + if (drm_debug & DRM_UT_KMS) {
> + struct intel_dp_desc desc;
>
> - intel_dp_probe_oui(intel_dp);
> -
> - intel_dp_print_hw_revision(intel_dp);
> - intel_dp_print_sw_revision(intel_dp);
> + if (intel_dp_read_desc(intel_dp, &desc))
> + intel_dp_print_desc(intel_dp, &desc);
> + }
Ditto. I do like putting the desc reading into one place though.
>
> intel_dp_configure_mst(intel_dp);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c06a33e..a35e241 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -883,6 +883,14 @@ enum link_m_n_set {
> M2_N2
> };
>
> +struct intel_dp_desc {
> + u8 oui[3];
> + u8 device_id[6];
> + u8 hw_rev;
> + u8 sw_major_rev;
> + u8 sw_minor_rev;
> +} __packed;
> +
I understand this, but makes me wonder about dp helpers vs. i915. I
guess we could add this for now.
> struct intel_dp {
> i915_reg_t output_reg;
> i915_reg_t aux_ch_ctl_reg;
> @@ -1460,6 +1468,11 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
> return ~((1 << lane_count) - 1) & 0xf;
> }
>
> +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);
> +
> /* intel_dp_aux_backlight.c */
> int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
>
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index 632149c..d2c8cb2 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -131,6 +131,13 @@ 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);
> + }
Again, reads conditional to debugs is scary.
> +
> DRM_DEBUG_KMS("Success: LSPCON init\n");
> return true;
> }
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode
2016-10-20 17:06 ` [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode Imre Deak
@ 2016-10-20 18:20 ` Jani Nikula
2016-10-20 19:20 ` Imre Deak
2016-10-20 19:24 ` Jani Nikula
0 siblings, 2 replies; 18+ messages in thread
From: Jani Nikula @ 2016-10-20 18:20 UTC (permalink / raw)
To: Imre Deak, intel-gfx
On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> 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 <shashank.sharma@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> 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.
Also, I'm wondering if we could stick with just aux here, and read
something else from dpcd instead.
> };
>
> 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. 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;
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs
2016-10-20 18:06 ` [PATCH 1/2] " Jani Nikula
@ 2016-10-20 18:58 ` Imre Deak
2016-10-24 8:36 ` Daniel Vetter
1 sibling, 0 replies; 18+ messages in thread
From: Imre Deak @ 2016-10-20 18:58 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
On Thu, 2016-10-20 at 21:06 +0300, Jani Nikula wrote:
> On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > Extend the branch/sink descriptor info with the missing device ID
> > field and print this info for eDP and LSPCON connectors too.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 83 +++++++++++++++----------------------
> > drivers/gpu/drm/i915/intel_drv.h | 13 ++++++
> > drivers/gpu/drm/i915/intel_lspcon.c | 7 ++++
> > 3 files changed, 53 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 88f3b74..e90211e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1442,42 +1442,34 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
> > DRM_DEBUG_KMS("common rates: %s\n", str);
> > }
> >
> > -static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
> > +static bool intel_dp_is_branch(struct intel_dp *intel_dp)
>
> Belongs in drm dp helpers.
Ok.
>
> > {
> > - uint8_t rev;
> > - int len;
> > -
> > - if ((drm_debug & DRM_UT_KMS) == 0)
> > - return;
> > -
> > - if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> > - DP_DWN_STRM_PORT_PRESENT))
> > - return;
> > -
> > - len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
> > - if (len < 0)
> > - return;
> > -
> > - DRM_DEBUG_KMS("sink hw revision: %d.%d\n", (rev & 0xf0) >> 4, rev & 0xf);
> > + return intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> > + DP_DWN_STRM_PORT_PRESENT;
> > }
> >
> > -static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
> > +bool
> > +intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
> > {
> > - uint8_t rev[2];
> > - int len;
> > + u32 base = intel_dp_is_branch(intel_dp) ? DP_BRANCH_OUI : DP_SINK_OUI;
> >
> > - if ((drm_debug & DRM_UT_KMS) == 0)
> > - return;
> > + return drm_dp_dpcd_read(&intel_dp->aux, base, desc, sizeof(*desc)) ==
> > + sizeof(*desc);
>
> Starting to read either branch or sink oui should be a standalone prep
> change. I guess this should be done, although I've seen crappy devices
> that report oui in wrong place...
Ok, can make that a separate change.
>
> > +}
> >
> > - if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> > - DP_DWN_STRM_PORT_PRESENT))
> > - return;
> > +void
> > +intel_dp_print_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
> > +{
> > + const char *dev_type = intel_dp_is_branch(intel_dp) ? "branch" : "sink";
> > + bool oui_sup = intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> > + DP_OUI_SUPPORT;
> >
> > - len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
> > - if (len < 0)
> > - return;
> > -
> > - DRM_DEBUG_KMS("sink sw revision: %d.%d\n", rev[0], rev[1]);
> > + DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %.*s HW-rev %d.%d SW-rev %d.%d\n",
> > + dev_type,
> > + (int)sizeof(desc->oui), desc->oui, oui_sup ? "" : "(NS)",
> > + (int)sizeof(desc->device_id), desc->device_id,
> > + (desc->hw_rev & 0xf0) >> 4, (desc->hw_rev & 0xf),
> > + desc->sw_major_rev, desc->sw_minor_rev);
>
> I've been thinking about starting to print this stuff too. But again,
> could be a standalone change.
>
> > }
> >
> > static int rate_to_index(int find, const int *rates)
> > @@ -3519,6 +3511,13 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
> > if (!intel_dp_read_dpcd(intel_dp))
> > return false;
> >
> > + if (drm_debug & DRM_UT_KMS) {
> > + struct intel_dp_desc desc;
> > +
> > + if (intel_dp_read_desc(intel_dp, &desc))
> > + intel_dp_print_desc(intel_dp, &desc);
> > + }
>
> I *really* don't think we should do dpcd access conditional to drm
> debugs. I smell heisenbugs just thinking about it.
It's already conditional,
see intel_dp_print_hw_revision(), intel_dp_print_sw_revision(). But I
can make it unconditional.
>
> > +
> > if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11)
> > dev_priv->no_aux_handshake = intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
> > DP_NO_AUX_HANDSHAKE_LINK_TRAINING;
> > @@ -3621,23 +3620,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > return true;
> > }
> >
> > -static void
> > -intel_dp_probe_oui(struct intel_dp *intel_dp)
> > -{
> > - u8 buf[3];
> > -
> > - if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
> > - return;
> > -
> > - if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
> > - DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
> > - buf[0], buf[1], buf[2]);
> > -
> > - if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
> > - DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
> > - buf[0], buf[1], buf[2]);
> > -}
> > -
> > static bool
> > intel_dp_can_mst(struct intel_dp *intel_dp)
> > {
> > @@ -4410,11 +4392,12 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > yesno(drm_dp_tps3_supported(intel_dp->dpcd)));
> >
> > intel_dp_print_rates(intel_dp);
> > + if (drm_debug & DRM_UT_KMS) {
> > + struct intel_dp_desc desc;
> >
> > - intel_dp_probe_oui(intel_dp);
> > -
> > - intel_dp_print_hw_revision(intel_dp);
> > - intel_dp_print_sw_revision(intel_dp);
> > + if (intel_dp_read_desc(intel_dp, &desc))
> > + intel_dp_print_desc(intel_dp, &desc);
> > + }
>
> Ditto. I do like putting the desc reading into one place though.
>
> >
> > intel_dp_configure_mst(intel_dp);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index c06a33e..a35e241 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -883,6 +883,14 @@ enum link_m_n_set {
> > M2_N2
> > };
> >
> > +struct intel_dp_desc {
> > + u8 oui[3];
> > + u8 device_id[6];
> > + u8 hw_rev;
> > + u8 sw_major_rev;
> > + u8 sw_minor_rev;
> > +} __packed;
> > +
>
> I understand this, but makes me wonder about dp helpers vs. i915. I
> guess we could add this for now.
>
> > struct intel_dp {
> > i915_reg_t output_reg;
> > i915_reg_t aux_ch_ctl_reg;
> > @@ -1460,6 +1468,11 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
> > return ~((1 << lane_count) - 1) & 0xf;
> > }
> >
> > +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);
> > +
> > /* intel_dp_aux_backlight.c */
> > int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> > index 632149c..d2c8cb2 100644
> > --- a/drivers/gpu/drm/i915/intel_lspcon.c
> > +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> > @@ -131,6 +131,13 @@ 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);
> > + }
>
> Again, reads conditional to debugs is scary.
>
> > +
> > DRM_DEBUG_KMS("Success: LSPCON init\n");
> > return true;
> > }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode
2016-10-20 18:20 ` Jani Nikula
@ 2016-10-20 19:20 ` Imre Deak
2016-10-20 21:07 ` Jani Nikula
2016-10-21 5:46 ` Sharma, Shashank
2016-10-20 19:24 ` Jani Nikula
1 sibling, 2 replies; 18+ messages in thread
From: Imre Deak @ 2016-10-20 19:20 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
On Thu, 2016-10-20 at 21:20 +0300, Jani Nikula wrote:
> On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> 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 <shashank.sharma@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > 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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode
2016-10-20 18:20 ` Jani Nikula
2016-10-20 19:20 ` Imre Deak
@ 2016-10-20 19:24 ` Jani Nikula
2016-10-20 19:43 ` Imre Deak
1 sibling, 1 reply; 18+ messages in thread
From: Jani Nikula @ 2016-10-20 19:24 UTC (permalink / raw)
To: Imre Deak, intel-gfx
On Thu, 20 Oct 2016, Jani Nikula <jani.nikula@intel.com> wrote:
> On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> 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 <shashank.sharma@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>> ---
>> 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.
>
> Also, I'm wondering if we could stick with just aux here, and read
> something else from dpcd instead.
>
>> };
>>
>> 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. Maybe this isn't a "workaround" after all.
Looks like this stuff is normal behaviour if the LSPCON has been in low
power state, and we need to wake it up. In PCON mode, this is done using
DP_SET_POWER. And while waking up, it'll issue AUX DEFER, as we can see
from the logs.
We need to do something, but I don't think this patch is the right fix.
BR,
Jani.
>
> 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;
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode
2016-10-20 19:24 ` Jani Nikula
@ 2016-10-20 19:43 ` Imre Deak
2016-10-20 20:50 ` Jani Nikula
0 siblings, 1 reply; 18+ messages in thread
From: Imre Deak @ 2016-10-20 19:43 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
On Thu, 2016-10-20 at 22:24 +0300, Jani Nikula wrote:
> On Thu, 20 Oct 2016, Jani Nikula <jani.nikula@intel.com> wrote:
> > On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> 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 <shashank.sharma@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > > 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.
> >
> > Also, I'm wondering if we could stick with just aux here, and read
> > something else from dpcd instead.
> >
> > > };
> > >
> > > 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. Maybe this isn't a "workaround" after all.
>
> Looks like this stuff is normal behaviour if the LSPCON has been in low
> power state, and we need to wake it up. In PCON mode, this is done using
> DP_SET_POWER. And while waking up, it'll issue AUX DEFER, as we can see
> from the logs.
I did think about that, but waking the device via DP_SET_POWER(D0)
didn't help. Doing any AUX write transactions actually gets the device
into a stuck state for good (until next reboot on APL).
More over, we don't get AUX DEFERs the AUX transactions succeed, it's
only that reads will return corrupted values for the given time period.
--Imre
>
> We need to do something, but I don't think this patch is the right fix.
>
> BR,
> Jani.
>
>
>
> >
> > 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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode
2016-10-20 19:43 ` Imre Deak
@ 2016-10-20 20:50 ` Jani Nikula
2016-10-20 21:40 ` Imre Deak
0 siblings, 1 reply; 18+ messages in thread
From: Jani Nikula @ 2016-10-20 20:50 UTC (permalink / raw)
To: imre.deak, intel-gfx
On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> On Thu, 2016-10-20 at 22:24 +0300, Jani Nikula wrote:
>> On Thu, 20 Oct 2016, Jani Nikula <jani.nikula@intel.com> wrote:
>> > On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> 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 <shashank.sharma@intel.com>
>> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > Cc: Jani Nikula <jani.nikula@intel.com>
>> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> > > ---
>> > > 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.
>> >
>> > Also, I'm wondering if we could stick with just aux here, and read
>> > something else from dpcd instead.
>> >
>> > > };
>> > >
>> > > 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. Maybe this isn't a "workaround" after all.
>>
>> Looks like this stuff is normal behaviour if the LSPCON has been in low
>> power state, and we need to wake it up. In PCON mode, this is done using
>> DP_SET_POWER. And while waking up, it'll issue AUX DEFER, as we can see
>> from the logs.
>
> I did think about that, but waking the device via DP_SET_POWER(D0)
> didn't help. Doing any AUX write transactions actually gets the device
> into a stuck state for good (until next reboot on APL).
>
> More over, we don't get AUX DEFERs the AUX transactions succeed, it's
> only that reads will return corrupted values for the given time period.
The bug at hand has native defers
https://bugs.freedesktop.org/show_bug.cgi?id=98353
BR,
Jani.
>
> --Imre
>
>>
>> We need to do something, but I don't think this patch is the right fix.
>>
>> BR,
>> Jani.
>>
>>
>>
>> >
>> > 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;
>>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode
2016-10-20 19:20 ` Imre Deak
@ 2016-10-20 21:07 ` Jani Nikula
2016-10-21 5:46 ` Sharma, Shashank
1 sibling, 0 replies; 18+ messages in thread
From: Jani Nikula @ 2016-10-20 21:07 UTC (permalink / raw)
To: imre.deak, intel-gfx
On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> On Thu, 2016-10-20 at 21:20 +0300, Jani Nikula wrote:
>> On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
>> > + 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.
That was my point with putting it in intel_dp. If it's read out anyway,
lspcon or not, just cache it in intel_dp.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode
2016-10-20 20:50 ` Jani Nikula
@ 2016-10-20 21:40 ` Imre Deak
2016-10-21 7:20 ` Imre Deak
0 siblings, 1 reply; 18+ messages in thread
From: Imre Deak @ 2016-10-20 21:40 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
On Thu, 2016-10-20 at 23:50 +0300, Jani Nikula wrote:
> On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > On Thu, 2016-10-20 at 22:24 +0300, Jani Nikula wrote:
> > > On Thu, 20 Oct 2016, Jani Nikula <jani.nikula@intel.com> wrote:
> > > > On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> 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 <shashank.sharma@intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > ---
> > > > > 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.
> > > >
> > > > Also, I'm wondering if we could stick with just aux here, and read
> > > > something else from dpcd instead.
> > > >
> > > > > };
> > > > >
> > > > > 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. Maybe this isn't a "workaround" after all.
> > >
> > > Looks like this stuff is normal behaviour if the LSPCON has been in low
> > > power state, and we need to wake it up. In PCON mode, this is done using
> > > DP_SET_POWER. And while waking up, it'll issue AUX DEFER, as we can see
> > > from the logs.
> >
> > I did think about that, but waking the device via DP_SET_POWER(D0)
> > didn't help. Doing any AUX write transactions actually gets the device
> > into a stuck state for good (until next reboot on APL).
> >
> > More over, we don't get AUX DEFERs the AUX transactions succeed, it's
> > only that reads will return corrupted values for the given time period.
>
> The bug at hand has native defers
> https://bugs.freedesktop.org/show_bug.cgi?id=98353
For I2C-over-AUX I also get defers, but not for DPCD transactions for
native AUX registers for instance DP_SET_POWER or the OUI, device-ID
registers. I also tried the LS mode wake-up sequence via
I2C-over-AUX (offset 0x20), but didn't help either.
The bug above could be a different issue, although I did see this
particular scenario getting fixed on the same machine see
/archive/results/CI_IGT_test/Patchwork_2778/
--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode
2016-10-20 19:20 ` Imre Deak
2016-10-20 21:07 ` Jani Nikula
@ 2016-10-21 5:46 ` Sharma, Shashank
2016-10-21 7:40 ` Imre Deak
1 sibling, 1 reply; 18+ messages in thread
From: Sharma, Shashank @ 2016-10-21 5:46 UTC (permalink / raw)
To: imre.deak, Jani Nikula, intel-gfx
Regards
Shashank
On 10/21/2016 12:50 AM, Imre Deak wrote:
> On Thu, 2016-10-20 at 21:20 +0300, Jani Nikula wrote:
>> On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> 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 <shashank.sharma@intel.com>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>>> ---
>>> 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;
IMHO, Its not required to have the intel_dp inside lspcon. we can always
get intel_dig_port from lspcon, and intel_dp from intel_dig_port
The reason why I kept aux here was thats the only thing required to
read/write from/to lspcon.
>>> + 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
I agree with Imre. There is nothing like that in specs as such, and even
If this was something missing during the reset/power down time, we
should have seen this on all/most of the boards.
Regards
Shashank
>
>> 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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode
2016-10-20 21:40 ` Imre Deak
@ 2016-10-21 7:20 ` Imre Deak
2016-10-21 8:51 ` Jani Nikula
0 siblings, 1 reply; 18+ messages in thread
From: Imre Deak @ 2016-10-21 7:20 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
On Fri, 2016-10-21 at 00:40 +0300, Imre Deak wrote:
> On Thu, 2016-10-20 at 23:50 +0300, Jani Nikula wrote:
> > On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > > On Thu, 2016-10-20 at 22:24 +0300, Jani Nikula wrote:
> > > > On Thu, 20 Oct 2016, Jani Nikula <jani.nikula@intel.com> wrote:
> > > > > On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> 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 <shashank.sharma@intel.com>
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > ---
> > > > > > 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.
> > > > >
> > > > > Also, I'm wondering if we could stick with just aux here, and
> > > > > read
> > > > > something else from dpcd instead.
> > > > >
> > > > > > };
> > > > > >
> > > > > > 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(jif
> > > > > > fies - 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. Maybe this isn't a "workaround" after
> > > > > all.
> > > >
> > > > Looks like this stuff is normal behaviour if the LSPCON has
> > > > been in low
> > > > power state, and we need to wake it up. In PCON mode, this is
> > > > done using
> > > > DP_SET_POWER. And while waking up, it'll issue AUX DEFER, as we
> > > > can see
> > > > from the logs.
> > >
> > > I did think about that, but waking the device via
> > > DP_SET_POWER(D0)
> > > didn't help. Doing any AUX write transactions actually gets the
> > > device
> > > into a stuck state for good (until next reboot on APL).
> > >
> > > More over, we don't get AUX DEFERs the AUX transactions succeed,
> > > it's
> > > only that reads will return corrupted values for the given time
> > > period.
> >
> > The bug at hand has native defers
> > https://bugs.freedesktop.org/show_bug.cgi?id=98353
>
> For I2C-over-AUX I also get defers, but not for DPCD transactions for
> native AUX registers for instance DP_SET_POWER or the OUI, device-ID
> registers. I also tried the LS mode wake-up sequence via
> I2C-over-AUX (offset 0x20), but didn't help either.
>
> The bug above could be a different issue, although I did see this
> particular scenario getting fixed on the same machine see
> /archive/results/CI_IGT_test/Patchwork_2778/
I tried this now on the fi-skl-6700hq machine in the bug report and it
seems to be the same failure mode as the APL one. There is one
difference in that on the SKL machine once the firmware is in the stuck
state after suspend/resume, a warm reboot may not recover it, so during
the next boot the LSPCON probe will fail. I guess that during warm
reboot LSPCON may continue to be powered, so to fully reset it a power
cycle is needed. In any case this patch fixes resume on that machine
too, so I couldn't reproduce the problem with it even across multiple
reboots, whereas without it it's 100% reproducible.
--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode
2016-10-21 5:46 ` Sharma, Shashank
@ 2016-10-21 7:40 ` Imre Deak
0 siblings, 0 replies; 18+ messages in thread
From: Imre Deak @ 2016-10-21 7:40 UTC (permalink / raw)
To: Sharma, Shashank, Jani Nikula, intel-gfx
On Fri, 2016-10-21 at 11:16 +0530, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 10/21/2016 12:50 AM, Imre Deak wrote:
> > On Thu, 2016-10-20 at 21:20 +0300, Jani Nikula wrote:
> > > On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> 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 <shashank.sharma@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > > 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;
> IMHO, Its not required to have the intel_dp inside lspcon. we can always
> get intel_dig_port from lspcon, and intel_dp from intel_dig_port
Yea I missed that, so the above change isn't needed, I'll fix this.
> The reason why I kept aux here was thats the only thing required to
> read/write from/to lspcon.
> > > > + 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
> I agree with Imre. There is nothing like that in specs as such, and even
> If this was something missing during the reset/power down time, we
> should have seen this on all/most of the boards.
>
> Regards
> Shashank
> >
> > > 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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode
2016-10-21 7:20 ` Imre Deak
@ 2016-10-21 8:51 ` Jani Nikula
2016-10-21 9:07 ` Imre Deak
0 siblings, 1 reply; 18+ messages in thread
From: Jani Nikula @ 2016-10-21 8:51 UTC (permalink / raw)
To: imre.deak, intel-gfx
On Fri, 21 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> On Fri, 2016-10-21 at 00:40 +0300, Imre Deak wrote:
>> On Thu, 2016-10-20 at 23:50 +0300, Jani Nikula wrote:
>> > On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
>> > > On Thu, 2016-10-20 at 22:24 +0300, Jani Nikula wrote:
>> > > > On Thu, 20 Oct 2016, Jani Nikula <jani.nikula@intel.com> wrote:
>> > > > > On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> 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 <shashank.sharma@intel.com>
>> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > > > > Cc: Jani Nikula <jani.nikula@intel.com>
>> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> > > > > > ---
>> > > > > > 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.
>> > > > >
>> > > > > Also, I'm wondering if we could stick with just aux here, and
>> > > > > read
>> > > > > something else from dpcd instead.
>> > > > >
>> > > > > > };
>> > > > > >
>> > > > > > 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(jif
>> > > > > > fies - 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. Maybe this isn't a "workaround" after
>> > > > > all.
>> > > >
>> > > > Looks like this stuff is normal behaviour if the LSPCON has
>> > > > been in low
>> > > > power state, and we need to wake it up. In PCON mode, this is
>> > > > done using
>> > > > DP_SET_POWER. And while waking up, it'll issue AUX DEFER, as we
>> > > > can see
>> > > > from the logs.
>> > >
>> > > I did think about that, but waking the device via
>> > > DP_SET_POWER(D0)
>> > > didn't help. Doing any AUX write transactions actually gets the
>> > > device
>> > > into a stuck state for good (until next reboot on APL).
>> > >
>> > > More over, we don't get AUX DEFERs the AUX transactions succeed,
>> > > it's
>> > > only that reads will return corrupted values for the given time
>> > > period.
>> >
>> > The bug at hand has native defers
>> > https://bugs.freedesktop.org/show_bug.cgi?id=98353
>>
>> For I2C-over-AUX I also get defers, but not for DPCD transactions for
>> native AUX registers for instance DP_SET_POWER or the OUI, device-ID
>> registers. I also tried the LS mode wake-up sequence via
>> I2C-over-AUX (offset 0x20), but didn't help either.
>>
>> The bug above could be a different issue, although I did see this
>> particular scenario getting fixed on the same machine see
>> /archive/results/CI_IGT_test/Patchwork_2778/
>
> I tried this now on the fi-skl-6700hq machine in the bug report and it
> seems to be the same failure mode as the APL one. There is one
> difference in that on the SKL machine once the firmware is in the stuck
> state after suspend/resume, a warm reboot may not recover it, so during
> the next boot the LSPCON probe will fail. I guess that during warm
> reboot LSPCON may continue to be powered, so to fully reset it a power
> cycle is needed. In any case this patch fixes resume on that machine
> too, so I couldn't reproduce the problem with it even across multiple
> reboots, whereas without it it's 100% reproducible.
A quick thought, when we do module unload or suspend, perhaps we should
put the LSPCON to the state in which we expect to find it on
probe/resume? In case it doesn't get reset.
BR,
Jani.
>
> --Imre
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode
2016-10-21 8:51 ` Jani Nikula
@ 2016-10-21 9:07 ` Imre Deak
0 siblings, 0 replies; 18+ messages in thread
From: Imre Deak @ 2016-10-21 9:07 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
On pe, 2016-10-21 at 11:51 +0300, Jani Nikula wrote:
> On Fri, 21 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > I tried this now on the fi-skl-6700hq machine in the bug report and it
> > seems to be the same failure mode as the APL one. There is one
> > difference in that on the SKL machine once the firmware is in the stuck
> > state after suspend/resume, a warm reboot may not recover it, so during
> > the next boot the LSPCON probe will fail. I guess that during warm
> > reboot LSPCON may continue to be powered, so to fully reset it a power
> > cycle is needed. In any case this patch fixes resume on that machine
> > too, so I couldn't reproduce the problem with it even across multiple
> > reboots, whereas without it it's 100% reproducible.
>
> A quick thought, when we do module unload or suspend, perhaps we should
> put the LSPCON to the state in which we expect to find it on
> probe/resume? In case it doesn't get reset.
The probe and resume functions handle both PCON and LS modes, switching
only if necessary. But in any case I tried the followings without
success:
- switch to LS mode during suspend
- power down the TMDS output during suspend after switching to LS mode
(DP_DUAL_MODE_TMDS_OEN) and then back on during resume
- while in PCON mode force DP_SET_POWER(D3) during suspend and then
DP_SET_POWER(D0) during resume
--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs
2016-10-20 18:06 ` [PATCH 1/2] " Jani Nikula
2016-10-20 18:58 ` Imre Deak
@ 2016-10-24 8:36 ` Daniel Vetter
1 sibling, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2016-10-24 8:36 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
On Thu, Oct 20, 2016 at 09:06:48PM +0300, Jani Nikula wrote:
> On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > Extend the branch/sink descriptor info with the missing device ID
> > field and print this info for eDP and LSPCON connectors too.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 83 +++++++++++++++----------------------
> > drivers/gpu/drm/i915/intel_drv.h | 13 ++++++
> > drivers/gpu/drm/i915/intel_lspcon.c | 7 ++++
> > 3 files changed, 53 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 88f3b74..e90211e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1442,42 +1442,34 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
> > DRM_DEBUG_KMS("common rates: %s\n", str);
> > }
> >
> > -static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
> > +static bool intel_dp_is_branch(struct intel_dp *intel_dp)
>
> Belongs in drm dp helpers.
So totally hit reply to say the same. Imo all of this debug stuff belongs
into helpers (but we're probably hitting the limit of what's possible
without further unification with the slightly-different helper world of
msm/tegra).
> > @@ -3519,6 +3511,13 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
> > if (!intel_dp_read_dpcd(intel_dp))
> > return false;
> >
> > + if (drm_debug & DRM_UT_KMS) {
> > + struct intel_dp_desc desc;
> > +
> > + if (intel_dp_read_desc(intel_dp, &desc))
> > + intel_dp_print_desc(intel_dp, &desc);
> > + }
>
> I *really* don't think we should do dpcd access conditional to drm
> debugs. I smell heisenbugs just thinking about it.
+1
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index c06a33e..a35e241 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -883,6 +883,14 @@ enum link_m_n_set {
> > M2_N2
> > };
> >
> > +struct intel_dp_desc {
> > + u8 oui[3];
> > + u8 device_id[6];
> > + u8 hw_rev;
> > + u8 sw_major_rev;
> > + u8 sw_minor_rev;
> > +} __packed;
> > +
>
> I understand this, but makes me wonder about dp helpers vs. i915. I
> guess we could add this for now.
It already exists, in the form of slightly-different dp helpers used by
msm and tegra. Compared to our helpers they have some data structure stuff
to cache things. I guess it's time to port i915 over to that world,
respectively unify things a _lot_.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-10-24 8:36 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-20 17:06 [PATCH 1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs Imre Deak
2016-10-20 17:06 ` [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode Imre Deak
2016-10-20 18:20 ` Jani Nikula
2016-10-20 19:20 ` Imre Deak
2016-10-20 21:07 ` Jani Nikula
2016-10-21 5:46 ` Sharma, Shashank
2016-10-21 7:40 ` Imre Deak
2016-10-20 19:24 ` Jani Nikula
2016-10-20 19:43 ` Imre Deak
2016-10-20 20:50 ` Jani Nikula
2016-10-20 21:40 ` Imre Deak
2016-10-21 7:20 ` Imre Deak
2016-10-21 8:51 ` Jani Nikula
2016-10-21 9:07 ` Imre Deak
2016-10-20 17:47 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs Patchwork
2016-10-20 18:06 ` [PATCH 1/2] " Jani Nikula
2016-10-20 18:58 ` Imre Deak
2016-10-24 8:36 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.