| Message ID | 2daf3079a687f128c95086cc1abad9420e6c91de.1522938790.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 05, 2018 at 05:39:00PM +0300, Jani Nikula wrote: > The debug prints make more sense where the results are actually used, > and this cleans up extra clutter from the already overcrowded > intel_dp_compute_config(). > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 9 ++------- > drivers/gpu/drm/i915/intel_dp_link_training.c | 5 +++++ > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 5f4b30faf6a2..81cf363e71af 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1706,7 +1706,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, > int bpp, mode_rate; > int link_avail, link_clock; > int common_len; > - uint8_t link_bw, rate_select; > bool reduce_m_n = drm_dp_has_quirk(&intel_dp->desc, > DP_DPCD_QUIRK_LIMITED_M_N); > > @@ -1852,12 +1851,8 @@ intel_dp_compute_config(struct intel_encoder *encoder, > pipe_config->pipe_bpp = bpp; > pipe_config->port_clock = intel_dp->common_rates[clock]; > > - intel_dp_compute_rate(intel_dp, pipe_config->port_clock, > - &link_bw, &rate_select); the commit message state more about the debug message itself, but by removing this call here it seems that you change the behavior of eDP. So I couldn't follow the changes you are actually aiming to do here. > - > - DRM_DEBUG_KMS("DP link bw %02x rate select %02x lane count %d clock %d bpp %d\n", > - link_bw, rate_select, pipe_config->lane_count, > - pipe_config->port_clock, bpp); > + DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n", > + pipe_config->lane_count, pipe_config->port_clock, bpp); > DRM_DEBUG_KMS("DP link bw required %i available %i\n", > mode_rate, link_avail); > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > index f59b59bb0a21..3fcaa98b9055 100644 > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > @@ -139,6 +139,11 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) > intel_dp_compute_rate(intel_dp, intel_dp->link_rate, > &link_bw, &rate_select); > > + if (link_bw) > + DRM_DEBUG_KMS("Using LINK_BW_SET value %02x\n", link_bw); > + else > + DRM_DEBUG_KMS("Using LINK_RATE_SET value %02x\n", rate_select); > + > /* Write the link configuration data */ > link_config[0] = link_bw; > link_config[1] = intel_dp->lane_count; > -- > 2.11.0 >
On Thu, Apr 05, 2018 at 10:22:38AM -0700, Rodrigo Vivi wrote: > On Thu, Apr 05, 2018 at 05:39:00PM +0300, Jani Nikula wrote: > > The debug prints make more sense where the results are actually used, > > and this cleans up extra clutter from the already overcrowded > > intel_dp_compute_config(). > > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 9 ++------- > > drivers/gpu/drm/i915/intel_dp_link_training.c | 5 +++++ > > 2 files changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 5f4b30faf6a2..81cf363e71af 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1706,7 +1706,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, > > int bpp, mode_rate; > > int link_avail, link_clock; > > int common_len; > > - uint8_t link_bw, rate_select; > > bool reduce_m_n = drm_dp_has_quirk(&intel_dp->desc, > > DP_DPCD_QUIRK_LIMITED_M_N); > > > > @@ -1852,12 +1851,8 @@ intel_dp_compute_config(struct intel_encoder *encoder, > > pipe_config->pipe_bpp = bpp; > > pipe_config->port_clock = intel_dp->common_rates[clock]; > > > > - intel_dp_compute_rate(intel_dp, pipe_config->port_clock, > > - &link_bw, &rate_select); > > the commit message state more about the debug message itself, > but by removing this call here it seems that you change > the behavior of eDP. So I couldn't follow the changes you are > actually aiming to do here. > Actually in dp_compute_config(), we just compute the optimum port_clock selected from the common_rates array. We dont really use rate_select or link_bw here in this function. These two parameters get used directly in the clock_recovery where we either write rate_select or link_bw to the dpcd link config registers. So by removing this call to intel_dp_compute_rate() from compute_config() makes sense to me. And it does not alter the eDP compute config behaviour. However I think to be clear, the above explanation needs to be included in th commit message. With that included, Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> Manasi > > - > > - DRM_DEBUG_KMS("DP link bw %02x rate select %02x lane count %d clock %d bpp %d\n", > > - link_bw, rate_select, pipe_config->lane_count, > > - pipe_config->port_clock, bpp); > > + DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n", > > + pipe_config->lane_count, pipe_config->port_clock, bpp); > > DRM_DEBUG_KMS("DP link bw required %i available %i\n", > > mode_rate, link_avail); > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > > index f59b59bb0a21..3fcaa98b9055 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > > @@ -139,6 +139,11 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) > > intel_dp_compute_rate(intel_dp, intel_dp->link_rate, > > &link_bw, &rate_select); > > > > + if (link_bw) > > + DRM_DEBUG_KMS("Using LINK_BW_SET value %02x\n", link_bw); > > + else > > + DRM_DEBUG_KMS("Using LINK_RATE_SET value %02x\n", rate_select); > > + > > /* Write the link configuration data */ > > link_config[0] = link_bw; > > link_config[1] = intel_dp->lane_count; > > -- > > 2.11.0 > >
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 5f4b30faf6a2..81cf363e71af 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1706,7 +1706,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, int bpp, mode_rate; int link_avail, link_clock; int common_len; - uint8_t link_bw, rate_select; bool reduce_m_n = drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_LIMITED_M_N); @@ -1852,12 +1851,8 @@ intel_dp_compute_config(struct intel_encoder *encoder, pipe_config->pipe_bpp = bpp; pipe_config->port_clock = intel_dp->common_rates[clock]; - intel_dp_compute_rate(intel_dp, pipe_config->port_clock, - &link_bw, &rate_select); - - DRM_DEBUG_KMS("DP link bw %02x rate select %02x lane count %d clock %d bpp %d\n", - link_bw, rate_select, pipe_config->lane_count, - pipe_config->port_clock, bpp); + DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n", + pipe_config->lane_count, pipe_config->port_clock, bpp); DRM_DEBUG_KMS("DP link bw required %i available %i\n", mode_rate, link_avail); diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c index f59b59bb0a21..3fcaa98b9055 100644 --- a/drivers/gpu/drm/i915/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c @@ -139,6 +139,11 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) intel_dp_compute_rate(intel_dp, intel_dp->link_rate, &link_bw, &rate_select); + if (link_bw) + DRM_DEBUG_KMS("Using LINK_BW_SET value %02x\n", link_bw); + else + DRM_DEBUG_KMS("Using LINK_RATE_SET value %02x\n", rate_select); + /* Write the link configuration data */ link_config[0] = link_bw; link_config[1] = intel_dp->lane_count;
The debug prints make more sense where the results are actually used, and this cleans up extra clutter from the already overcrowded intel_dp_compute_config(). Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 9 ++------- drivers/gpu/drm/i915/intel_dp_link_training.c | 5 +++++ 2 files changed, 7 insertions(+), 7 deletions(-)