| Message ID | 1484693835-32269-2-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jani, Could you please review this patch? I have added the lane count checking and other review comments you had. Is there anything else that is blocking from getting this patch merged? Regards Manasi On Tue, Jan 17, 2017 at 02:57:12PM -0800, Manasi Navare wrote: > This patch adds support to handle automated DP compliance > link training test requests. This patch has been tested with > Unigraf DPR-120 DP Compliance device for testing Link > Training Compliance. > After we get a short pulse Compliance test request, test > request values are read and hotplug uevent is sent in order > to trigger another modeset during which the pipe is configured > and link is retrained and enabled for link parameters requested > by the test. > > v2: > * Validate the test lane count before using it in > intel_dp_compute_config (Jani Nikula) > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 46 ++++++++++++++++++++++++++++++++++++---- > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > 2 files changed, 44 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index e80d620..7a1d3c4 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1613,6 +1613,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, > /* Conveniently, the link BW constants become indices with a shift...*/ > int min_clock = 0; > int max_clock; > + int link_rate_index; > int bpp, mode_rate; > int link_avail, link_clock; > int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > @@ -1654,6 +1655,17 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > return false; > > + /* Use values requested by Compliance Test Request */ > + if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) { > + link_rate_index = intel_dp_link_rate_index(intel_dp, > + common_rates, > + drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate)); > + if (link_rate_index >= 0) > + min_clock = max_clock = link_rate_index; > + if (min_lane_count <= intel_dp->compliance.test_lane_count > + && intel_dp->compliance.test_lane_count >= max_lane_count) > + min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count; > + } > DRM_DEBUG_KMS("DP link computation with max lane count %i " > "max bw %d pixel clock %iKHz\n", > max_lane_count, common_rates[max_clock], > @@ -3921,6 +3933,27 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp) > { > uint8_t test_result = DP_TEST_ACK; > + int status = 0; > + /* (DP CTS 1.2) > + * 4.3.1.11 > + */ > + /* Read the TEST_LANE_COUNT and TEST_LINK_RTAE fields (DP CTS 3.1.4) */ > + status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LANE_COUNT, > + &intel_dp->compliance.test_lane_count); > + > + if (status <= 0) { > + DRM_DEBUG_KMS("Lane count read failed\n"); > + return 0; > + } > + intel_dp->compliance.test_lane_count &= DP_MAX_LANE_COUNT_MASK; > + > + status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE, > + &intel_dp->compliance.test_link_rate); > + if (status <= 0) { > + DRM_DEBUG_KMS("Link Rate read failed\n"); > + return 0; > + } > + > return test_result; > } > > @@ -4135,9 +4168,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) > if (!intel_dp->lane_count) > return; > > - /* if link training is requested we should perform it always */ > - if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) || > - (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) { > + /* Retrain if Channel EQ or CR not ok */ > + if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { > DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", > intel_encoder->base.name); > > @@ -4162,6 +4194,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) > intel_dp_short_pulse(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > + struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; > u8 sink_irq_vector = 0; > u8 old_sink_count = intel_dp->sink_count; > bool ret; > @@ -4195,7 +4228,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) > sink_irq_vector); > > if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) > - DRM_DEBUG_DRIVER("Test request in short pulse not handled\n"); > + intel_dp_handle_test_request(intel_dp); > if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ)) > DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); > } > @@ -4203,6 +4236,11 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > intel_dp_check_link_status(intel_dp); > drm_modeset_unlock(&dev->mode_config.connection_mutex); > + if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) { > + DRM_DEBUG_KMS("Link Training Compliance Test requested\n"); > + /* Send a Hotplug Uevent to userspace to start modeset */ > + drm_kms_helper_hotplug_event(intel_encoder->base.dev); > + } > > return true; > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 84258df..7a2eb76 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -893,6 +893,8 @@ struct intel_dp_compliance { > unsigned long test_type; > struct intel_dp_compliance_data test_data; > bool test_active; > + u8 test_link_rate; > + u8 test_lane_count; > }; > > struct intel_dp { > -- > 1.9.1 >
On Wed, 18 Jan 2017, Manasi Navare <manasi.d.navare@intel.com> wrote: > This patch adds support to handle automated DP compliance > link training test requests. This patch has been tested with > Unigraf DPR-120 DP Compliance device for testing Link > Training Compliance. > After we get a short pulse Compliance test request, test > request values are read and hotplug uevent is sent in order > to trigger another modeset during which the pipe is configured > and link is retrained and enabled for link parameters requested > by the test. > > v2: > * Validate the test lane count before using it in > intel_dp_compute_config (Jani Nikula) > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 46 ++++++++++++++++++++++++++++++++++++---- > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > 2 files changed, 44 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index e80d620..7a1d3c4 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1613,6 +1613,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, > /* Conveniently, the link BW constants become indices with a shift...*/ > int min_clock = 0; > int max_clock; > + int link_rate_index; > int bpp, mode_rate; > int link_avail, link_clock; > int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > @@ -1654,6 +1655,17 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > return false; > > + /* Use values requested by Compliance Test Request */ > + if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) { > + link_rate_index = intel_dp_link_rate_index(intel_dp, > + common_rates, > + drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate)); > + if (link_rate_index >= 0) > + min_clock = max_clock = link_rate_index; > + if (min_lane_count <= intel_dp->compliance.test_lane_count > + && intel_dp->compliance.test_lane_count >= max_lane_count) > + min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count; > + } Why don't we do these checks and conversions when we get the test request, so we can NAK the request on errors? > DRM_DEBUG_KMS("DP link computation with max lane count %i " > "max bw %d pixel clock %iKHz\n", > max_lane_count, common_rates[max_clock], > @@ -3921,6 +3933,27 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp) > { > uint8_t test_result = DP_TEST_ACK; > + int status = 0; > + /* (DP CTS 1.2) > + * 4.3.1.11 > + */ > + /* Read the TEST_LANE_COUNT and TEST_LINK_RTAE fields (DP CTS 3.1.4) */ > + status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LANE_COUNT, > + &intel_dp->compliance.test_lane_count); > + > + if (status <= 0) { > + DRM_DEBUG_KMS("Lane count read failed\n"); > + return 0; return DP_TEST_NAK; > + } > + intel_dp->compliance.test_lane_count &= DP_MAX_LANE_COUNT_MASK; > + > + status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE, > + &intel_dp->compliance.test_link_rate); > + if (status <= 0) { > + DRM_DEBUG_KMS("Link Rate read failed\n"); > + return 0; return DP_TEST_NAK; > + } > + > return test_result; Could replace this with a direct return DP_TEST_ACK without the temp variable. > } > > @@ -4135,9 +4168,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) > if (!intel_dp->lane_count) > return; > > - /* if link training is requested we should perform it always */ > - if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) || > - (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) { > + /* Retrain if Channel EQ or CR not ok */ > + if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { > DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", > intel_encoder->base.name); > > @@ -4162,6 +4194,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) > intel_dp_short_pulse(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > + struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; > u8 sink_irq_vector = 0; > u8 old_sink_count = intel_dp->sink_count; > bool ret; > @@ -4195,7 +4228,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) > sink_irq_vector); > > if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) > - DRM_DEBUG_DRIVER("Test request in short pulse not handled\n"); > + intel_dp_handle_test_request(intel_dp); > if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ)) > DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); > } > @@ -4203,6 +4236,11 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > intel_dp_check_link_status(intel_dp); > drm_modeset_unlock(&dev->mode_config.connection_mutex); > + if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) { > + DRM_DEBUG_KMS("Link Training Compliance Test requested\n"); > + /* Send a Hotplug Uevent to userspace to start modeset */ > + drm_kms_helper_hotplug_event(intel_encoder->base.dev); > + } > > return true; > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 84258df..7a2eb76 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -893,6 +893,8 @@ struct intel_dp_compliance { > unsigned long test_type; > struct intel_dp_compliance_data test_data; > bool test_active; > + u8 test_link_rate; > + u8 test_lane_count; > }; > > struct intel_dp {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index e80d620..7a1d3c4 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1613,6 +1613,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, /* Conveniently, the link BW constants become indices with a shift...*/ int min_clock = 0; int max_clock; + int link_rate_index; int bpp, mode_rate; int link_avail, link_clock; int common_rates[DP_MAX_SUPPORTED_RATES] = {}; @@ -1654,6 +1655,17 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) return false; + /* Use values requested by Compliance Test Request */ + if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) { + link_rate_index = intel_dp_link_rate_index(intel_dp, + common_rates, + drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate)); + if (link_rate_index >= 0) + min_clock = max_clock = link_rate_index; + if (min_lane_count <= intel_dp->compliance.test_lane_count + && intel_dp->compliance.test_lane_count >= max_lane_count) + min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count; + } DRM_DEBUG_KMS("DP link computation with max lane count %i " "max bw %d pixel clock %iKHz\n", max_lane_count, common_rates[max_clock], @@ -3921,6 +3933,27 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp) { uint8_t test_result = DP_TEST_ACK; + int status = 0; + /* (DP CTS 1.2) + * 4.3.1.11 + */ + /* Read the TEST_LANE_COUNT and TEST_LINK_RTAE fields (DP CTS 3.1.4) */ + status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LANE_COUNT, + &intel_dp->compliance.test_lane_count); + + if (status <= 0) { + DRM_DEBUG_KMS("Lane count read failed\n"); + return 0; + } + intel_dp->compliance.test_lane_count &= DP_MAX_LANE_COUNT_MASK; + + status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE, + &intel_dp->compliance.test_link_rate); + if (status <= 0) { + DRM_DEBUG_KMS("Link Rate read failed\n"); + return 0; + } + return test_result; } @@ -4135,9 +4168,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) if (!intel_dp->lane_count) return; - /* if link training is requested we should perform it always */ - if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) || - (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) { + /* Retrain if Channel EQ or CR not ok */ + if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", intel_encoder->base.name); @@ -4162,6 +4194,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) intel_dp_short_pulse(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); + struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; u8 sink_irq_vector = 0; u8 old_sink_count = intel_dp->sink_count; bool ret; @@ -4195,7 +4228,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) sink_irq_vector); if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) - DRM_DEBUG_DRIVER("Test request in short pulse not handled\n"); + intel_dp_handle_test_request(intel_dp); if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ)) DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); } @@ -4203,6 +4236,11 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); intel_dp_check_link_status(intel_dp); drm_modeset_unlock(&dev->mode_config.connection_mutex); + if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) { + DRM_DEBUG_KMS("Link Training Compliance Test requested\n"); + /* Send a Hotplug Uevent to userspace to start modeset */ + drm_kms_helper_hotplug_event(intel_encoder->base.dev); + } return true; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 84258df..7a2eb76 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -893,6 +893,8 @@ struct intel_dp_compliance { unsigned long test_type; struct intel_dp_compliance_data test_data; bool test_active; + u8 test_link_rate; + u8 test_lane_count; }; struct intel_dp {
This patch adds support to handle automated DP compliance link training test requests. This patch has been tested with Unigraf DPR-120 DP Compliance device for testing Link Training Compliance. After we get a short pulse Compliance test request, test request values are read and hotplug uevent is sent in order to trigger another modeset during which the pipe is configured and link is retrained and enabled for link parameters requested by the test. v2: * Validate the test lane count before using it in intel_dp_compute_config (Jani Nikula) Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 46 ++++++++++++++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_drv.h | 2 ++ 2 files changed, 44 insertions(+), 4 deletions(-)