| Message ID | 20240301074923.485807-1-charlton.lin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/i915/dp: Log message when limiting SST link rate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit | expand |
On Thu, 29 Feb 2024, Charlton Lin <charlton.lin@intel.com> wrote: > Driver currently limits link rate up to HBR3 in SST mode. Log a > message with monitor vendor, product id, and MSTM_CAP to > help understand what monitors are being downgraded by this limit. Any logging of the sink details should be done exactly once at detect time. No matter what happens after that, there's no need to spam the dmesg with duplicate information. If the information currently logged is insufficient, please add it at detect, where it helps *all* debugging, not just a single restricted case. More details inline. > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Khaled Almahallawy <khaled.almahallawy@intel.com> > Cc: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Charlton Lin <charlton.lin@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 6ece2c563c7a..0b2d6d88fd37 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -2437,6 +2437,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder, > false, > &limits); > > + if (intel_dp_max_common_rate(intel_dp) > limits.max_rate) { If link training has failed and the max rate is limited due to that, you'll hit this on all retraining attempts. And, of course, this is hit at every single modeset for 128b/132b capable displays that don't support MST. That's just excessive. > + u8 mstm_cap; > + u32 panel_id = drm_edid_get_panel_id(&intel_dp->aux.ddc); That reads the entire EDID base block again in the middle of compute config. A big no. We've also taken care to cache the EDID to avoid duplicate reads otherwise. Moreover, it ignores any EDID overrides etc. > + char vend[4]; > + u16 product_id; > + > + drm_dbg_kms(&i915->drm, > + "Limiting LR from max common rate %d to %d\n", > + intel_dp_max_common_rate(intel_dp), limits.max_rate); > + > + drm_edid_decode_panel_id(panel_id, vend, &product_id); > + > + if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_12 && > + drm_dp_dpcd_readb(&intel_dp->aux, DP_MSTM_CAP, &mstm_cap) == 1) We shouldn't add extra DPCD reads nilly willy either. This should be debug logged once at detect. I've got a WIP series that should address this [1], once I fix it. > + drm_dbg_kms(&i915->drm, > + "Manufacturer=%s Model=%x Sink MSTM_CAP=%x\n", > + vend, product_id, mstm_cap); Ville and I have discussed adding entire EDID debug logging at drm_edid.c level, which would address a lot more things than this. BR, Jani. [1] https://patchwork.freedesktop.org/series/129468/ > + } > + > if (!dsc_needed) { > /* > * Optimize for slow and wide for everything, because there are some
On Fri, Mar 01, 2024 at 11:07:43AM +0200, Jani Nikula wrote: > On Thu, 29 Feb 2024, Charlton Lin <charlton.lin@intel.com> wrote: > > Driver currently limits link rate up to HBR3 in SST mode. Log a > > message with monitor vendor, product id, and MSTM_CAP to > > help understand what monitors are being downgraded by this limit. > > Any logging of the sink details should be done exactly once at detect > time. No matter what happens after that, there's no need to spam the > dmesg with duplicate information. If the information currently logged is > insufficient, please add it at detect, where it helps *all* debugging, > not just a single restricted case. > > More details inline. > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Khaled Almahallawy <khaled.almahallawy@intel.com> > > Cc: Sean Paul <seanpaul@chromium.org> > > Signed-off-by: Charlton Lin <charlton.lin@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 6ece2c563c7a..0b2d6d88fd37 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -2437,6 +2437,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder, > > false, > > &limits); > > > > + if (intel_dp_max_common_rate(intel_dp) > limits.max_rate) { > > If link training has failed and the max rate is limited due to that, > you'll hit this on all retraining attempts. > > And, of course, this is hit at every single modeset for 128b/132b > capable displays that don't support MST. That's just excessive. > > > + u8 mstm_cap; > > + u32 panel_id = drm_edid_get_panel_id(&intel_dp->aux.ddc); > > That reads the entire EDID base block again in the middle of compute > config. A big no. We've also taken care to cache the EDID to avoid > duplicate reads otherwise. > > Moreover, it ignores any EDID overrides etc. > > > + char vend[4]; > > + u16 product_id; > > + > > + drm_dbg_kms(&i915->drm, > > + "Limiting LR from max common rate %d to %d\n", > > + intel_dp_max_common_rate(intel_dp), limits.max_rate); > > + > > + drm_edid_decode_panel_id(panel_id, vend, &product_id); > > + > > + if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_12 && > > + drm_dp_dpcd_readb(&intel_dp->aux, DP_MSTM_CAP, &mstm_cap) == 1) > > We shouldn't add extra DPCD reads nilly willy either. This should be > debug logged once at detect. I've got a WIP series that should address > this [1], once I fix it. Also the stone tablets say "thou shalt not touch the hardware in atomic_check()/.compute_config()". This stuff gets executed for every TEST_ONLY commit, which means it: - potentially runs while the hardware is asleep - should be relatively fast > > > + drm_dbg_kms(&i915->drm, > > + "Manufacturer=%s Model=%x Sink MSTM_CAP=%x\n", > > + vend, product_id, mstm_cap); > > Ville and I have discussed adding entire EDID debug logging at > drm_edid.c level, which would address a lot more things than this. > > BR, > Jani. > > > [1] https://patchwork.freedesktop.org/series/129468/ > > > > > + } > > + > > if (!dsc_needed) { > > /* > > * Optimize for slow and wide for everything, because there are some > > -- > Jani Nikula, Intel
Thanks Charlton for the patch. I think in general it is a good idea to log this when the max rate is dropped to HBR3 for SST case. Please find my comments below, On Thu, Feb 29, 2024 at 11:49 PM Charlton Lin <charlton.lin@intel.com> wrote: > > Driver currently limits link rate up to HBR3 in SST mode. Log a > message with monitor vendor, product id, and MSTM_CAP to > help understand what monitors are being downgraded by this limit. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Khaled Almahallawy <khaled.almahallawy@intel.com> > Cc: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Charlton Lin <charlton.lin@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 6ece2c563c7a..0b2d6d88fd37 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -2437,6 +2437,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder, > false, > &limits); > > + if (intel_dp_max_common_rate(intel_dp) > limits.max_rate) { > + u8 mstm_cap; > + u32 panel_id = drm_edid_get_panel_id(&intel_dp->aux.ddc); > + char vend[4]; > + u16 product_id; > + > + drm_dbg_kms(&i915->drm, > + "Limiting LR from max common rate %d to %d\n", We dont use LR acronym anywhere in the kernel for link rate, just say link rate here. Also I think would be good to log the reason why we are dropping this to HBR3 or add a comment with a Todo for this Manasi > + intel_dp_max_common_rate(intel_dp), limits.max_rate); > + > + drm_edid_decode_panel_id(panel_id, vend, &product_id); > + > + if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_12 && > + drm_dp_dpcd_readb(&intel_dp->aux, DP_MSTM_CAP, &mstm_cap) == 1) > + drm_dbg_kms(&i915->drm, > + "Manufacturer=%s Model=%x Sink MSTM_CAP=%x\n", > + vend, product_id, mstm_cap); > + } > + > if (!dsc_needed) { > /* > * Optimize for slow and wide for everything, because there are some > -- > 2.25.1 >
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 6ece2c563c7a..0b2d6d88fd37 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -2437,6 +2437,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder, false, &limits); + if (intel_dp_max_common_rate(intel_dp) > limits.max_rate) { + u8 mstm_cap; + u32 panel_id = drm_edid_get_panel_id(&intel_dp->aux.ddc); + char vend[4]; + u16 product_id; + + drm_dbg_kms(&i915->drm, + "Limiting LR from max common rate %d to %d\n", + intel_dp_max_common_rate(intel_dp), limits.max_rate); + + drm_edid_decode_panel_id(panel_id, vend, &product_id); + + if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_12 && + drm_dp_dpcd_readb(&intel_dp->aux, DP_MSTM_CAP, &mstm_cap) == 1) + drm_dbg_kms(&i915->drm, + "Manufacturer=%s Model=%x Sink MSTM_CAP=%x\n", + vend, product_id, mstm_cap); + } + if (!dsc_needed) { /* * Optimize for slow and wide for everything, because there are some
Driver currently limits link rate up to HBR3 in SST mode. Log a message with monitor vendor, product id, and MSTM_CAP to help understand what monitors are being downgraded by this limit. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Khaled Almahallawy <khaled.almahallawy@intel.com> Cc: Sean Paul <seanpaul@chromium.org> Signed-off-by: Charlton Lin <charlton.lin@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)