[Intel-gfx] [PATCH 1/9] drm/i915/mst: Debug log connector name in destroy_connector()

[Dhinakaran Pandiyan]

Print connector name in destroy_connect() and this doesn't add any extra
lines to dmesg. The debug macro has been moved before the unregister
call so that we don't lose the connector name and id.

Signed-off-by: Dhinakaran Pandiyan <***@intel.com>
---
drivers/gpu/drm/i915/intel_dp_mst.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 8e3aad0ea60b..88d1d2b9ac56 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -494,6 +494,7 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
struct intel_connector *intel_connector = to_intel_connector(connector);
struct drm_i915_private *dev_priv = to_i915(connector->dev);

+ DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name);
drm_connector_unregister(connector);

if (dev_priv->fbdev)
@@ -505,7 +506,6 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
drm_modeset_unlock(&connector->dev->mode_config.connection_mutex);

drm_connector_unreference(connector);
- DRM_DEBUG_KMS("\n");
}

static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)

[Dhinakaran Pandiyan]

Both mst_disable_dp and mst_post_disable_dp print number of active links
before the variable has been updated. Move the print statement in
mst_disable_dp after the decrement so that the printed values indicate
the disabing of a mst connector. Also, add some text to clarify what we
are printing.

Signed-off-by: Dhinakaran Pandiyan <***@intel.com>
---
drivers/gpu/drm/i915/intel_dp_mst.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 88d1d2b9ac56..9a396f483f8b 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -133,7 +133,7 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder,
to_intel_connector(old_conn_state->connector);
int ret;

- DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
+ DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);

drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, connector->port);

@@ -155,8 +155,6 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
struct intel_connector *connector =
to_intel_connector(old_conn_state->connector);

- DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
-
/* this can fail */
drm_dp_check_act_status(&intel_dp->mst_mgr);
/* and this can also fail */
@@ -173,6 +171,7 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,

intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
}
+ DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
}

static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
@@ -195,7 +194,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
connector->encoder = encoder;
intel_mst->connector = connector;

- DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
+ DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);

if (intel_dp->active_mst_links == 0)
intel_dig_port->base.pre_enable(&intel_dig_port->base,
@@ -229,7 +228,7 @@ static void intel_mst_enable_dp(struct intel_encoder *encoder,
enum port port = intel_dig_port->port;
int ret;

- DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
+ DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);

if (intel_wait_for_register(dev_priv,
DP_TP_STATUS(port),

[Ausmus, James]

On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan

Post by Dhinakaran Pandiyan
Both mst_disable_dp and mst_post_disable_dp print number of active links
before the variable has been updated. Move the print statement in
mst_disable_dp after the decrement so that the printed values indicate
the disabing of a mst connector. Also, add some text to clarify what we
are printing.
---
drivers/gpu/drm/i915/intel_dp_mst.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 88d1d2b9ac56..9a396f483f8b 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -133,7 +133,7 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder,
to_intel_connector(old_conn_state->connector);
int ret;
- DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
+ DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, connector->port);
@@ -155,8 +155,6 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
struct intel_connector *connector =
to_intel_connector(old_conn_state->connector);
- DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
-
/* this can fail */
drm_dp_check_act_status(&intel_dp->mst_mgr);
/* and this can also fail */
@@ -173,6 +171,7 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
}
+ DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
}
static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
@@ -195,7 +194,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
connector->encoder = encoder;
intel_mst->connector = connector;
- DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
+ DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
if (intel_dp->active_mst_links == 0)
intel_dig_port->base.pre_enable(&intel_dig_port->base,
@@ -229,7 +228,7 @@ static void intel_mst_enable_dp(struct intel_encoder *encoder,
enum port port = intel_dig_port->port;
int ret;
- DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
+ DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
if (intel_wait_for_register(dev_priv,
DP_TP_STATUS(port),
--
2.11.0
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Dhinakaran Pandiyan]

The buffer size defined is 16 bytes whereas only 14 bytes are read. Add a
macro to avoid this discrepancy.

Signed-off-by: Dhinakaran Pandiyan <***@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 887953c0f495..98e7b96ca826 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -42,6 +42,7 @@
#include "i915_drv.h"

#define DP_LINK_CHECK_TIMEOUT (10 * 1000)
+#define DP_DPRX_ESI_LEN 14

/* Compliance test status bits */
#define INTEL_DP_RESOLUTION_SHIFT_MASK 0
@@ -3991,15 +3992,9 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
static bool
intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
{
- int ret;
-
- ret = drm_dp_dpcd_read(&intel_dp->aux,
- DP_SINK_COUNT_ESI,
- sink_irq_vector, 14);
- if (ret != 14)
- return false;
-
- return true;
+ return drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT_ESI,
+ sink_irq_vector, DP_DPRX_ESI_LEN) ==
+ DP_DPRX_ESI_LEN;
}

static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
@@ -4199,7 +4194,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
bool bret;

if (intel_dp->is_mst) {
- u8 esi[16] = { 0 };
+ u8 esi[DP_DPRX_ESI_LEN] = { 0 };
int ret = 0;
int retry;
bool handled;

[Ausmus, James]

On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan

Post by Dhinakaran Pandiyan
The buffer size defined is 16 bytes whereas only 14 bytes are read. Add a
macro to avoid this discrepancy.
---
drivers/gpu/drm/i915/intel_dp.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 887953c0f495..98e7b96ca826 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -42,6 +42,7 @@
#include "i915_drv.h"
#define DP_LINK_CHECK_TIMEOUT (10 * 1000)
+#define DP_DPRX_ESI_LEN 14
/* Compliance test status bits */
#define INTEL_DP_RESOLUTION_SHIFT_MASK 0
@@ -3991,15 +3992,9 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
static bool
intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
{
- int ret;
-
- ret = drm_dp_dpcd_read(&intel_dp->aux,
- DP_SINK_COUNT_ESI,
- sink_irq_vector, 14);
- if (ret != 14)
- return false;
-
- return true;
+ return drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT_ESI,
+ sink_irq_vector, DP_DPRX_ESI_LEN) ==
+ DP_DPRX_ESI_LEN;
}
static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
@@ -4199,7 +4194,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
bool bret;
if (intel_dp->is_mst) {
- u8 esi[16] = { 0 };
+ u8 esi[DP_DPRX_ESI_LEN] = { 0 };
int ret = 0;
int retry;
bool handled;
--
2.11.0
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Dhinakaran Pandiyan]

There is just only one caller now, which already checks for
intel_dp->is_mst. So, remove this and fix some braces while at it.

Signed-off-by: Dhinakaran Pandiyan <***@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++---------------------
1 file changed, 42 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b3d95cb7546b..3f2ca10ccbcd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4192,57 +4192,56 @@ static int
intel_dp_check_mst_status(struct intel_dp *intel_dp)
{
bool bret;
+ u8 esi[DP_DPRX_ESI_LEN] = { 0 };
+ int ret = 0;
+ int retry;
+ bool handled;

- if (intel_dp->is_mst) {
- u8 esi[DP_DPRX_ESI_LEN] = { 0 };
- int ret = 0;
- int retry;
- bool handled;
- bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
+ bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
go_again:
- if (bret == true) {
-
- /* check link status - esi[10] = 0x200c */
- if (intel_dp->active_mst_links &&
- !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
- DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
- intel_dp_start_link_train(intel_dp);
- intel_dp_stop_link_train(intel_dp);
- }
+ if (bret == true) {
+
+ /* check link status - esi[10] = 0x200c */
+ if (intel_dp->active_mst_links &&
+ !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
+ DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+ intel_dp_start_link_train(intel_dp);
+ intel_dp_stop_link_train(intel_dp);
+ }

- DRM_DEBUG_KMS("got esi %3ph\n", esi);
- ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
-
- if (handled) {
- for (retry = 0; retry < 3; retry++) {
- int wret;
- wret = drm_dp_dpcd_write(&intel_dp->aux,
- DP_SINK_COUNT_ESI+1,
- &esi[1], 3);
- if (wret == 3) {
- break;
- }
- }
+ DRM_DEBUG_KMS("got esi %3ph\n", esi);
+ ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);

- bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
- if (bret == true) {
- DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
- goto go_again;
- }
- } else
- ret = 0;
+ if (handled) {
+ for (retry = 0; retry < 3; retry++) {
+ int wret;

- return ret;
+ wret = drm_dp_dpcd_write(&intel_dp->aux,
+ DP_SINK_COUNT_ESI+1,
+ &esi[1], 3);
+ if (wret == 3)
+ break;
+ }
+
+ bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
+ if (bret == true) {
+ DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
+ goto go_again;
+ }
} else {
- struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
- DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
- intel_dp->is_mst = false;
- drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
- /* send a hotplug event */
- drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
+ ret = 0;
}
+ } else {
+ struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+
+ DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
+ intel_dp->is_mst = false;
+ drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+ intel_dp->is_mst);
+ drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
+ ret = -EINVAL;
}
- return -EINVAL;
+ return ret;
}

static void

[Jani Nikula]

Post by Dhinakaran Pandiyan
There is just only one caller now, which already checks for
intel_dp->is_mst. So, remove this and fix some braces while at it.
---
drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++---------------------
1 file changed, 42 insertions(+), 43 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b3d95cb7546b..3f2ca10ccbcd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4192,57 +4192,56 @@ static int
intel_dp_check_mst_status(struct intel_dp *intel_dp)
{
bool bret;
+ u8 esi[DP_DPRX_ESI_LEN] = { 0 };
+ int ret = 0;
+ int retry;
+ bool handled;
- if (intel_dp->is_mst) {
- u8 esi[DP_DPRX_ESI_LEN] = { 0 };
- int ret = 0;
- int retry;
- bool handled;
- bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
+ bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
- if (bret == true) {
-
- /* check link status - esi[10] = 0x200c */

Drive-by observation unrelated to this patch: 0x200c isn't even defined
in drm_dp_helper.h... care to add that, please?

BR,
Jani.

Post by Dhinakaran Pandiyan
- if (intel_dp->active_mst_links &&
- !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
- DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
- intel_dp_start_link_train(intel_dp);
- intel_dp_stop_link_train(intel_dp);
- }
+ if (bret == true) {
+
+ /* check link status - esi[10] = 0x200c */
+ if (intel_dp->active_mst_links &&
+ !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
+ DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+ intel_dp_start_link_train(intel_dp);
+ intel_dp_stop_link_train(intel_dp);
+ }
- DRM_DEBUG_KMS("got esi %3ph\n", esi);
- ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
-
- if (handled) {
- for (retry = 0; retry < 3; retry++) {
- int wret;
- wret = drm_dp_dpcd_write(&intel_dp->aux,
- DP_SINK_COUNT_ESI+1,
- &esi[1], 3);
- if (wret == 3) {
- break;
- }
- }
+ DRM_DEBUG_KMS("got esi %3ph\n", esi);
+ ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
- bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
- if (bret == true) {
- DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
- goto go_again;
- }
- } else
- ret = 0;
+ if (handled) {
+ for (retry = 0; retry < 3; retry++) {
+ int wret;
- return ret;
+ wret = drm_dp_dpcd_write(&intel_dp->aux,
+ DP_SINK_COUNT_ESI+1,
+ &esi[1], 3);
+ if (wret == 3)
+ break;
+ }
+
+ bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
+ if (bret == true) {
+ DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
+ goto go_again;
+ }
} else {
- struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
- DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
- intel_dp->is_mst = false;
- drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
- /* send a hotplug event */
- drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
+ ret = 0;
}
+ } else {
+ struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+
+ DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
+ intel_dp->is_mst = false;
+ drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+ intel_dp->is_mst);
+ drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
+ ret = -EINVAL;
}
- return -EINVAL;
+ return ret;
}
static void

[Pandiyan, Dhinakaran]

Post by Jani Nikula

Post by Dhinakaran Pandiyan
There is just only one caller now, which already checks for
intel_dp->is_mst. So, remove this and fix some braces while at it.
---
drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++---------------------
1 file changed, 42 insertions(+), 43 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b3d95cb7546b..3f2ca10ccbcd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4192,57 +4192,56 @@ static int
intel_dp_check_mst_status(struct intel_dp *intel_dp)
{
bool bret;
+ u8 esi[DP_DPRX_ESI_LEN] = { 0 };
+ int ret = 0;
+ int retry;
+ bool handled;
- if (intel_dp->is_mst) {
- u8 esi[DP_DPRX_ESI_LEN] = { 0 };
- int ret = 0;
- int retry;
- bool handled;
- bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
+ bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
- if (bret == true) {
-
- /* check link status - esi[10] = 0x200c */

Drive-by observation unrelated to this patch: 0x200c isn't even defined
in drm_dp_helper.h... care to add that, please?

Sure.

Post by Jani Nikula
BR,
Jani.

Post by Dhinakaran Pandiyan
- if (intel_dp->active_mst_links &&
- !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
- DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
- intel_dp_start_link_train(intel_dp);
- intel_dp_stop_link_train(intel_dp);
- }
+ if (bret == true) {
+
+ /* check link status - esi[10] = 0x200c */
+ if (intel_dp->active_mst_links &&
+ !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
+ DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+ intel_dp_start_link_train(intel_dp);
+ intel_dp_stop_link_train(intel_dp);
+ }
- DRM_DEBUG_KMS("got esi %3ph\n", esi);
- ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
-
- if (handled) {
- for (retry = 0; retry < 3; retry++) {
- int wret;
- wret = drm_dp_dpcd_write(&intel_dp->aux,
- DP_SINK_COUNT_ESI+1,
- &esi[1], 3);
- if (wret == 3) {
- break;
- }
- }
+ DRM_DEBUG_KMS("got esi %3ph\n", esi);
+ ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
- bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
- if (bret == true) {
- DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
- goto go_again;
- }
- } else
- ret = 0;
+ if (handled) {
+ for (retry = 0; retry < 3; retry++) {
+ int wret;
- return ret;
+ wret = drm_dp_dpcd_write(&intel_dp->aux,
+ DP_SINK_COUNT_ESI+1,
+ &esi[1], 3);
+ if (wret == 3)
+ break;
+ }
+
+ bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
+ if (bret == true) {
+ DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
+ goto go_again;
+ }
} else {
- struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
- DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
- intel_dp->is_mst = false;
- drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
- /* send a hotplug event */
- drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
+ ret = 0;
}
+ } else {
+ struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+
+ DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
+ intel_dp->is_mst = false;
+ drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+ intel_dp->is_mst);
+ drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
+ ret = -EINVAL;
}
- return -EINVAL;
+ return ret;
}
static void

[Ville Syrjälä]

Post by Dhinakaran Pandiyan
There is just only one caller now, which already checks for
intel_dp->is_mst. So, remove this and fix some braces while at it.

Hmm. So this depends on the subtle detail that
drm_dp_mst_topology_mgr_resume() will "fail" if didn't enable mst
prior to calling it. That fact isn't obvious at all, so I would
suggest noting that in the commit message at least (maybe we should
even have a comment in the code?)

I'm assuming there are not other changes beyond just dropping the
top level 'if (mst) { }' thing? This is one reason I prefer flat code.
If the function had been flat to begin with the diff would have
actually turned out legible. IIRC I even posted a flattening patch at
some point but I guess it didn't go in. I would suggest that you take
the opportunity to flatten out the rest of this function while you
in the neighbourhood.

Post by Dhinakaran Pandiyan
---
drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++---------------------
1 file changed, 42 insertions(+), 43 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b3d95cb7546b..3f2ca10ccbcd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4192,57 +4192,56 @@ static int
intel_dp_check_mst_status(struct intel_dp *intel_dp)
{
bool bret;
+ u8 esi[DP_DPRX_ESI_LEN] = { 0 };
+ int ret = 0;
+ int retry;
+ bool handled;
- if (intel_dp->is_mst) {
- u8 esi[DP_DPRX_ESI_LEN] = { 0 };
- int ret = 0;
- int retry;
- bool handled;
- bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
+ bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
- if (bret == true) {
-
- /* check link status - esi[10] = 0x200c */
- if (intel_dp->active_mst_links &&
- !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
- DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
- intel_dp_start_link_train(intel_dp);
- intel_dp_stop_link_train(intel_dp);
- }
+ if (bret == true) {
+
+ /* check link status - esi[10] = 0x200c */
+ if (intel_dp->active_mst_links &&
+ !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
+ DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+ intel_dp_start_link_train(intel_dp);
+ intel_dp_stop_link_train(intel_dp);
+ }
- DRM_DEBUG_KMS("got esi %3ph\n", esi);
- ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
-
- if (handled) {
- for (retry = 0; retry < 3; retry++) {
- int wret;
- wret = drm_dp_dpcd_write(&intel_dp->aux,
- DP_SINK_COUNT_ESI+1,
- &esi[1], 3);
- if (wret == 3) {
- break;
- }
- }
+ DRM_DEBUG_KMS("got esi %3ph\n", esi);
+ ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
- bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
- if (bret == true) {
- DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
- goto go_again;
- }
- } else
- ret = 0;
+ if (handled) {
+ for (retry = 0; retry < 3; retry++) {
+ int wret;
- return ret;
+ wret = drm_dp_dpcd_write(&intel_dp->aux,
+ DP_SINK_COUNT_ESI+1,
+ &esi[1], 3);
+ if (wret == 3)
+ break;
+ }
+
+ bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
+ if (bret == true) {
+ DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
+ goto go_again;
+ }
} else {
- struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
- DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
- intel_dp->is_mst = false;
- drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
- /* send a hotplug event */
- drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
+ ret = 0;
}
+ } else {
+ struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+
+ DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
+ intel_dp->is_mst = false;
+ drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+ intel_dp->is_mst);
+ drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
+ ret = -EINVAL;
}
- return -EINVAL;
+ return ret;
}
static void
--
2.11.0

[Pandiyan, Dhinakaran]

Post by Ville Syrjälä

Post by Dhinakaran Pandiyan
There is just only one caller now, which already checks for
intel_dp->is_mst. So, remove this and fix some braces while at it.

Hmm. So this depends on the subtle detail that
drm_dp_mst_topology_mgr_resume() will "fail" if didn't enable mst
prior to calling it. That fact isn't obvious at all, so I would
suggest noting that in the commit message at least (maybe we should
even have a comment in the code?)
I'm assuming there are not other changes beyond just dropping the
top level 'if (mst) { }' thing? This is one reason I prefer flat code.
If the function had been flat to begin with the diff would have
actually turned out legible. IIRC I even posted a flattening patch at
some point but I guess it didn't go in. I would suggest that you take
the opportunity to flatten out the rest of this function while you
in the neighbourhood.

That was the goal :) Then I decided to simplify the function a little
bit in patches 4, 5, 6 and 7 before working on modifying it's structure.

Post by Ville Syrjälä

Post by Dhinakaran Pandiyan
---
drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++---------------------
1 file changed, 42 insertions(+), 43 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b3d95cb7546b..3f2ca10ccbcd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4192,57 +4192,56 @@ static int
intel_dp_check_mst_status(struct intel_dp *intel_dp)
{
bool bret;
+ u8 esi[DP_DPRX_ESI_LEN] = { 0 };
+ int ret = 0;
+ int retry;
+ bool handled;
- if (intel_dp->is_mst) {
- u8 esi[DP_DPRX_ESI_LEN] = { 0 };
- int ret = 0;
- int retry;
- bool handled;
- bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
+ bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
- if (bret == true) {
-
- /* check link status - esi[10] = 0x200c */
- if (intel_dp->active_mst_links &&
- !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
- DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
- intel_dp_start_link_train(intel_dp);
- intel_dp_stop_link_train(intel_dp);
- }
+ if (bret == true) {
+
+ /* check link status - esi[10] = 0x200c */
+ if (intel_dp->active_mst_links &&
+ !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
+ DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+ intel_dp_start_link_train(intel_dp);
+ intel_dp_stop_link_train(intel_dp);
+ }
- DRM_DEBUG_KMS("got esi %3ph\n", esi);
- ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
-
- if (handled) {
- for (retry = 0; retry < 3; retry++) {
- int wret;
- wret = drm_dp_dpcd_write(&intel_dp->aux,
- DP_SINK_COUNT_ESI+1,
- &esi[1], 3);
- if (wret == 3) {
- break;
- }
- }
+ DRM_DEBUG_KMS("got esi %3ph\n", esi);
+ ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
- bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
- if (bret == true) {
- DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
- goto go_again;
- }
- } else
- ret = 0;
+ if (handled) {
+ for (retry = 0; retry < 3; retry++) {
+ int wret;
- return ret;
+ wret = drm_dp_dpcd_write(&intel_dp->aux,
+ DP_SINK_COUNT_ESI+1,
+ &esi[1], 3);
+ if (wret == 3)
+ break;
+ }
+
+ bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
+ if (bret == true) {
+ DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
+ goto go_again;
+ }
} else {
- struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
- DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
- intel_dp->is_mst = false;
- drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
- /* send a hotplug event */
- drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
+ ret = 0;
}
+ } else {
+ struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+
+ DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
+ intel_dp->is_mst = false;
+ drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+ intel_dp->is_mst);
+ drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
+ ret = -EINVAL;
}
- return -EINVAL;
+ return ret;
}
static void
--
2.11.0

[Dhinakaran Pandiyan]

drm_dp_mst_topology_mgr_resume() fails if there are dpcd failures, so
there's no need to try that again in _check_mst_status()

Signed-off-by: Dhinakaran Pandiyan <***@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 98e7b96ca826..b3d95cb7546b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6224,12 +6224,16 @@ void intel_dp_mst_resume(struct drm_device *dev)
for (i = 0; i < I915_MAX_PORTS; i++) {
struct intel_digital_port *intel_dig_port = dev_priv->hotplug.irq_port[i];
int ret;
+ struct intel_dp *intel_dp = &intel_dig_port->dp;

- if (!intel_dig_port || !intel_dig_port->dp.can_mst)
+ if (!intel_dig_port || !intel_dp->can_mst)
continue;

- ret = drm_dp_mst_topology_mgr_resume(&intel_dig_port->dp.mst_mgr);
- if (ret)
- intel_dp_check_mst_status(&intel_dig_port->dp);
+ ret = drm_dp_mst_topology_mgr_resume(&intel_dp->mst_mgr);
+ if (ret) {
+ intel_dp->is_mst = false;
+ drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
+ drm_kms_helper_hotplug_event(dev);
+ }
}
}

[Ville Syrjälä]

Post by Dhinakaran Pandiyan
drm_dp_mst_topology_mgr_resume() fails if there are dpcd failures, so
there's no need to try that again in _check_mst_status()

That commit message somehow doesn't seem to match this patch. You're not
removing anything from _check_mst_status() here.

Post by Dhinakaran Pandiyan
---
drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 98e7b96ca826..b3d95cb7546b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6224,12 +6224,16 @@ void intel_dp_mst_resume(struct drm_device *dev)
for (i = 0; i < I915_MAX_PORTS; i++) {
struct intel_digital_port *intel_dig_port = dev_priv->hotplug.irq_port[i];
int ret;
+ struct intel_dp *intel_dp = &intel_dig_port->dp;
- if (!intel_dig_port || !intel_dig_port->dp.can_mst)
+ if (!intel_dig_port || !intel_dp->can_mst)
continue;
- ret = drm_dp_mst_topology_mgr_resume(&intel_dig_port->dp.mst_mgr);
- if (ret)
- intel_dp_check_mst_status(&intel_dig_port->dp);
+ ret = drm_dp_mst_topology_mgr_resume(&intel_dp->mst_mgr);
+ if (ret) {
+ intel_dp->is_mst = false;
+ drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
+ drm_kms_helper_hotplug_event(dev);
+ }
}
}
--
2.11.0

[Pandiyan, Dhinakaran]

Post by Ville Syrjälä

Post by Dhinakaran Pandiyan
drm_dp_mst_topology_mgr_resume() fails if there are dpcd failures, so
there's no need to try that again in _check_mst_status()

That commit message somehow doesn't seem to match this patch. You're not
removing anything from _check_mst_status() here.

You are right, I meant to write "there's no need to call
_check_mst_status to try dpcd reads again"

Is that good?

Post by Ville Syrjälä

Post by Dhinakaran Pandiyan
---
drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 98e7b96ca826..b3d95cb7546b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6224,12 +6224,16 @@ void intel_dp_mst_resume(struct drm_device *dev)
for (i = 0; i < I915_MAX_PORTS; i++) {
struct intel_digital_port *intel_dig_port = dev_priv->hotplug.irq_port[i];
int ret;
+ struct intel_dp *intel_dp = &intel_dig_port->dp;
- if (!intel_dig_port || !intel_dig_port->dp.can_mst)
+ if (!intel_dig_port || !intel_dp->can_mst)
continue;
- ret = drm_dp_mst_topology_mgr_resume(&intel_dig_port->dp.mst_mgr);
- if (ret)
- intel_dp_check_mst_status(&intel_dig_port->dp);
+ ret = drm_dp_mst_topology_mgr_resume(&intel_dp->mst_mgr);
+ if (ret) {
+ intel_dp->is_mst = false;
+ drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
+ drm_kms_helper_hotplug_event(dev);
+ }
}
}
--
2.11.0

[Dhinakaran Pandiyan]

The other instances of link training are protected with
connection_mutex, so do the same in check_mst_status() too.

Signed-off-by: Dhinakaran Pandiyan <***@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index aab9ba31f79e..644463ba313e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
static int
intel_dp_check_mst_status(struct intel_dp *intel_dp)
{
+ struct drm_device *dev = intel_dp_to_dev(intel_dp);
bool bret;
u8 esi[DP_DPRX_ESI_LEN] = { 0 };
int ret = 0;
@@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
if (intel_dp->active_mst_links &&
!drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+
+ drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
intel_dp_start_link_train(intel_dp);
intel_dp_stop_link_train(intel_dp);
+ drm_modeset_unlock(&dev->mode_config.connection_mutex);
}

DRM_DEBUG_KMS("got esi %3ph\n", esi);

[Ausmus, James]

On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan

Post by Dhinakaran Pandiyan
The other instances of link training are protected with
connection_mutex, so do the same in check_mst_status() too.

We don't seem to be taking connection_mutex around
intel_dp_start/stop_link_train in the intel_enable_dp or
intel_ddi_pre_enable_dp paths (unless it's taken higher in the stack)
- is it needed in all instances?

Post by Dhinakaran Pandiyan
---
drivers/gpu/drm/i915/intel_dp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index aab9ba31f79e..644463ba313e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
static int
intel_dp_check_mst_status(struct intel_dp *intel_dp)
{
+ struct drm_device *dev = intel_dp_to_dev(intel_dp);
bool bret;
u8 esi[DP_DPRX_ESI_LEN] = { 0 };
int ret = 0;
@@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
if (intel_dp->active_mst_links &&
!drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+
+ drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
intel_dp_start_link_train(intel_dp);
intel_dp_stop_link_train(intel_dp);
+ drm_modeset_unlock(&dev->mode_config.connection_mutex);
}
DRM_DEBUG_KMS("got esi %3ph\n", esi);
--
2.11.0
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Pandiyan, Dhinakaran]

Post by Ausmus, James
On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan

Post by Dhinakaran Pandiyan
The other instances of link training are protected with
connection_mutex, so do the same in check_mst_status() too.

We don't seem to be taking connection_mutex around
intel_dp_start/stop_link_train in the intel_enable_dp or
intel_ddi_pre_enable_dp paths (unless it's taken higher in the stack)

That is right, it is take when the modeset begins.

Post by Ausmus, James
- is it needed in all instances?

Post by Dhinakaran Pandiyan
---
drivers/gpu/drm/i915/intel_dp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index aab9ba31f79e..644463ba313e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
static int
intel_dp_check_mst_status(struct intel_dp *intel_dp)
{
+ struct drm_device *dev = intel_dp_to_dev(intel_dp);
bool bret;
u8 esi[DP_DPRX_ESI_LEN] = { 0 };
int ret = 0;
@@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
if (intel_dp->active_mst_links &&
!drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+
+ drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
intel_dp_start_link_train(intel_dp);
intel_dp_stop_link_train(intel_dp);
+ drm_modeset_unlock(&dev->mode_config.connection_mutex);
}
DRM_DEBUG_KMS("got esi %3ph\n", esi);
--
2.11.0
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Manasi Navare]

Post by Ausmus, James
On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan

Post by Dhinakaran Pandiyan
The other instances of link training are protected with
connection_mutex, so do the same in check_mst_status() too.

We don't seem to be taking connection_mutex around
intel_dp_start/stop_link_train in the intel_enable_dp or
intel_ddi_pre_enable_dp paths (unless it's taken higher in the stack)
- is it needed in all instances?

intel_ddi_pre_enable_dp() gets called from the crtc_enable hook higher
up in the stack which gets called essentially from atomic_commit() hook
that traces back eventually to the drm_mode_setcrtc call that grabs the
mod_config mutex there: mutex_lock(&crtc->dev->mode_config.mutex);

Manasi

Post by Ausmus, James

Post by Dhinakaran Pandiyan
---
drivers/gpu/drm/i915/intel_dp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index aab9ba31f79e..644463ba313e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
static int
intel_dp_check_mst_status(struct intel_dp *intel_dp)
{
+ struct drm_device *dev = intel_dp_to_dev(intel_dp);
bool bret;
u8 esi[DP_DPRX_ESI_LEN] = { 0 };
int ret = 0;
@@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
if (intel_dp->active_mst_links &&
!drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+
+ drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
intel_dp_start_link_train(intel_dp);
intel_dp_stop_link_train(intel_dp);
+ drm_modeset_unlock(&dev->mode_config.connection_mutex);
}
DRM_DEBUG_KMS("got esi %3ph\n", esi);
--
2.11.0
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
James Ausmus
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Ausmus, James]

On Fri, Sep 15, 2017 at 11:38 AM, Manasi Navare

Post by Manasi Navare

Post by Ausmus, James
On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan

Post by Dhinakaran Pandiyan
The other instances of link training are protected with
connection_mutex, so do the same in check_mst_status() too.

We don't seem to be taking connection_mutex around
intel_dp_start/stop_link_train in the intel_enable_dp or
intel_ddi_pre_enable_dp paths (unless it's taken higher in the stack)
- is it needed in all instances?

intel_ddi_pre_enable_dp() gets called from the crtc_enable hook higher
up in the stack which gets called essentially from atomic_commit() hook
that traces back eventually to the drm_mode_setcrtc call that grabs the
mod_config mutex there: mutex_lock(&crtc->dev->mode_config.mutex);

Got it - thanks for the pointer!

-James

Post by Manasi Navare
Manasi

Post by Ausmus, James

Post by Dhinakaran Pandiyan
---
drivers/gpu/drm/i915/intel_dp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index aab9ba31f79e..644463ba313e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
static int
intel_dp_check_mst_status(struct intel_dp *intel_dp)
{
+ struct drm_device *dev = intel_dp_to_dev(intel_dp);
bool bret;
u8 esi[DP_DPRX_ESI_LEN] = { 0 };
int ret = 0;
@@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
if (intel_dp->active_mst_links &&
!drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+
+ drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
intel_dp_start_link_train(intel_dp);
intel_dp_stop_link_train(intel_dp);
+ drm_modeset_unlock(&dev->mode_config.connection_mutex);
}
DRM_DEBUG_KMS("got esi %3ph\n", esi);
--
2.11.0
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
James Ausmus
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Ville Syrjälä]

Post by Dhinakaran Pandiyan
The other instances of link training are protected with
connection_mutex, so do the same in check_mst_status() too.
---
drivers/gpu/drm/i915/intel_dp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index aab9ba31f79e..644463ba313e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
static int
intel_dp_check_mst_status(struct intel_dp *intel_dp)
{
+ struct drm_device *dev = intel_dp_to_dev(intel_dp);
bool bret;
u8 esi[DP_DPRX_ESI_LEN] = { 0 };
int ret = 0;
@@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
if (intel_dp->active_mst_links &&
!drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+
+ drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
intel_dp_start_link_train(intel_dp);
intel_dp_stop_link_train(intel_dp);
+ drm_modeset_unlock(&dev->mode_config.connection_mutex);

This can deadlock. We should not grab any modeset locks from the
dig_work. I had some patches at some point to move the link training to
the hotplug work so SST. I don't think I had the MST side really sorted
out at any point.

Post by Dhinakaran Pandiyan
}
DRM_DEBUG_KMS("got esi %3ph\n", esi);
--
2.11.0

[Pandiyan, Dhinakaran]

Post by Ville Syrjälä

Post by Dhinakaran Pandiyan
The other instances of link training are protected with
connection_mutex, so do the same in check_mst_status() too.
---
drivers/gpu/drm/i915/intel_dp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index aab9ba31f79e..644463ba313e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
static int
intel_dp_check_mst_status(struct intel_dp *intel_dp)
{
+ struct drm_device *dev = intel_dp_to_dev(intel_dp);
bool bret;
u8 esi[DP_DPRX_ESI_LEN] = { 0 };
int ret = 0;
@@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
if (intel_dp->active_mst_links &&
!drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+
+ drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
intel_dp_start_link_train(intel_dp);
intel_dp_stop_link_train(intel_dp);
+ drm_modeset_unlock(&dev->mode_config.connection_mutex);

This can deadlock. We should not grab any modeset locks from the
dig_work. I had some patches at some point to move the link training to
the hotplug work so SST. I don't think I had the MST side really sorted
out at any point.

Interesting, the only lock we grab in this path from the work function
is the connection_mutex. So, I am not clear how this will deadlock. This
lock around link training is also at the same depth as the one around
link training in intel_dp_short_pulse(). Wouldn't that also deadlock if
that's the case?

Post by Ville Syrjälä

Post by Dhinakaran Pandiyan
}
DRM_DEBUG_KMS("got esi %3ph\n", esi);
--
2.11.0

[Ville Syrjälä]

Post by Pandiyan, Dhinakaran

Post by Ville Syrjälä

Post by Dhinakaran Pandiyan
The other instances of link training are protected with
connection_mutex, so do the same in check_mst_status() too.
---
drivers/gpu/drm/i915/intel_dp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index aab9ba31f79e..644463ba313e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
static int
intel_dp_check_mst_status(struct intel_dp *intel_dp)
{
+ struct drm_device *dev = intel_dp_to_dev(intel_dp);
bool bret;
u8 esi[DP_DPRX_ESI_LEN] = { 0 };
int ret = 0;
@@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
if (intel_dp->active_mst_links &&
!drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+
+ drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
intel_dp_start_link_train(intel_dp);
intel_dp_stop_link_train(intel_dp);
+ drm_modeset_unlock(&dev->mode_config.connection_mutex);

This can deadlock. We should not grab any modeset locks from the
dig_work. I had some patches at some point to move the link training to
the hotplug work so SST. I don't think I had the MST side really sorted
out at any point.

Interesting, the only lock we grab in this path from the work function
is the connection_mutex. So, I am not clear how this will deadlock. This
lock around link training is also at the same depth as the one around
link training in intel_dp_short_pulse(). Wouldn't that also deadlock if
that's the case?

Theoretically. Though I think the deadlock is only likely to happen with
MST since that requires sideband to work during a modeset when we have
connection_mutex already locked. So modeset code will be waiting for
sideband to happen, and hpd_pulse which is responsible for doing sideband
is waiting on the lock already held by the modeset. Thus we're stuck.

[Pandiyan, Dhinakaran]

Post by Ville Syrjälä

Post by Pandiyan, Dhinakaran

Post by Ville Syrjälä

Post by Dhinakaran Pandiyan
The other instances of link training are protected with
connection_mutex, so do the same in check_mst_status() too.
---
drivers/gpu/drm/i915/intel_dp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index aab9ba31f79e..644463ba313e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
static int
intel_dp_check_mst_status(struct intel_dp *intel_dp)
{
+ struct drm_device *dev = intel_dp_to_dev(intel_dp);
bool bret;
u8 esi[DP_DPRX_ESI_LEN] = { 0 };
int ret = 0;
@@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
if (intel_dp->active_mst_links &&
!drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+
+ drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
intel_dp_start_link_train(intel_dp);
intel_dp_stop_link_train(intel_dp);
+ drm_modeset_unlock(&dev->mode_config.connection_mutex);

This can deadlock. We should not grab any modeset locks from the
dig_work. I had some patches at some point to move the link training to
the hotplug work so SST. I don't think I had the MST side really sorted
out at any point.

Interesting, the only lock we grab in this path from the work function
is the connection_mutex. So, I am not clear how this will deadlock. This
lock around link training is also at the same depth as the one around
link training in intel_dp_short_pulse(). Wouldn't that also deadlock if
that's the case?

Theoretically. Though I think the deadlock is only likely to happen with
MST since that requires sideband to work during a modeset when we have
connection_mutex already locked. So modeset code will be waiting for
sideband to happen, and hpd_pulse which is responsible for doing sideband
is waiting on the lock already held by the modeset. Thus we're stuck.

disable_dp and enable_dp are the ones that use sideband messages during
modeset. But, there doesn't seem to be any real dependency on the
sideband messages, mostly because there is not much we do differently
when a downstream reply does not arrive.

Having said that, now that I know you (moving link training over to
hotplug work) and Maarten (link training vs modeset) already have code
to deal with this properly, this patch should be dropped. Thanks for the
review!

[Dhinakaran Pandiyan]

intel_dp_can_mst() performs these checks.

Signed-off-by: Dhinakaran Pandiyan <***@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 6 ------
1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 644463ba313e..a4465b46bb27 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3845,12 +3845,6 @@ intel_dp_can_mst(struct intel_dp *intel_dp)
static void
intel_dp_configure_mst(struct intel_dp *intel_dp)
{
- if (!i915.enable_dp_mst)
- return;
-
- if (!intel_dp->can_mst)
- return;
-
intel_dp->is_mst = intel_dp_can_mst(intel_dp);

if (intel_dp->is_mst)

[Ausmus, James]

On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan

Post by Dhinakaran Pandiyan
intel_dp_can_mst() performs these checks.
---
drivers/gpu/drm/i915/intel_dp.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 644463ba313e..a4465b46bb27 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3845,12 +3845,6 @@ intel_dp_can_mst(struct intel_dp *intel_dp)
static void
intel_dp_configure_mst(struct intel_dp *intel_dp)
{
- if (!i915.enable_dp_mst)
- return;
-
- if (!intel_dp->can_mst)
- return;
-

By dropping these returns, we will now always get DRM_DEBUG_KMS output
on whether the sink is MST capable, even if the i915.enable_dp_mst
param is false - maybe drop these from intel_dp_can_mst instead?

Post by Dhinakaran Pandiyan
intel_dp->is_mst = intel_dp_can_mst(intel_dp);
if (intel_dp->is_mst)
--
2.11.0
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Pandiyan, Dhinakaran]

Post by Ausmus, James
On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan

Post by Dhinakaran Pandiyan
intel_dp_can_mst() performs these checks.
---
drivers/gpu/drm/i915/intel_dp.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 644463ba313e..a4465b46bb27 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3845,12 +3845,6 @@ intel_dp_can_mst(struct intel_dp *intel_dp)
static void
intel_dp_configure_mst(struct intel_dp *intel_dp)
{
- if (!i915.enable_dp_mst)
- return;
-
- if (!intel_dp->can_mst)
- return;
-

By dropping these returns, we will now always get DRM_DEBUG_KMS output
on whether the sink is MST capable, even if the i915.enable_dp_mst
param is false

You are right, that debug msg is unnecessary.

Post by Ausmus, James
- maybe drop these from intel_dp_can_mst instead?

Those checks are needed by the other caller - intel_detect_dpcd()


On second thought, the early returns are indeed useful. Thanks for the
review.

Post by Ausmus, James

Post by Dhinakaran Pandiyan
intel_dp->is_mst = intel_dp_can_mst(intel_dp);
if (intel_dp->is_mst)
--
2.11.0
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Dhinakaran Pandiyan]

We debug log the esi dpcd's twice after reading them back, which is not
necessary.

Signed-off-by: Dhinakaran Pandiyan <***@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2886a2ef1591..aab9ba31f79e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4224,10 +4224,8 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
}

bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
- if (bret == true) {
- DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
+ if (bret == true)
goto go_again;
- }
} else {
ret = 0;
}

[Ausmus, James]

On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan

Post by Dhinakaran Pandiyan
We debug log the esi dpcd's twice after reading them back, which is not
necessary.

While you're cleaning this up, why not move the do_again label beneath
the if - the only time the goto happens is immediately after you've
already checked if (bret == true), so no need to check again.

Either way -

Post by Dhinakaran Pandiyan
---
drivers/gpu/drm/i915/intel_dp.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2886a2ef1591..aab9ba31f79e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4224,10 +4224,8 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
}
bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
- if (bret == true) {
- DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
+ if (bret == true)
goto go_again;
- }
} else {
ret = 0;
}
--
2.11.0
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Dhinakaran Pandiyan]

The caller already has code to handle failure, no need to duplicate
that.

Signed-off-by: Dhinakaran Pandiyan <***@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 6 ------
1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3f2ca10ccbcd..2886a2ef1591 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4232,13 +4232,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
ret = 0;
}
} else {
- struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-
DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
- intel_dp->is_mst = false;
- drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
- intel_dp->is_mst);
- drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
ret = -EINVAL;
}
return ret;

[Ausmus, James]

On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan

Post by Dhinakaran Pandiyan
The caller already has code to handle failure, no need to duplicate
that.
---
drivers/gpu/drm/i915/intel_dp.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3f2ca10ccbcd..2886a2ef1591 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4232,13 +4232,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
ret = 0;
}
} else {
- struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-
DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
- intel_dp->is_mst = false;
- drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
- intel_dp->is_mst);
- drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);

It looks like intel_dp_hpd_pulse doesn't call
drm_kms_helper_hotplug_event on a failure in
intel_dp_check_mst_status, so we would lose that path with this patch
- do we need that?

Post by Dhinakaran Pandiyan
ret = -EINVAL;
}
return ret;
--
2.11.0
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Pandiyan, Dhinakaran]

Post by Ausmus, James
On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan

Post by Dhinakaran Pandiyan
The caller already has code to handle failure, no need to duplicate
that.
---
drivers/gpu/drm/i915/intel_dp.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3f2ca10ccbcd..2886a2ef1591 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4232,13 +4232,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
ret = 0;
}
} else {
- struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-
DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
- intel_dp->is_mst = false;
- drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
- intel_dp->is_mst);
- drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);

It looks like intel_dp_hpd_pulse doesn't call
drm_kms_helper_hotplug_event on a failure in
intel_dp_check_mst_status, so we would lose that path with this patch
- do we need that?

Because we return IRQ_NONE when that happens, intel_dp_detect() should
eventually get called from i915_digport_work_func(). Further, when the
connector status gets updated in
intel_dp_detect()->intel_dp_long_pulse(), a hotplug_event ends up being
sent.

But this patch only should be removing redundant code, not make alter
the code path. I'll fix that up. Thanks for catching this.

Post by Ausmus, James

Post by Dhinakaran Pandiyan
ret = -EINVAL;
}
return ret;
--
2.11.0
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Patchwork]

== Series Details ==

Series: series starting with [1/9] drm/i915/mst: Debug log connector name in destroy_connector()
URL : https://patchwork.freedesktop.org/series/30250/
State : failure

== Summary ==

Series 30250v1 series starting with [1/9] drm/i915/mst: Debug log connector name in destroy_connector()
https://patchwork.freedesktop.org/api/1.0/series/30250/revisions/1/mbox/

Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-atomic:
fail -> PASS (fi-snb-2600) fdo#100215
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-c:
pass -> FAIL (fi-kbl-r)

fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215

fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:447s
fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:457s
fi-blb-e6850 total:289 pass:224 dwarn:1 dfail:0 fail:0 skip:64 time:375s
fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:537s
fi-bwr-2160 total:289 pass:184 dwarn:0 dfail:0 fail:0 skip:105 time:269s
fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:478s
fi-byt-j1900 total:289 pass:254 dwarn:1 dfail:0 fail:0 skip:34 time:502s
fi-byt-n2820 total:289 pass:250 dwarn:1 dfail:0 fail:0 skip:38 time:495s
fi-cfl-s total:289 pass:250 dwarn:4 dfail:0 fail:0 skip:35 time:448s
fi-elk-e7500 total:289 pass:230 dwarn:0 dfail:0 fail:0 skip:59 time:456s
fi-glk-2a total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:566s
fi-hsw-4770r total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:411s
fi-ilk-650 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:426s
fi-ivb-3520m total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:489s
fi-ivb-3770 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:464s
fi-kbl-7500u total:289 pass:264 dwarn:1 dfail:0 fail:0 skip:24 time:487s
fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:563s
fi-kbl-r total:289 pass:261 dwarn:0 dfail:0 fail:1 skip:27 time:552s
fi-pnv-d510 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:548s
fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:460s
fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:782s
fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:503s
fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:467s
fi-skl-x1585l total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:477s
fi-snb-2520m total:289 pass:251 dwarn:0 dfail:0 fail:0 skip:38 time:572s
fi-snb-2600 total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:421s

c9e787e0e616ba39a2ef195cb24d2e79b628bb9a drm-tip: 2017y-09m-12d-20h-02m-24s UTC integration manifest
855dacf6da80 drm/i915/dp: Remove redundant can_mst checks in intel_dp_configure_mst()
2b0def9797ca drm/i915/dp: Protect link training with connection mutex
53df94b117ab drm/i915/dp: Avoid double-printing esi regs
ab2e0660d53c drm/i915/dp: Handle check_mst_status() failure in one place
42b6914d0a96 drm/i915/dp: Remove intel_dp->is_mst check in intel_dp_check_mst_status
477a2fb3b949 drm/i915/dp: Avoid more dpcd transactions after resume failure
c11032688bb7 drm/i915/dp: Fix buffer size for sink_irq_esi read
3c930225c7cf drm/i915/mst: Print active mst links after update
c42ee9091c0c drm/i915/mst: Debug log connector name in destroy_connector()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5670/

[Ausmus, James]

On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan

Post by Dhinakaran Pandiyan
Print connector name in destroy_connect() and this doesn't add any extra
lines to dmesg. The debug macro has been moved before the unregister
call so that we don't lose the connector name and id.

Reviewed-by: James Ausmus <***@intel.com>

While I was looking around here, though, I noticed we don't currently
handle error returns from the drm layer calls in
intel_dp_add_mst_connector. I'll try to send a patch along tomorrow
for that.

Post by Dhinakaran Pandiyan
---
drivers/gpu/drm/i915/intel_dp_mst.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 8e3aad0ea60b..88d1d2b9ac56 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -494,6 +494,7 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
struct intel_connector *intel_connector = to_intel_connector(connector);
struct drm_i915_private *dev_priv = to_i915(connector->dev);
+ DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name);
drm_connector_unregister(connector);
if (dev_priv->fbdev)
@@ -505,7 +506,6 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
drm_modeset_unlock(&connector->dev->mode_config.connection_mutex);
drm_connector_unreference(connector);
- DRM_DEBUG_KMS("\n");
}
static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
--
2.11.0
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Pandiyan, Dhinakaran]

Post by Ausmus, James
On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan

Post by Dhinakaran Pandiyan
Print connector name in destroy_connect() and this doesn't add any extra
lines to dmesg. The debug macro has been moved before the unregister
call so that we don't lose the connector name and id.

While I was looking around here, though, I noticed we don't currently
handle error returns from the drm layer calls in
intel_dp_add_mst_connector. I'll try to send a patch along tomorrow
for that.

Thanks for the review and yeah, that does look wrong.

Post by Ausmus, James

Post by Dhinakaran Pandiyan
---
drivers/gpu/drm/i915/intel_dp_mst.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 8e3aad0ea60b..88d1d2b9ac56 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -494,6 +494,7 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
struct intel_connector *intel_connector = to_intel_connector(connector);
struct drm_i915_private *dev_priv = to_i915(connector->dev);
+ DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name);
drm_connector_unregister(connector);
if (dev_priv->fbdev)
@@ -505,7 +506,6 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
drm_modeset_unlock(&connector->dev->mode_config.connection_mutex);
drm_connector_unreference(connector);
- DRM_DEBUG_KMS("\n");
}
static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
--
2.11.0
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx