subject:"\[Intel\-gfx\] \[PATCH 08\/13\] drm\/i915\: Support EDID compliance tests with the intel_dp_autotest

Re: [Intel-gfx] [PATCH 08/13] drm/i915: Support EDID compliance tests with the intel_dp_autotest_edid() function

2015-04-14 Thread Paulo Zanoni

2015-04-10 14:42 GMT-03:00 Todd Previte tprev...@gmail.com:
 Updates the EDID compliance test function to perform the EDID read as
 required by the tests. This read needs to take place in the kernel for
 reasons of speed and efficiency. The results of the EDID read operations
 are handed off to userspace so that the userspace app can set the display
 mode appropriately for the test response.

 The compliance_test_active flag now appears at the end of the individual
 test handling functions. This is so that the kernel-side operations can
 be completed without the risk of interruption from the userspace app
 that is polling on that flag.

 V2:
 - Addressed mailing list feedback
 - Removed excess debug messages
 - Removed extraneous comments
 - Fixed formatting issues (line length  80)
 - Updated the debug message in compute_edid_checksum to output hex values
   instead of decimal
 V3:
 - Addressed more list feedback
 - Added the test_active flag to the autotest function
 - Removed test_active flag from handler
 - Added failsafe check on the compliance test active flag
   at the end of the test handler
 - Fixed checkpatch.pl issues
 V4:
 - Removed the checksum computation function and its use as it has been
   rendered superfluous by changes to the core DRM EDID functions
 - Updated to use the raw header corruption detection mechanism
 - Moved the declaration of the test_data variable here
 V5:
 - Update test active flag variable name to match the change in the
   first patch of the series.
 - Relocated the test active flag declaration and initialization
   to this patch
 V6:
 - Updated to use the new flag for raw EDID header corruption
 - Removed the extra EDID read from the autotest function
 - Added the edid_checksum variable to struct intel_dp so that the
   autotest function can write it to the sink device
 - Moved the update to the hpd_pulse function to another patch
 - Removed extraneous constants
 V7:
 - Fixed erroneous placement of the checksum assignment. In some cases
   such as when the EDID read fails and is NULL, this causes a NULL ptr
   dereference in the kernel. Bad news. Fixed now.

 Signed-off-by: Todd Previte tprev...@gmail.com
 ---
  drivers/gpu/drm/i915/intel_dp.c  | 46 
 
  drivers/gpu/drm/i915/intel_drv.h |  4 
  2 files changed, 50 insertions(+)

 diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
 index ba2da44..3bfdc40 100644
 --- a/drivers/gpu/drm/i915/intel_dp.c
 +++ b/drivers/gpu/drm/i915/intel_dp.c
 @@ -41,6 +41,12 @@

  #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)

 +/* Compliance test status bits  */
 +#define INTEL_DP_RESOLUTION_SHIFT_MASK 4
 +#define INTEL_DP_RESOLUTION_PREFERRED  (1  INTEL_DP_RESOLUTION_SHIFT_MASK)
 +#define INTEL_DP_RESOLUTION_STANDARD   (2  INTEL_DP_RESOLUTION_SHIFT_MASK)
 +#define INTEL_DP_RESOLUTION_FAILSAFE   (3  INTEL_DP_RESOLUTION_SHIFT_MASK)
 +
  struct dp_link_dpll {
 int link_bw;
 struct dpll dpll;
 @@ -3770,6 +3776,35 @@ static uint8_t intel_dp_autotest_video_pattern(struct 
 intel_dp *intel_dp)
  static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
  {
 uint8_t test_result = DP_TEST_NAK;
 +   uint32_t ret = 0;
 +
 +   if (intel_dp-compliance_edid_invalid ||
 +   intel_dp-aux.i2c_defer_count  6) {
 +   /* Check for NACKs/DEFERs, use failsafe if detected
 +*  (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5)
 +*/
 +   if (intel_dp-aux.i2c_nack_count  0 ||
 +   intel_dp-aux.i2c_defer_count  0)
 +   DRM_DEBUG_KMS(EDID read had %d NACKs, %d DEFERs\n,
 + intel_dp-aux.i2c_nack_count,
 + intel_dp-aux.i2c_defer_count);
 +   intel_dp-compliance_test_data = INTEL_DP_RESOLUTION_FAILSAFE;

Since this case is expected to happen, shouldn't we return something
that's not DP_TEST_NAK here?

Also, we should try to write the checksum on this case too, shouldn't we?

I see on 4.2.2.6 that the only happy case involves setting the
failsafe resolution, and optionally sending the checksum.

(sorry for not catching this earlier... I just didn't spot the problem)


 +   } else {
 +   ret = drm_dp_dpcd_write(intel_dp-aux,
 +   DP_TEST_EDID_CHECKSUM,
 +   intel_dp-compliance_edid_checksum, 
 1);
 +   if (ret = 0)
 +   DRM_DEBUG_DRIVER(Failed to write EDID checksum\n);
 +   else
 +   DRM_DEBUG_DRIVER(EDID checksum written to sink\n);
 +
 +   test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
 +   intel_dp-compliance_test_data = INTEL_DP_RESOLUTION_STANDARD;
 +   }
 +
 +   /* Set test active flag here so userspace doesn't interrupt things */
 +   intel_dp-compliance_test_active = 1;
 +

Re: [Intel-gfx] [PATCH 08/13] drm/i915: Support EDID compliance tests with the intel_dp_autotest_edid() function

2015-04-14 Thread Todd Previte




On 4/14/15 6:35 AM, Paulo Zanoni wrote:

2015-04-10 14:42 GMT-03:00 Todd Previte tprev...@gmail.com:

Updates the EDID compliance test function to perform the EDID read as
required by the tests. This read needs to take place in the kernel for
reasons of speed and efficiency. The results of the EDID read operations
are handed off to userspace so that the userspace app can set the display
mode appropriately for the test response.

The compliance_test_active flag now appears at the end of the individual
test handling functions. This is so that the kernel-side operations can
be completed without the risk of interruption from the userspace app
that is polling on that flag.

V2:
- Addressed mailing list feedback
- Removed excess debug messages
- Removed extraneous comments
- Fixed formatting issues (line length  80)
- Updated the debug message in compute_edid_checksum to output hex values
   instead of decimal
V3:
- Addressed more list feedback
- Added the test_active flag to the autotest function
- Removed test_active flag from handler
- Added failsafe check on the compliance test active flag
   at the end of the test handler
- Fixed checkpatch.pl issues
V4:
- Removed the checksum computation function and its use as it has been
   rendered superfluous by changes to the core DRM EDID functions
- Updated to use the raw header corruption detection mechanism
- Moved the declaration of the test_data variable here
V5:
- Update test active flag variable name to match the change in the
   first patch of the series.
- Relocated the test active flag declaration and initialization
   to this patch
V6:
- Updated to use the new flag for raw EDID header corruption
- Removed the extra EDID read from the autotest function
- Added the edid_checksum variable to struct intel_dp so that the
   autotest function can write it to the sink device
- Moved the update to the hpd_pulse function to another patch
- Removed extraneous constants
V7:
- Fixed erroneous placement of the checksum assignment. In some cases
   such as when the EDID read fails and is NULL, this causes a NULL ptr
   dereference in the kernel. Bad news. Fixed now.

Signed-off-by: Todd Previte tprev...@gmail.com
---
  drivers/gpu/drm/i915/intel_dp.c  | 46 
  drivers/gpu/drm/i915/intel_drv.h |  4 
  2 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ba2da44..3bfdc40 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -41,6 +41,12 @@

  #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)

+/* Compliance test status bits  */
+#define INTEL_DP_RESOLUTION_SHIFT_MASK 4
+#define INTEL_DP_RESOLUTION_PREFERRED  (1  INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_STANDARD   (2  INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_FAILSAFE   (3  INTEL_DP_RESOLUTION_SHIFT_MASK)
+
  struct dp_link_dpll {
 int link_bw;
 struct dpll dpll;
@@ -3770,6 +3776,35 @@ static uint8_t intel_dp_autotest_video_pattern(struct 
intel_dp *intel_dp)
  static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
  {
 uint8_t test_result = DP_TEST_NAK;
+   uint32_t ret = 0;
+
+   if (intel_dp-compliance_edid_invalid ||
+   intel_dp-aux.i2c_defer_count  6) {
+   /* Check for NACKs/DEFERs, use failsafe if detected
+*  (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5)
+*/
+   if (intel_dp-aux.i2c_nack_count  0 ||
+   intel_dp-aux.i2c_defer_count  0)
+   DRM_DEBUG_KMS(EDID read had %d NACKs, %d DEFERs\n,
+ intel_dp-aux.i2c_nack_count,
+ intel_dp-aux.i2c_defer_count);
+   intel_dp-compliance_test_data = INTEL_DP_RESOLUTION_FAILSAFE;

Since this case is expected to happen, shouldn't we return something
that's not DP_TEST_NAK here?
The spec does not state whether or not we should ACK or NAK this case. 
This value is written to the sink device as the response, so for 
compliance testing purposes, the test device does not appear to care 
whether it's ACK or NAK.


Also, we should try to write the checksum on this case too, shouldn't we?

I see on 4.2.2.6 that the only happy case involves setting the
failsafe resolution, and optionally sending the checksum.

(sorry for not catching this earlier... I just didn't spot the problem)
Correct. Writing the bad checksum is optional, so I opted not to. Less 
code, less AUX transactions.



+   } else {
+   ret = drm_dp_dpcd_write(intel_dp-aux,
+   DP_TEST_EDID_CHECKSUM,
+   intel_dp-compliance_edid_checksum, 1);
+   if (ret = 0)
+   DRM_DEBUG_DRIVER(Failed to write EDID checksum\n);
+   else
+   DRM_DEBUG_DRIVER(EDID checksum written to sink\n);

Re: [Intel-gfx] [PATCH 08/13] drm/i915: Support EDID compliance tests with the intel_dp_autotest_edid() function

Re: [Intel-gfx] [PATCH 08/13] drm/i915: Support EDID compliance tests with the intel_dp_autotest_edid() function

2 matches

Site Navigation

Mail list logo

Footer information