Re: [PATCH 1/9] drm/i915: Add automated testing support for Displayport compliance testing

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 4/6/15 5:10 PM, Paulo Zanoni wrote:
2015-03-31 14:14 GMT-03:00 Todd Previte <tprevite@xxxxxxxxx>:
Add the skeleton framework for supporting automation for Displayport compliance
testing. This patch adds the necessary framework for the source device to
appropriately respond to test automation requests from a sink device.

V2:
- Addressed previous mailing list feedback
- Fixed compilation issue (struct members declared in a later patch)
- Updated debug messages to be more accurate
- Added status checks for the DPCD read/write calls
- Removed excess comments and debug messages
- Fixed debug message compilation warnings
- Fixed compilation issue with missing variables
- Updated link training autotest to ACK

V3:
- Fixed the checks on the DPCD return code to be <= 0
   rather than != 0
- Removed extraneous assignment of a NAK return code in the
   DPCD read failure case
- Changed the return in the DPCD read failure case to a goto
   to the exit point where the status code is written to the sink
- Removed FAUX test case since it's deprecated now
- Removed the compliance flag assignment in handle_test_request

V4:
- Moved declaration of type_type here
- Removed declaration of test_data (moved to a later patch)
- Added reset to 0 for compliance test variables

Signed-off-by: Todd Previte <tprevite@xxxxxxxxx>
---
  drivers/gpu/drm/i915/intel_dp.c  | 75 +++++++++++++++++++++++++++++++++++++---
  drivers/gpu/drm/i915/intel_drv.h |  4 +++
  2 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index eea9e36..960cc68 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3746,11 +3746,78 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
         return true;
  }

-static void
-intel_dp_handle_test_request(struct intel_dp *intel_dp)
+static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
+{
+       uint8_t test_result = DP_TEST_ACK;
+       return test_result;
+}
+
+static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
+{
+       uint8_t test_result = DP_TEST_NAK;
+       return test_result;
+}
+
+static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
+{
+       uint8_t test_result = DP_TEST_NAK;
+       return test_result;
+}
+
+static uint8_t intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
+{
+       uint8_t test_result = DP_TEST_NAK;
+       return test_result;
+}
+
+static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
  {
-       /* NAK by default */
-       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
+       uint8_t response = DP_TEST_NAK;
+       uint8_t rxdata = 0;
+       int status = 0;
+
+       intel_dp->compliance_testing_active = 0;
+       intel_dp->aux.i2c_nack_count = 0;
+       intel_dp->aux.i2c_defer_count = 0;
+
+       status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
+       if (status <= 0) {
+               DRM_DEBUG_KMS("Could not read test request from sink\n");
+               goto update_status;
+       }
+
+       switch (rxdata) {
+       case DP_TEST_LINK_TRAINING:
+               DRM_DEBUG_KMS("LINK_TRAINING test requested\n");
+               intel_dp->compliance_test_type = DP_TEST_LINK_TRAINING;
+               response = intel_dp_autotest_link_training(intel_dp);
+               break;
+       case DP_TEST_LINK_VIDEO_PATTERN:
+               DRM_DEBUG_KMS("TEST_PATTERN test requested\n");
+               intel_dp->compliance_test_type = DP_TEST_LINK_VIDEO_PATTERN;
+               response = intel_dp_autotest_video_pattern(intel_dp);
+               break;
+       case DP_TEST_LINK_EDID_READ:
+               DRM_DEBUG_KMS("EDID test requested\n");
+               intel_dp->compliance_test_type = DP_TEST_LINK_EDID_READ;
+               response = intel_dp_autotest_edid(intel_dp);
+               break;
+       case DP_TEST_LINK_PHY_TEST_PATTERN:
+               DRM_DEBUG_KMS("PHY_PATTERN test requested\n");
+               intel_dp->compliance_test_type = DP_TEST_LINK_PHY_TEST_PATTERN;
+               response = intel_dp_autotest_phy_pattern(intel_dp);
+               break;
+       default:
+               DRM_DEBUG_KMS("Invalid test request '%02x'\n", rxdata);
+               break;
+       }
+
+update_status:
+       status = drm_dp_dpcd_write(&intel_dp->aux,
+                                  DP_TEST_RESPONSE,
+                                  &response, 1);
+       if (status <= 0)
+               DRM_DEBUG_KMS("Could not write test response to sink\n");
  }

  static int
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index eef79cc..e7b62be 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -647,6 +647,10 @@ struct intel_dp {
                                      bool has_aux_irq,
                                      int send_bytes,
                                      uint32_t aux_clock_divider);
+
+       /* Displayport compliance testing */
+       unsigned long compliance_test_type;
Shouldn't this have a default/initialized value? I see we assign
values to it, but then we never assign back to a value that means "not
testing anything". It's hard to see if this is a problem since this
variable is not really used on this patch. Ideally, the definition and
assignments should be placed on the patch that actually uses them
(patch 8).

I also see that on patch 5 you change this to char instead of long,
but you still don't use it for anything... This is a little confusing.
Yeah I was trying to get everything placed correctly but apparently I missed on some of the variables. They all should be initialized at the top of the handle_test_request function. Each time a request comes in, those variables need to be reset so that's why they need to be there. I'll work on it more tonight and tomorrow to see if I can get everything put in the right place/patch.

+       bool compliance_testing_active;
Same thing for this: ideally it should be defined on the patch that
actually does something with the variable.

Also, one variable is compliance_test_ and the other is
compliance_testING_ . It would be nice to keep both in the same
"namespace".

Anyway, the comments above are probably bikesheds. I'll keep
reviewing, so when I reach patch 8 I'll have a clearer view of these
variables, then I can come back to this patch.
Heh it's funny that you pointed that out. I was going to go back and change that, since it bugs me that it's inconsistent. I'll fix it for the next revision of this patch.
  };

  struct intel_digital_port {
--
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx



_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux