On Tue, 22 Nov 2016, Manasi Navare <manasi.d.nav...@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.
>
> Signed-off-by: Manasi Navare <manasi.d.nav...@intel.com>
> Cc: Jani Nikula <jani.nik...@linux.intel.com>
> Cc: Daniel Vetter <daniel.vet...@intel.com>
> Cc: Ville Syrjala <ville.syrj...@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 73
> ++++++++++++++++++++++++++++++++++++----
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> 2 files changed, 69 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 90283ed..69944d1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -288,6 +288,21 @@ static int intel_dp_common_rates(struct intel_dp
> *intel_dp,
> common_rates);
> }
>
> +static int intel_dp_link_rate_index(struct intel_dp *intel_dp,
> + int *common_rates, int link_rate)
> +{
> + int common_len;
> + int index;
> +
> + common_len = intel_dp_common_rates(intel_dp, common_rates);
> + for (index = 0; index < common_len; index++) {
> + if (link_rate == common_rates[common_len - index - 1])
> + return common_len - index - 1;
> + }
> +
> + return -1;
> +}
> +
> static enum drm_mode_status
> intel_dp_mode_valid(struct drm_connector *connector,
> struct drm_display_mode *mode)
> @@ -1554,6 +1569,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] = {};
> @@ -1595,6 +1611,16 @@ 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;
> + min_lane_count = max_lane_count =
> intel_dp->compliance_test_lane_count;
You need to be more strict about validating
compliance_test_lane_count. You do mask it with DP_MAX_LANE_COUNT_MASK,
but that's 0x1f, quite a few more lanes than we have...
> + }
> +
> 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],
> @@ -1642,6 +1668,7 @@ static int intel_dp_compute_bpp(struct intel_dp
> *intel_dp,
> }
> }
> }
> +
Please pay attention to not making unrelated changes.
> }
>
> return false;
> @@ -3804,6 +3831,29 @@ 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("Could not read test lane count from "
> + "reference sink\n");
No need to be so verbose, DRM_DEBUG_KMS will include the function name,
so a simple "Lane count read failed" or something will suffice.
> + return 0;
Should these return DP_TEST_NAK on errors or what?
> + }
> + 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("Could not read test link rate from "
> + "refernce sink\n");
Ditto.
> + return 0;
> + }
> +
> return test_result;
> }
>
> @@ -3908,7 +3958,8 @@ static void intel_dp_handle_test_request(struct
> intel_dp *intel_dp)
> DP_TEST_RESPONSE,
> &response, 1);
> if (status <= 0)
> - DRM_DEBUG_KMS("Could not write test response to sink\n");
> + DRM_DEBUG_KMS("Could not write test response "
> + "to sink\n");
Unrelated change, and one we don't want.
> }
>
> static int
> @@ -4018,9 +4069,8 @@ static void intel_dp_handle_test_request(struct
> intel_dp *intel_dp)
> if (WARN_ON_ONCE(!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))) {
Too many braces.
> DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> intel_encoder->base.name);
>
> @@ -4045,6 +4095,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;
> @@ -4056,6 +4107,8 @@ static void intel_dp_handle_test_request(struct
> intel_dp *intel_dp)
> intel_dp->compliance_test_active = 0;
> intel_dp->compliance_test_type = 0;
> intel_dp->compliance_test_data = 0;
> + intel_dp->compliance_test_lane_count = 0;
> + intel_dp->compliance_test_link_rate = 0;
Looks like compliance stuff should be a sub struct in intel_dp, and you
could just memset it to 0.
>
> /*
> * Now read the DPCD to see if it's actually running
> @@ -4079,8 +4132,9 @@ static void intel_dp_handle_test_request(struct
> intel_dp *intel_dp)
> DP_DEVICE_SERVICE_IRQ_VECTOR,
> sink_irq_vector);
>
> - if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
> - DRM_DEBUG_DRIVER("Test request in short pulse not
> handled\n");
> + if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) {
> + intel_dp_handle_test_request(intel_dp);
> + }
Unnecessary curly braces.
> if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
> DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
> }
> @@ -4088,6 +4142,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)) {
Too many braces.
> + 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;
> }
> @@ -4375,6 +4434,8 @@ static bool intel_digital_port_connected(struct
> drm_i915_private *dev_priv,
> intel_dp->compliance_test_active = 0;
> intel_dp->compliance_test_type = 0;
> intel_dp->compliance_test_data = 0;
> + intel_dp->compliance_test_lane_count = 0;
> + intel_dp->compliance_test_link_rate = 0;
Same thing about making compliance sub struct.
>
> if (intel_dp->is_mst) {
> DRM_DEBUG_KMS("MST device may have disappeared %d vs
> %d\n",
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index cd132c2..1e88288 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -958,6 +958,8 @@ struct intel_dp {
> unsigned long compliance_test_type;
> unsigned long compliance_test_data;
> bool compliance_test_active;
> + u8 compliance_test_lane_count;
> + u8 compliance_test_link_rate;
> };
>
> struct intel_lspcon {
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
