Re: [Intel-gfx] [PATCH v2 4/5] drm/i915: Pimp link training debug prints
On Wed, Oct 06, 2021 at 07:48:18PM +0300, Ville Syrjälä wrote: > On Wed, Oct 06, 2021 at 07:09:02PM +0300, Imre Deak wrote: > > On Mon, Oct 04, 2021 at 08:05:34PM +0300, Ville Syrjala wrote: > > > From: Ville Syrjälä > > > > > > Unify all debug prints during link training to include information > > > on both the encoder and the LTTPR. We unify the format to something > > > like "[ENCODER:1:FOO][LTTPR 1] Something something". Though not > > > sure if those brackets around the dp_phy just make it look like > > > line noise? I'll accept suggestions on better formatting. > > > > > > I'm slightly on the fence about also including the connector, > > > but technically only the DPRX is the SST connector (ie. > > > intel_dp->attached_connector). I suppose you could think of it > > > as the branch device/whatever in the topology, and we're training > > > the link leading to it. So that could argue for its inclusion. > > > But it's all getting a bit long alrady, so not going to do it > > > I think. > > > > Imo including the connector info eventually would be good to be able to > > match these lines with those only showing the connector, or connectors > > in i915_display_info etc. > > You're probably right. I was just looking at a dmesg wondering which > connector it's training there... > > Although with MST it of course doesn't match up with anything > that the user thinks as a connected connector. So a bit annoying. > And using a single MST connector wouldn't really lead to a > coherent debug message either since there could be many MST > connectors active on the same link :/ As a compromise I kept the SST connector name in the final passed/failed debug print. That was the only place where we printed it previously as well. Not ideal perhaps but at least it's something. Series pushed. Thanks for the review. -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH v2 4/5] drm/i915: Pimp link training debug prints
On Wed, Oct 06, 2021 at 07:09:02PM +0300, Imre Deak wrote: > On Mon, Oct 04, 2021 at 08:05:34PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä > > > > Unify all debug prints during link training to include information > > on both the encoder and the LTTPR. We unify the format to something > > like "[ENCODER:1:FOO][LTTPR 1] Something something". Though not > > sure if those brackets around the dp_phy just make it look like > > line noise? I'll accept suggestions on better formatting. > > > > I'm slightly on the fence about also including the connector, > > but technically only the DPRX is the SST connector (ie. > > intel_dp->attached_connector). I suppose you could think of it > > as the branch device/whatever in the topology, and we're training > > the link leading to it. So that could argue for its inclusion. > > But it's all getting a bit long alrady, so not going to do it > > I think. > > Imo including the connector info eventually would be good to be able to > match these lines with those only showing the connector, or connectors > in i915_display_info etc. You're probably right. I was just looking at a dmesg wondering which connector it's training there... Although with MST it of course doesn't match up with anything that the user thinks as a connected connector. So a bit annoying. And using a single MST connector wouldn't really lead to a coherent debug message either since there could be many MST connectors active on the same link :/ -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH v2 4/5] drm/i915: Pimp link training debug prints
On Mon, Oct 04, 2021 at 08:05:34PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Unify all debug prints during link training to include information
> on both the encoder and the LTTPR. We unify the format to something
> like "[ENCODER:1:FOO][LTTPR 1] Something something". Though not
> sure if those brackets around the dp_phy just make it look like
> line noise? I'll accept suggestions on better formatting.
>
> I'm slightly on the fence about also including the connector,
> but technically only the DPRX is the SST connector (ie.
> intel_dp->attached_connector). I suppose you could think of it
> as the branch device/whatever in the topology, and we're training
> the link leading to it. So that could argue for its inclusion.
> But it's all getting a bit long alrady, so not going to do it
> I think.
Imo including the connector info eventually would be good to be able to
match these lines with those only showing the connector, or connectors
in i915_display_info etc.
> Signed-off-by: Ville Syrjälä
Reviewed-by: Imre Deak
> ---
> .../drm/i915/display/intel_dp_link_training.c | 167 +++---
> 1 file changed, 106 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> index 5657be1461ec..18f4b469766e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -25,15 +25,6 @@
> #include "intel_dp.h"
> #include "intel_dp_link_training.h"
>
> -static void
> -intel_dp_dump_link_status(struct drm_device *drm,
> - const u8 link_status[DP_LINK_STATUS_SIZE])
> -{
> - drm_dbg_kms(drm,
> - "ln0_1:0x%x ln2_3:0x%x align:0x%x sink:0x%x adj_req0_1:0x%x
> adj_req2_3:0x%x\n",
> - link_status[0], link_status[1], link_status[2],
> - link_status[3], link_status[4], link_status[5]);
> -}
>
> static void intel_dp_reset_lttpr_common_caps(struct intel_dp *intel_dp)
> {
> @@ -66,6 +57,7 @@ static u8 *intel_dp_lttpr_phy_caps(struct intel_dp
> *intel_dp,
> static void intel_dp_read_lttpr_phy_caps(struct intel_dp *intel_dp,
>enum drm_dp_phy dp_phy)
> {
> + struct intel_encoder *encoder = _to_dig_port(intel_dp)->base;
> u8 *phy_caps = intel_dp_lttpr_phy_caps(intel_dp, dp_phy);
> char phy_name[10];
>
> @@ -73,21 +65,22 @@ static void intel_dp_read_lttpr_phy_caps(struct intel_dp
> *intel_dp,
>
> if (drm_dp_read_lttpr_phy_caps(_dp->aux, dp_phy, phy_caps) < 0) {
> drm_dbg_kms(_to_i915(intel_dp)->drm,
> - "failed to read the PHY caps for %s\n",
> - phy_name);
> + "[ENCODER:%d:%s][%s] failed to read the PHY caps\n",
> + encoder->base.base.id, encoder->base.name,
> phy_name);
> return;
> }
>
> drm_dbg_kms(_to_i915(intel_dp)->drm,
> - "%s PHY capabilities: %*ph\n",
> - phy_name,
> + "[ENCODER:%d:%s][%s] PHY capabilities: %*ph\n",
> + encoder->base.base.id, encoder->base.name, phy_name,
> (int)sizeof(intel_dp->lttpr_phy_caps[0]),
> phy_caps);
> }
>
> static bool intel_dp_read_lttpr_common_caps(struct intel_dp *intel_dp)
> {
> - struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> + struct intel_encoder *encoder = _to_dig_port(intel_dp)->base;
> + struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>
> if (intel_dp_is_edp(intel_dp))
> return false;
> @@ -104,7 +97,8 @@ static bool intel_dp_read_lttpr_common_caps(struct
> intel_dp *intel_dp)
> goto reset_caps;
>
> drm_dbg_kms(_to_i915(intel_dp)->drm,
> - "LTTPR common capabilities: %*ph\n",
> + "[ENCODER:%d:%s] LTTPR common capabilities: %*ph\n",
> + encoder->base.base.id, encoder->base.name,
> (int)sizeof(intel_dp->lttpr_common_caps),
> intel_dp->lttpr_common_caps);
>
> @@ -130,6 +124,8 @@ intel_dp_set_lttpr_transparent_mode(struct intel_dp
> *intel_dp, bool enable)
>
> static int intel_dp_init_lttpr(struct intel_dp *intel_dp)
> {
> + struct intel_encoder *encoder = _to_dig_port(intel_dp)->base;
> + struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> int lttpr_count;
> int i;
>
> @@ -161,8 +157,9 @@ static int intel_dp_init_lttpr(struct intel_dp *intel_dp)
> return 0;
>
> if (!intel_dp_set_lttpr_transparent_mode(intel_dp, false)) {
> - drm_dbg_kms(_to_i915(intel_dp)->drm,
> - "Switching to LTTPR non-transparent LT mode failed,
> fall-back to transparent mode\n");
> + drm_dbg_kms(>drm,
> + "[ENCODER:%d:%s] Switching to LTTPR
[Intel-gfx] [PATCH v2 4/5] drm/i915: Pimp link training debug prints
From: Ville Syrjälä
Unify all debug prints during link training to include information
on both the encoder and the LTTPR. We unify the format to something
like "[ENCODER:1:FOO][LTTPR 1] Something something". Though not
sure if those brackets around the dp_phy just make it look like
line noise? I'll accept suggestions on better formatting.
I'm slightly on the fence about also including the connector,
but technically only the DPRX is the SST connector (ie.
intel_dp->attached_connector). I suppose you could think of it
as the branch device/whatever in the topology, and we're training
the link leading to it. So that could argue for its inclusion.
But it's all getting a bit long alrady, so not going to do it
I think.
Signed-off-by: Ville Syrjälä
---
.../drm/i915/display/intel_dp_link_training.c | 167 +++---
1 file changed, 106 insertions(+), 61 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
index 5657be1461ec..18f4b469766e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
@@ -25,15 +25,6 @@
#include "intel_dp.h"
#include "intel_dp_link_training.h"
-static void
-intel_dp_dump_link_status(struct drm_device *drm,
- const u8 link_status[DP_LINK_STATUS_SIZE])
-{
- drm_dbg_kms(drm,
- "ln0_1:0x%x ln2_3:0x%x align:0x%x sink:0x%x adj_req0_1:0x%x
adj_req2_3:0x%x\n",
- link_status[0], link_status[1], link_status[2],
- link_status[3], link_status[4], link_status[5]);
-}
static void intel_dp_reset_lttpr_common_caps(struct intel_dp *intel_dp)
{
@@ -66,6 +57,7 @@ static u8 *intel_dp_lttpr_phy_caps(struct intel_dp *intel_dp,
static void intel_dp_read_lttpr_phy_caps(struct intel_dp *intel_dp,
enum drm_dp_phy dp_phy)
{
+ struct intel_encoder *encoder = _to_dig_port(intel_dp)->base;
u8 *phy_caps = intel_dp_lttpr_phy_caps(intel_dp, dp_phy);
char phy_name[10];
@@ -73,21 +65,22 @@ static void intel_dp_read_lttpr_phy_caps(struct intel_dp
*intel_dp,
if (drm_dp_read_lttpr_phy_caps(_dp->aux, dp_phy, phy_caps) < 0) {
drm_dbg_kms(_to_i915(intel_dp)->drm,
- "failed to read the PHY caps for %s\n",
- phy_name);
+ "[ENCODER:%d:%s][%s] failed to read the PHY caps\n",
+ encoder->base.base.id, encoder->base.name,
phy_name);
return;
}
drm_dbg_kms(_to_i915(intel_dp)->drm,
- "%s PHY capabilities: %*ph\n",
- phy_name,
+ "[ENCODER:%d:%s][%s] PHY capabilities: %*ph\n",
+ encoder->base.base.id, encoder->base.name, phy_name,
(int)sizeof(intel_dp->lttpr_phy_caps[0]),
phy_caps);
}
static bool intel_dp_read_lttpr_common_caps(struct intel_dp *intel_dp)
{
- struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+ struct intel_encoder *encoder = _to_dig_port(intel_dp)->base;
+ struct drm_i915_private *i915 = to_i915(encoder->base.dev);
if (intel_dp_is_edp(intel_dp))
return false;
@@ -104,7 +97,8 @@ static bool intel_dp_read_lttpr_common_caps(struct intel_dp
*intel_dp)
goto reset_caps;
drm_dbg_kms(_to_i915(intel_dp)->drm,
- "LTTPR common capabilities: %*ph\n",
+ "[ENCODER:%d:%s] LTTPR common capabilities: %*ph\n",
+ encoder->base.base.id, encoder->base.name,
(int)sizeof(intel_dp->lttpr_common_caps),
intel_dp->lttpr_common_caps);
@@ -130,6 +124,8 @@ intel_dp_set_lttpr_transparent_mode(struct intel_dp
*intel_dp, bool enable)
static int intel_dp_init_lttpr(struct intel_dp *intel_dp)
{
+ struct intel_encoder *encoder = _to_dig_port(intel_dp)->base;
+ struct drm_i915_private *i915 = to_i915(encoder->base.dev);
int lttpr_count;
int i;
@@ -161,8 +157,9 @@ static int intel_dp_init_lttpr(struct intel_dp *intel_dp)
return 0;
if (!intel_dp_set_lttpr_transparent_mode(intel_dp, false)) {
- drm_dbg_kms(_to_i915(intel_dp)->drm,
- "Switching to LTTPR non-transparent LT mode failed,
fall-back to transparent mode\n");
+ drm_dbg_kms(>drm,
+ "[ENCODER:%d:%s] Switching to LTTPR non-transparent
LT mode failed, fall-back to transparent mode\n",
+ encoder->base.base.id, encoder->base.name);
intel_dp_set_lttpr_transparent_mode(intel_dp, true);
intel_dp_reset_lttpr_count(intel_dp);
@@ -366,17 +363,18 @@ intel_dp_get_adjust_train(struct intel_dp *intel_dp,
const u8
