Displayport compliance testing V5

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

V5:
- Moved test_active variable declaration and initialization out of this patch and into the patch where it's used
- Changed variable name compliance_testing_active to compliance_test_active to unify the naming convention
- Added initialization for compliance_test_type variable

Signed-off-by Todd Previte <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c  | 75 +++++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h |  3 ++
 2 files changed, 74 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +71 -4

index eea9e36..ae0fb98 100644
@@ -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_test_type = 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

drivers/gpu/drm/i915/intel_drv.h +3 -1

index eef79cc..ab811e5 100644
@@ -647,6 +647,9 @@ struct intel_dp {
                                     bool has_aux_irq,
                                     int send_bytes,
                                     uint32_t aux_clock_divider);
+
+       /* Displayport compliance testing */
+       unsigned long compliance_test_type;
 };
 
 struct intel_digital_port {
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Move the DPCD read to the top and check for an interrupt from the sink to catch Displayport automated testing requests necessary to support Displayport compliance testing. The checks for active connectors and link status are moved below the check for the interrupt.

The main reason for doing this is to make sure that a test request isn't missed. Checking for the status of the encoder/crtc isn't necessary for some test cases (AUX channel tests are one example) and without moving the check for the interrupt, these tests may not execute if one of those checks fails. Additionally, if reading the DPCD fails, regardless of whether or not testing is happening, there's no way to train the link since configurations and status can't be read, nor can link training parameters be written.

V1:
- This is the second part of the single-patch split previously mentioned.

Signed-off-by Todd Previte <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 15 ++++++++++++++-
  1 file changed, 14 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_dp.c +7 -1

index 30cd433..23184b0 100644
@@ -3913,13 +3913,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
  		drm_dp_dpcd_writeb(&intel_dp->aux,
  				   DP_DEVICE_SERVICE_IRQ_VECTOR,
  				   sink_irq_vector);
-
  		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
  			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");
  	}
 
+	if (!intel_encoder->connectors_active)
+		return;
+
+	if (WARN_ON(!intel_encoder->base.crtc))
+		return;
+
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)

Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for all HPD plug events. To reduce the amount of code, this EDID read is also used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual support for these tests is implemented in later patches in this series.

Signed-off-by Todd Previte <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/gpu/drm/i915/intel_dp.c +11 -1

index 23184b0..a5dfaff 100644
@@ -3890,6 +3890,9 @@ intel_dp_check_link_status(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;
+       struct drm_connector *connector = &intel_dp->attached_connector->base;
+       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+       struct edid *edid_read = NULL;
        u8 sink_irq_vector;
        u8 link_status[DP_LINK_STATUS_SIZE];
 

@@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
                return;
        }
 
+       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
+        * 4.2.2.1 - EDID read required for all HPD events
+         */
+        edid_read = drm_get_edid(connector, adapter);
+        if (!edid_read || connector->edid_header_corrupt == 1) {
+                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
+        }
+
        /* Try to read the source of the interrupt */
        if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
            intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-04-10 13:12 GMT-03:00 Todd Previte <[email protected]>:

Quoted 7 lines

Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for all HPD plug events. To reduce the amount of code, this EDID read is also used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual support for these tests is implemented in later patches in this series.

Signed-off-by Todd Previte <[email protected]>

drivers/gpu/drm/i915/intel_dp.c: In function ‘intel_dp_check_link_status’:
drivers/gpu/drm/i915/intel_dp.c:4140:36: error: ‘struct drm_connector’ has no member named ‘edid_header_corrupt’

          if (!edid_read || connector->edid_header_corrupt == 1) {

scripts/Makefile.build:258: recipe for target 'drivers/gpu/drm/i915/intel_dp.o' failed make[4]: *** [drivers/gpu/drm/i915/intel_dp.o] Error 1

Quoted 36 lines

---
  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
  1 file changed, 11 insertions(+)

drivers/gpu/drm/i915/intel_dp.c +11 -1

index 23184b0..a5dfaff 100644
@@ -3890,6 +3890,9 @@ intel_dp_check_link_status(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;
+       struct drm_connector *connector = &intel_dp->attached_connector->base;
+       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+       struct edid *edid_read = NULL;
         u8 sink_irq_vector;
         u8 link_status[DP_LINK_STATUS_SIZE];
 

@@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
                 return;
         }
 
+       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
+        * 4.2.2.1 - EDID read required for all HPD events
+         */
+        edid_read = drm_get_edid(connector, adapter);
+        if (!edid_read || connector->edid_header_corrupt == 1) {
+                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
+        }
+
         /* Try to read the source of the interrupt */
         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
             intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
--

1.9.1

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for all HPD plug events. To reduce the amount of code, this EDID read is also used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual support for these tests is implemented in later patches in this series.

V2:
- Fixed compilation error introduced during rework

Signed-off-by Todd Previte <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/gpu/drm/i915/intel_dp.c +3 -0

index 23184b0..75df3e2 100644
@@ -3890,6 +3890,9 @@ intel_dp_check_link_status(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;
+       struct drm_connector *connector = &intel_dp->attached_connector->base;
+       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+       struct edid *edid_read = NULL;
        u8 sink_irq_vector;
        u8 link_status[DP_LINK_STATUS_SIZE];
 

@@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
                return;

drivers/gpu/drm/i915/intel_dp.c +0 -1

        /* Try to read the source of the interrupt */
        if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
            intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

... I will leave to the > maintainers the decision of whether the current proposed implementation is acceptable or [not] ...

2015-04-13 11:53 GMT-03:00 Todd Previte <[email protected]>:

Quoted 30 lines

Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for all HPD plug events. To reduce the amount of code, this EDID read is also used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual support for these tests is implemented in later patches in this series.

V2:
- Fixed compilation error introduced during rework

Signed-off-by Todd Previte <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
  1 file changed, 11 insertions(+)

drivers/gpu/drm/i915/intel_dp.c +3 -0

index 23184b0..75df3e2 100644
@@ -3890,6 +3890,9 @@ intel_dp_check_link_status(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;
+       struct drm_connector *connector = &intel_dp->attached_connector->base;
+       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+       struct edid *edid_read = NULL;
         u8 sink_irq_vector;
         u8 link_status[DP_LINK_STATUS_SIZE];
 

@@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
                 return;

drivers/gpu/drm/i915/intel_dp.c intel_dp_check_link_status(struct intel_dp *intel_dp) · line 3919

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp.c +8 -0

         }
 
+       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
+        * 4.2.2.1 - EDID read required for all HPD events
+         */
+        edid_read = drm_get_edid(connector, adapter);
+        if (!edid_read) {
+                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
+        }
+

We already briefly discussed this patch in private, so I'm going to summarize the discussion and also add some more points here.

Frist, the actual detailed review: the indentation here is using spaces and we're leaking the EDID. This will cause rebases to a few of the next patches.

Back to the hight level architecture: your initial versions of the series contained just 1 extra EDID read, and it was contained inside the compliance testing function. Then the versions submitted a few days ago had 2 extra EDID reads, then after some discussion you reduced to 1 extra EDID read (the one on this patch). I previously asked "But what about the automatic EDID read we do when we get a hotplug? Can't we just rely on it?". I got some answers to the question, but I was not really convinced.

Yesterday I was arguing that this extra EDID read is going to add a small delay to every hotplug event we get, so my initial suggestion was to organize the compliance testing in a way that would require the user space program to call the GetResources() IOCTL to force the EDID when needed. Your argument was that then the DP compliance testing procedure would be testing our app for compliance, not the Kernel.

But today I decided to finally do some debugging regarding this, and I was able to confirm that we do follow the DP requirements: we do have an automatic EDID read done by the Kernel whenever we do a hotplug:
i915_hotplug_work_func() calls intel_dp_detect(), which ends calling drm_get_edid() at some point. This function also does other stuff that is required by the compliance testing, such as the DPCD reads.

Now there's a problem with using i915_hotplug_work_func(), which could the reason why you rejected it: it only happens after intel_dp_hpd_pulse(), which means that we only really do the EDID read after intel_dp_handle_test_request().

I consider i915_hotplug_work_func() a fundamental part of our DP framework, and the DP compliance testing seems to be just ignoring its existence. So my idea for a solution here would be to make intel_dp_handle_test_request() run on its own delayed work function. It would wait for both i915_digport_work_func() and i915_hotplug_work_func() to finish, and only then it would do the normal processing. With this, we would be able to avoid the edid read on this patch, we would maybe be able to avoid at least part of patch 2, we would maybe be able to completely avoid patch 7, and then on patch 8 we would start touching intel_dp_get_edid() instead.

I know this is sort of a fundamental change that is being requested a little late in the review process, and it can be frustrating, but this aspect of the code only recently changed (I was fine with the EDID reads just in the compliance testing function), and since the DP compliance code is quite complex, it took me a while to realize everything that's going on and what is the purpose of each piece. I also think that, since this idea will allow the compliance testing to take into consideration the work done by i915_hotplug_work_func(), compliance testing will better reflect the behavior that is actually done by the Kernel when DP devices are plugged/unplugged. And I did ask about those new EDID reads as soon as I started reviewing the patch that introduced them.

Now, since I know how frustrating it is to have to change a significant portion of the code once again, I will leave to the maintainers the decision of whether the current proposed implementation is acceptable or if we want to make the DP compliance testing code take into consideration the work done by i915_hotplug_work_func(). I would also like to know your opinion on this. Maybe my idea just doesn't make sense because of something else I didn't realize :)

Quoted 6 lines

drivers/gpu/drm/i915/intel_dp.c +0 -1

         /* Try to read the source of the interrupt */
         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
             intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
--

1.9.1

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 109 lines

2015-04-13 11:53 GMT-03:00 Todd Previte <[email protected]>:

Quoted 37 lines

Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the
Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for all HPD plug events. To reduce the amount of code, this EDID read is also used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual support for these tests is implemented in later patches in this series.

V2:
- Fixed compilation error introduced during rework

Signed-off-by Todd Previte <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
  1 file changed, 11 insertions(+)

drivers/gpu/drm/i915/intel_dp.c +3 -0

index 23184b0..75df3e2 100644
@@ -3890,6 +3890,9 @@ intel_dp_check_link_status(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;
+       struct drm_connector *connector =
 &intel_dp->attached_connector->base;
+       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+       struct edid *edid_read = NULL;

         u8 sink_irq_vector;
         u8 link_status[DP_LINK_STATUS_SIZE];

drivers/gpu/drm/i915/intel_dp.c +0 -0

@@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp
 *intel_dp)
                 return;

drivers/gpu/drm/i915/intel_dp.c intel_dp_check_link_status(struct intel_dp · line 3920

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp.c +8 -0

         }
 
+       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
+        * 4.2.2.1 - EDID read required for all HPD events
+         */
+        edid_read = drm_get_edid(connector, adapter);
+        if (!edid_read) {
+                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
+        }
+

We already briefly discussed this patch in private, so I'm going to summarize the discussion and also add some more points here.

Frist, the actual detailed review: the indentation here is using spaces and we're leaking the EDID. This will cause rebases to a few of the next patches.

Back to the hight level architecture: your initial versions of the series contained just 1 extra EDID read, and it was contained inside the compliance testing function. Then the versions submitted a few days ago had 2 extra EDID reads, then after some discussion you reduced to 1 extra EDID read (the one on this patch). I previously asked "But what about the automatic EDID read we do when we get a hotplug? Can't we just rely on it?". I got some answers to the question, but I was not really convinced.

Yesterday I was arguing that this extra EDID read is going to add a small delay to every hotplug event we get, so my initial suggestion was to organize the compliance testing in a way that would require the user space program to call the GetResources() IOCTL to force the EDID when needed. Your argument was that then the DP compliance testing procedure would be testing our app for compliance, not the Kernel.

But today I decided to finally do some debugging regarding this, and I was able to confirm that we do follow the DP requirements: we do have an automatic EDID read done by the Kernel whenever we do a hotplug:
i915_hotplug_work_func() calls intel_dp_detect(), which ends calling drm_get_edid() at some point. This function also does other stuff that is required by the compliance testing, such as the DPCD reads.

Now there's a problem with using i915_hotplug_work_func(), which could the reason why you rejected it: it only happens after intel_dp_hpd_pulse(), which means that we only really do the EDID read after intel_dp_handle_test_request().

I consider i915_hotplug_work_func() a fundamental part of our DP framework, and the DP compliance testing seems to be just ignoring its existence. So my idea for a solution here would be to make intel_dp_handle_test_request() run on its own delayed work function. It would wait for both i915_digport_work_func() and i915_hotplug_work_func() to finish, and only then it would do the normal processing. With this, we would be able to avoid the edid read on this patch, we would maybe be able to avoid at least part of patch 2, we would maybe be able to completely avoid patch 7, and then on patch 8 we would start touching intel_dp_get_edid() instead.

I know this is sort of a fundamental change that is being requested a little late in the review process, and it can be frustrating, but this aspect of the code only recently changed (I was fine with the EDID reads just in the compliance testing function), and since the DP compliance code is quite complex, it took me a while to realize everything that's going on and what is the purpose of each piece. I also think that, since this idea will allow the compliance testing to take into consideration the work done by i915_hotplug_work_func(), compliance testing will better reflect the behavior that is actually done by the Kernel when DP devices are plugged/unplugged. And I did ask about those new EDID reads as soon as I started reviewing the patch that introduced them.

Quoted 8 lines

Now, since I know how frustrating it is to have to change a significant portion of the code once again, I will leave to the maintainers the decision of whether the current proposed implementation is acceptable or if we want to make the DP compliance testing code take into consideration the work done by i915_hotplug_work_func(). I would also like to know your opinion on this. Maybe my idea just doesn't make sense because of something else I didn't realize :)

I don't think this is a good idea. The work loop aspect seems like a very complex solution solution to a problem that is relatively simple. In a discussion with Daniel, he indicated that adding a work loop is something to be avoided unless it's *really* necessary, as they are prone to race conditions. In this case, I just don't see that it's necessary.

Additionally, you have been keen to note invasive solutions and this seems like a highly invasive solution especially in light of having a viable one right here.

This solution adds very little overhead along the HPD path and that overhead is a single read of the EDID for each event. To further address any concerns about performance, for V6 I've propagated the long_hpd flag forward into check_link_status so that the EDID read is only called for a hot plug event, not short pulses. Another plus to this solution is that if/when this needs to moved to userspace, this one is a lot easier to unwind and pull out than the work loop you're suggesting.

With respect to the user space solution, that's something worth investigating in the future. I have some concerns about doing that, the primary one being the ability to detect the corrupted header, but I think it's something to consider for a follow-on update.

Quoted 13 lines

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp.c +0 -0

         /* Try to read the source of the interrupt */
         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&

             intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {

--
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-04-15 12:37 GMT-03:00 Todd Previte <[email protected]>:

Quoted 116 lines · Paulo Zanoni

Quoted 106 lines

2015-04-13 11:53 GMT-03:00 Todd Previte <[email protected]>:

Quoted 36 lines

Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the
Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for all HPD plug events. To reduce the amount of code, this EDID read is also used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual support for these tests is implemented in later patches in this series.

V2:
- Fixed compilation error introduced during rework

Signed-off-by Todd Previte <[email protected]>

---
   drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
   1 file changed, 11 insertions(+)

drivers/gpu/drm/i915/intel_dp.c +3 -0

index 23184b0..75df3e2 100644
@@ -3890,6 +3890,9 @@ intel_dp_check_link_status(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;
+       struct drm_connector *connector =
 &intel_dp->attached_connector->base;
+       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+       struct edid *edid_read = NULL;

          u8 sink_irq_vector;
          u8 link_status[DP_LINK_STATUS_SIZE];

drivers/gpu/drm/i915/intel_dp.c +0 -0

@@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp
 *intel_dp)
                  return;

drivers/gpu/drm/i915/intel_dp.c intel_dp_check_link_status(struct intel_dp · line 3920

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp.c +8 -0

          }
 
+       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
+        * 4.2.2.1 - EDID read required for all HPD events
+         */
+        edid_read = drm_get_edid(connector, adapter);
+        if (!edid_read) {
+                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
+        }
+

We already briefly discussed this patch in private, so I'm going to summarize the discussion and also add some more points here.

Frist, the actual detailed review: the indentation here is using spaces and we're leaking the EDID. This will cause rebases to a few of the next patches.

Back to the hight level architecture: your initial versions of the series contained just 1 extra EDID read, and it was contained inside the compliance testing function. Then the versions submitted a few days ago had 2 extra EDID reads, then after some discussion you reduced to 1 extra EDID read (the one on this patch). I previously asked "But what about the automatic EDID read we do when we get a hotplug? Can't we just rely on it?". I got some answers to the question, but I was not really convinced.

Yesterday I was arguing that this extra EDID read is going to add a small delay to every hotplug event we get, so my initial suggestion was to organize the compliance testing in a way that would require the user space program to call the GetResources() IOCTL to force the EDID when needed. Your argument was that then the DP compliance testing procedure would be testing our app for compliance, not the Kernel.

But today I decided to finally do some debugging regarding this, and I was able to confirm that we do follow the DP requirements: we do have an automatic EDID read done by the Kernel whenever we do a hotplug:
i915_hotplug_work_func() calls intel_dp_detect(), which ends calling drm_get_edid() at some point. This function also does other stuff that is required by the compliance testing, such as the DPCD reads.

Now there's a problem with using i915_hotplug_work_func(), which could the reason why you rejected it: it only happens after intel_dp_hpd_pulse(), which means that we only really do the EDID read after intel_dp_handle_test_request().

I consider i915_hotplug_work_func() a fundamental part of our DP framework, and the DP compliance testing seems to be just ignoring its existence. So my idea for a solution here would be to make intel_dp_handle_test_request() run on its own delayed work function. It would wait for both i915_digport_work_func() and i915_hotplug_work_func() to finish, and only then it would do the normal processing. With this, we would be able to avoid the edid read on this patch, we would maybe be able to avoid at least part of patch 2, we would maybe be able to completely avoid patch 7, and then on patch 8 we would start touching intel_dp_get_edid() instead.

I know this is sort of a fundamental change that is being requested a little late in the review process, and it can be frustrating, but this aspect of the code only recently changed (I was fine with the EDID reads just in the compliance testing function), and since the DP compliance code is quite complex, it took me a while to realize everything that's going on and what is the purpose of each piece. I also think that, since this idea will allow the compliance testing to take into consideration the work done by i915_hotplug_work_func(), compliance testing will better reflect the behavior that is actually done by the Kernel when DP devices are plugged/unplugged. And I did ask about those new EDID reads as soon as I started reviewing the patch that introduced them.

Quoted 8 lines

Now, since I know how frustrating it is to have to change a significant portion of the code once again, I will leave to the maintainers the decision of whether the current proposed implementation is acceptable or if we want to make the DP compliance testing code take into consideration the work done by i915_hotplug_work_func(). I would also like to know your opinion on this. Maybe my idea just doesn't make sense because of something else I didn't realize :)

Quoted 5 lines

I don't think this is a good idea. The work loop aspect seems like a very complex solution solution to a problem that is relatively simple. In a discussion with Daniel, he indicated that adding a work loop is something to be avoided unless it's *really* necessary, as they are prone to race conditions. In this case, I just don't see that it's necessary.

The workqueue thing was just an idea to implement a solution for the real problem. I think we should be focusing about discussing the fact that we're not taking i915_hotplug_work_func() into consideration when doing the compliance testing, not on the fact that one of the possible implementations could use a workqueue. I'd still like to hear your arguments on that.

Quoted 25 lines

Additionally, you have been keen to note invasive solutions and this seems like a highly invasive solution especially in light of having a viable one right here.

This solution adds very little overhead along the HPD path and that overhead is a single read of the EDID for each event. To further address any concerns about performance, for V6 I've propagated the long_hpd flag forward into check_link_status so that the EDID read is only called for a hot plug event, not short pulses. Another plus to this solution is that if/when this needs to moved to userspace, this one is a lot easier to unwind and pull out than the work loop you're suggesting.

With respect to the user space solution, that's something worth investigating in the future. I have some concerns about doing that, the primary one being the ability to detect the corrupted header, but I think it's something to consider for a follow-on update.

Quoted 6 lines

Quoted 6 lines

drivers/gpu/drm/i915/intel_dp.c +0 -0

          /* Try to read the source of the interrupt */
          if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&

              intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {

--
1.9.1

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 121 lines · Paulo Zanoni

2015-04-15 12:37 GMT-03:00 Todd Previte <[email protected]>:

Quoted 115 lines · Paulo Zanoni

Quoted 106 lines

2015-04-13 11:53 GMT-03:00 Todd Previte <[email protected]>:

Quoted 36 lines

Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the
Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for all HPD plug events. To reduce the amount of code, this EDID read is also used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual support for these tests is implemented in later patches in this series.

V2:
- Fixed compilation error introduced during rework

Signed-off-by Todd Previte <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
  1 file changed, 11 insertions(+)

drivers/gpu/drm/i915/intel_dp.c +3 -0

index 23184b0..75df3e2 100644
@@ -3890,6 +3890,9 @@ intel_dp_check_link_status(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;
+       struct drm_connector *connector =
 &intel_dp->attached_connector->base;
+       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+       struct edid *edid_read = NULL;

         u8 sink_irq_vector;
         u8 link_status[DP_LINK_STATUS_SIZE];

drivers/gpu/drm/i915/intel_dp.c +0 -0

@@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp
 *intel_dp)
                 return;

drivers/gpu/drm/i915/intel_dp.c intel_dp_check_link_status(struct intel_dp · line 3920

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp.c +8 -0

         }
 
+       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
+        * 4.2.2.1 - EDID read required for all HPD events
+         */
+        edid_read = drm_get_edid(connector, adapter);
+        if (!edid_read) {
+                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
+        }
+

We already briefly discussed this patch in private, so I'm going to summarize the discussion and also add some more points here.

Frist, the actual detailed review: the indentation here is using spaces and we're leaking the EDID. This will cause rebases to a few of the next patches.

Back to the hight level architecture: your initial versions of the series contained just 1 extra EDID read, and it was contained inside the compliance testing function. Then the versions submitted a few days ago had 2 extra EDID reads, then after some discussion you reduced to 1 extra EDID read (the one on this patch). I previously asked "But what about the automatic EDID read we do when we get a hotplug? Can't we just rely on it?". I got some answers to the question, but I was not really convinced.

Yesterday I was arguing that this extra EDID read is going to add a small delay to every hotplug event we get, so my initial suggestion was to organize the compliance testing in a way that would require the user space program to call the GetResources() IOCTL to force the EDID when needed. Your argument was that then the DP compliance testing procedure would be testing our app for compliance, not the Kernel.

But today I decided to finally do some debugging regarding this, and I was able to confirm that we do follow the DP requirements: we do have an automatic EDID read done by the Kernel whenever we do a hotplug:
i915_hotplug_work_func() calls intel_dp_detect(), which ends calling drm_get_edid() at some point. This function also does other stuff that is required by the compliance testing, such as the DPCD reads.

Now there's a problem with using i915_hotplug_work_func(), which could the reason why you rejected it: it only happens after intel_dp_hpd_pulse(), which means that we only really do the EDID read after intel_dp_handle_test_request().

I consider i915_hotplug_work_func() a fundamental part of our DP framework, and the DP compliance testing seems to be just ignoring its existence. So my idea for a solution here would be to make intel_dp_handle_test_request() run on its own delayed work function. It would wait for both i915_digport_work_func() and i915_hotplug_work_func() to finish, and only then it would do the normal processing. With this, we would be able to avoid the edid read on this patch, we would maybe be able to avoid at least part of patch 2, we would maybe be able to completely avoid patch 7, and then on patch 8 we would start touching intel_dp_get_edid() instead.

I know this is sort of a fundamental change that is being requested a little late in the review process, and it can be frustrating, but this aspect of the code only recently changed (I was fine with the EDID reads just in the compliance testing function), and since the DP compliance code is quite complex, it took me a while to realize everything that's going on and what is the purpose of each piece. I also think that, since this idea will allow the compliance testing to take into consideration the work done by i915_hotplug_work_func(), compliance testing will better reflect the behavior that is actually done by the Kernel when DP devices are plugged/unplugged. And I did ask about those new EDID reads as soon as I started reviewing the patch that introduced them.

Quoted 8 lines

Now, since I know how frustrating it is to have to change a significant portion of the code once again, I will leave to the maintainers the decision of whether the current proposed implementation is acceptable or if we want to make the DP compliance testing code take into consideration the work done by i915_hotplug_work_func(). I would also like to know your opinion on this. Maybe my idea just doesn't make sense because of something else I didn't realize :)

Quoted 5 lines

I don't think this is a good idea. The work loop aspect seems like a very complex solution solution to a problem that is relatively simple. In a discussion with Daniel, he indicated that adding a work loop is something to be avoided unless it's *really* necessary, as they are prone to race conditions. In this case, I just don't see that it's necessary.

Quoted 6 lines

The workqueue thing was just an idea to implement a solution for the real problem. I think we should be focusing about discussing the fact that we're not taking i915_hotplug_work_func() into consideration when doing the compliance testing, not on the fact that one of the possible implementations could use a workqueue. I'd still like to hear your arguments on that.

Fair enough.

So I've been looking into this and why the i915_hotplug_work_func wasn't part of this. It is, as you said, a relatively fundamental code path for Displayport through the driver. What I discovered was that this function is never called on HSW (my primary test vehicle), mainly because check_link_status() returns IRQ_HANDLED instead of IRQ_NONE. The work function for HSW is i915_digport_work_func, so when it gets the IRQ_HANDLED return code, it doesn't fall through to the legacy i915_hotplug_work_func handler. This is important because this handler calls intel_hpd_irq_event which is where the ->detect connector function is called. And intel_dp_detect() is where all the happy goodness for Displayport begins.

Up until I discovered this, I had mistakenly propagated that problem forward in to the SST case in intel_dp_hot_pulse() in patch 6 by returning IRQ_HANDLED instead of IRQ_NONE, which is what the code was doing for SST prior to patch 6. With this problem corrected (as it is in the latest update in patch set V6) the work functions are now called as they should be. The point being that this opens up the possibility of using elements along this path to pass compliance testing, thereby creating a more valid test case.

With this in mind, I am not opposed to using elements along that path to satisfy compliance requirements (that's the spirit of the tests, anyways) but as I've indicated, there are cases where we need to take special steps (like the edid_corrupt flag) in order to do the right things to pass the tests. I have concerns about trying to do that at this point, as it requires substantial rework to that code path that have a significant chance of breaking things. So to avoid that, I propose that this patch be merged now so that a working solution is in place. This discussion should continue and we can decide where to put things in the hotplug_work path to satisfy the compliance requires over the course of some followup patches.

Quoted 149 lines · Todd Previte

Quoted 121 lines · Paulo Zanoni

2015-04-15 12:37 GMT-03:00 Todd Previte <[email protected]>:

Quoted 115 lines · Paulo Zanoni

Quoted 106 lines

2015-04-13 11:53 GMT-03:00 Todd Previte <[email protected]>:

Quoted 36 lines

Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the
Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for all HPD plug events. To reduce the amount of code, this EDID read is also used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual support for these tests is implemented in later patches in this series.

V2:
- Fixed compilation error introduced during rework

Signed-off-by Todd Previte <[email protected]>

---
   drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
   1 file changed, 11 insertions(+)

drivers/gpu/drm/i915/intel_dp.c +3 -0

index 23184b0..75df3e2 100644
@@ -3890,6 +3890,9 @@ intel_dp_check_link_status(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;
+       struct drm_connector *connector =
 &intel_dp->attached_connector->base;
+       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+       struct edid *edid_read = NULL;

          u8 sink_irq_vector;
          u8 link_status[DP_LINK_STATUS_SIZE];

drivers/gpu/drm/i915/intel_dp.c +0 -0

@@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp
 *intel_dp)
                  return;

drivers/gpu/drm/i915/intel_dp.c intel_dp_check_link_status(struct intel_dp · line 3920

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp.c +8 -0

          }
 
+       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
+        * 4.2.2.1 - EDID read required for all HPD events
+         */
+        edid_read = drm_get_edid(connector, adapter);
+        if (!edid_read) {
+                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
+        }
+

We already briefly discussed this patch in private, so I'm going to summarize the discussion and also add some more points here.

Frist, the actual detailed review: the indentation here is using spaces and we're leaking the EDID. This will cause rebases to a few of the next patches.

Back to the hight level architecture: your initial versions of the series contained just 1 extra EDID read, and it was contained inside the compliance testing function. Then the versions submitted a few days ago had 2 extra EDID reads, then after some discussion you reduced to 1 extra EDID read (the one on this patch). I previously asked "But what about the automatic EDID read we do when we get a hotplug? Can't we just rely on it?". I got some answers to the question, but I was not really convinced.

Yesterday I was arguing that this extra EDID read is going to add a small delay to every hotplug event we get, so my initial suggestion was to organize the compliance testing in a way that would require the user space program to call the GetResources() IOCTL to force the EDID when needed. Your argument was that then the DP compliance testing procedure would be testing our app for compliance, not the Kernel.

But today I decided to finally do some debugging regarding this, and I was able to confirm that we do follow the DP requirements: we do have an automatic EDID read done by the Kernel whenever we do a hotplug:
i915_hotplug_work_func() calls intel_dp_detect(), which ends calling drm_get_edid() at some point. This function also does other stuff that is required by the compliance testing, such as the DPCD reads.

Now there's a problem with using i915_hotplug_work_func(), which could the reason why you rejected it: it only happens after intel_dp_hpd_pulse(), which means that we only really do the EDID read after intel_dp_handle_test_request().

I consider i915_hotplug_work_func() a fundamental part of our DP framework, and the DP compliance testing seems to be just ignoring its existence. So my idea for a solution here would be to make intel_dp_handle_test_request() run on its own delayed work function. It would wait for both i915_digport_work_func() and i915_hotplug_work_func() to finish, and only then it would do the normal processing. With this, we would be able to avoid the edid read on this patch, we would maybe be able to avoid at least part of patch 2, we would maybe be able to completely avoid patch 7, and then on patch 8 we would start touching intel_dp_get_edid() instead.

I know this is sort of a fundamental change that is being requested a little late in the review process, and it can be frustrating, but this aspect of the code only recently changed (I was fine with the EDID reads just in the compliance testing function), and since the DP compliance code is quite complex, it took me a while to realize everything that's going on and what is the purpose of each piece. I also think that, since this idea will allow the compliance testing to take into consideration the work done by i915_hotplug_work_func(), compliance testing will better reflect the behavior that is actually done by the Kernel when DP devices are plugged/unplugged. And I did ask about those new EDID reads as soon as I started reviewing the patch that introduced them.

Quoted 8 lines

Now, since I know how frustrating it is to have to change a significant portion of the code once again, I will leave to the maintainers the decision of whether the current proposed implementation is acceptable or if we want to make the DP compliance testing code take into consideration the work done by i915_hotplug_work_func(). I would also like to know your opinion on this. Maybe my idea just doesn't make sense because of something else I didn't realize :)

Quoted 5 lines

I don't think this is a good idea. The work loop aspect seems like a very complex solution solution to a problem that is relatively simple. In a discussion with Daniel, he indicated that adding a work loop is something to be avoided unless it's *really* necessary, as they are prone to race conditions. In this case, I just don't see that it's necessary.

Quoted 6 lines

The workqueue thing was just an idea to implement a solution for the real problem. I think we should be focusing about discussing the fact that we're not taking i915_hotplug_work_func() into consideration when doing the compliance testing, not on the fact that one of the possible implementations could use a workqueue. I'd still like to hear your arguments on that.

Fair enough.

So I've been looking into this and why the i915_hotplug_work_func wasn't part of this. It is, as you said, a relatively fundamental code path for Displayport through the driver. What I discovered was that this function is never called on HSW (my primary test vehicle), mainly because check_link_status() returns IRQ_HANDLED instead of IRQ_NONE. The work function for HSW is i915_digport_work_func, so when it gets the IRQ_HANDLED return code, it doesn't fall through to the legacy i915_hotplug_work_func handler. This is important because this handler calls intel_hpd_irq_event which is where the ->detect connector function is called. And intel_dp_detect() is where all the happy goodness for Displayport begins.

Up until I discovered this, I had mistakenly propagated that problem forward in to the SST case in intel_dp_hot_pulse() in patch 6 by returning IRQ_HANDLED instead of IRQ_NONE, which is what the code was doing for SST prior to patch 6. With this problem corrected (as it is in the latest update in patch set V6) the work functions are now called as they should be. The point being that this opens up the possibility of using elements along this path to pass compliance testing, thereby creating a more valid test case.

Quoted 10 lines

With this in mind, I am not opposed to using elements along that path to satisfy compliance requirements (that's the spirit of the tests, anyways) but as I've indicated, there are cases where we need to take special steps (like the edid_corrupt flag) in order to do the right things to pass the tests. I have concerns about trying to do that at this point, as it requires substantial rework to that code path that have a significant chance of breaking things. So to avoid that, I propose that this patch be merged now so that a working solution is in place. This discussion should continue and we can decide where to put things in the hotplug_work path to satisfy the compliance requires over the course of some followup patches.

I've looked a bit at all this and I think the other issue here is the placement of intel_dp_handle_test_request in check_link_status. This has been done almost 4 years ago in

commit a60f0e38d72a5e24085d6e7e27a4cadc20ae268a

Author: Jesse Barnes <[email protected]>
Date:   Thu Oct 20 15:09:17 2011 -0700

drm/i915: add DP test request handling

but never contained (up to this point) any functional code. It was however dutifully moved around together with the other code. And way back the placement even made some sense - check_link_status was called unconditionally from our hot_plug handler. But since the MST rework (and a few other things) happened that has been changed pretty radically and the current place where the test request handler is called just doesn't seem to make that much sense any more.

While I started pulling in patches I also noticed other places where we duplicate existing logic (e.g. the dpcd read), so this isn't just about the edid.

The other aspect here is that nowadays we do cache the edid for dp ports aggressively, which means if we don't read the edid the kernel will indeed keep on using a stale one. Hence there's a good risk that we don't just have a minor piece of duct-tape to keep the somewhat strange expectations of DP compliance testers happy but might be hiding a real bug in our DP code. Giving how many we've had that seems fairly likely and I'm not happy with sweeping this under the rug. This definitely needs a solid analysis and explanation either way (i.e. whether this is a bug or just an overly strict dp compliance tester requirement).

Finally there's the dp hotplug handling. Ever since MST support was merged this has a been a lot of fun and took us a while to make it work correctly
- lots of deadlocks and other issues. And given the above we still seem to have regressions due to MST support, or at least evidence for such. Since this is a fairly fragile piece of code, which is also not that well-tested (we don't have any MST hw in our test matrix yet) I prefer to keep changes to a minimum. Merging these patches here first and then potentially undoing them again because the bug has been the (mis)placement of the test request handler in the MST patches feels too risky.

Given all that I'd like to hold off merging these patches that rework the code around the check_link_status function until we have clarity here.

I've pulled in the other patches meanwhile which are reviewed and ready. I also think we can pull in the drm_edid.c patch with the statistics code we need for compliance testing ahead of resolving the above opens. And it would be good to get some feedback from other (non-intel) drm developers beforehand, the changes are quite invasive in some parts.

I've chatted a lot with Todd and Paulo and I think my decision here and the rough plan laid out is the best choice I have from a technical point of view.

Thanks, Daniel

Quoted 149 lines · Todd Previte

Quoted 121 lines · Paulo Zanoni

2015-04-15 12:37 GMT-03:00 Todd Previte <[email protected]>:

Quoted 115 lines · Paulo Zanoni

Quoted 106 lines

2015-04-13 11:53 GMT-03:00 Todd Previte <[email protected]>:

Quoted 36 lines

Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the
Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for all HPD plug events. To reduce the amount of code, this EDID read is also used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual support for these tests is implemented in later patches in this series.

V2:
- Fixed compilation error introduced during rework

Signed-off-by Todd Previte <[email protected]>

---
   drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
   1 file changed, 11 insertions(+)

drivers/gpu/drm/i915/intel_dp.c +3 -0

index 23184b0..75df3e2 100644
@@ -3890,6 +3890,9 @@ intel_dp_check_link_status(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;
+       struct drm_connector *connector =
 &intel_dp->attached_connector->base;
+       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+       struct edid *edid_read = NULL;

          u8 sink_irq_vector;
          u8 link_status[DP_LINK_STATUS_SIZE];

drivers/gpu/drm/i915/intel_dp.c +0 -0

@@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp
 *intel_dp)
                  return;

drivers/gpu/drm/i915/intel_dp.c intel_dp_check_link_status(struct intel_dp · line 3920

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp.c +8 -0

          }
 
+       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
+        * 4.2.2.1 - EDID read required for all HPD events
+         */
+        edid_read = drm_get_edid(connector, adapter);
+        if (!edid_read) {
+                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
+        }
+

We already briefly discussed this patch in private, so I'm going to summarize the discussion and also add some more points here.

Frist, the actual detailed review: the indentation here is using spaces and we're leaking the EDID. This will cause rebases to a few of the next patches.

Back to the hight level architecture: your initial versions of the series contained just 1 extra EDID read, and it was contained inside the compliance testing function. Then the versions submitted a few days ago had 2 extra EDID reads, then after some discussion you reduced to 1 extra EDID read (the one on this patch). I previously asked "But what about the automatic EDID read we do when we get a hotplug? Can't we just rely on it?". I got some answers to the question, but I was not really convinced.

Yesterday I was arguing that this extra EDID read is going to add a small delay to every hotplug event we get, so my initial suggestion was to organize the compliance testing in a way that would require the user space program to call the GetResources() IOCTL to force the EDID when needed. Your argument was that then the DP compliance testing procedure would be testing our app for compliance, not the Kernel.

But today I decided to finally do some debugging regarding this, and I was able to confirm that we do follow the DP requirements: we do have an automatic EDID read done by the Kernel whenever we do a hotplug:
i915_hotplug_work_func() calls intel_dp_detect(), which ends calling drm_get_edid() at some point. This function also does other stuff that is required by the compliance testing, such as the DPCD reads.

Now there's a problem with using i915_hotplug_work_func(), which could the reason why you rejected it: it only happens after intel_dp_hpd_pulse(), which means that we only really do the EDID read after intel_dp_handle_test_request().

I consider i915_hotplug_work_func() a fundamental part of our DP framework, and the DP compliance testing seems to be just ignoring its existence. So my idea for a solution here would be to make intel_dp_handle_test_request() run on its own delayed work function. It would wait for both i915_digport_work_func() and i915_hotplug_work_func() to finish, and only then it would do the normal processing. With this, we would be able to avoid the edid read on this patch, we would maybe be able to avoid at least part of patch 2, we would maybe be able to completely avoid patch 7, and then on patch 8 we would start touching intel_dp_get_edid() instead.

I know this is sort of a fundamental change that is being requested a little late in the review process, and it can be frustrating, but this aspect of the code only recently changed (I was fine with the EDID reads just in the compliance testing function), and since the DP compliance code is quite complex, it took me a while to realize everything that's going on and what is the purpose of each piece. I also think that, since this idea will allow the compliance testing to take into consideration the work done by i915_hotplug_work_func(), compliance testing will better reflect the behavior that is actually done by the Kernel when DP devices are plugged/unplugged. And I did ask about those new EDID reads as soon as I started reviewing the patch that introduced them.

Quoted 8 lines

Now, since I know how frustrating it is to have to change a significant portion of the code once again, I will leave to the maintainers the decision of whether the current proposed implementation is acceptable or if we want to make the DP compliance testing code take into consideration the work done by i915_hotplug_work_func(). I would also like to know your opinion on this. Maybe my idea just doesn't make sense because of something else I didn't realize :)

Quoted 5 lines

I don't think this is a good idea. The work loop aspect seems like a very complex solution solution to a problem that is relatively simple. In a discussion with Daniel, he indicated that adding a work loop is something to be avoided unless it's *really* necessary, as they are prone to race conditions. In this case, I just don't see that it's necessary.

Quoted 6 lines

The workqueue thing was just an idea to implement a solution for the real problem. I think we should be focusing about discussing the fact that we're not taking i915_hotplug_work_func() into consideration when doing the compliance testing, not on the fact that one of the possible implementations could use a workqueue. I'd still like to hear your arguments on that.

Fair enough.

So I've been looking into this and why the i915_hotplug_work_func wasn't part of this. It is, as you said, a relatively fundamental code path for Displayport through the driver. What I discovered was that this function is never called on HSW (my primary test vehicle), mainly because check_link_status() returns IRQ_HANDLED instead of IRQ_NONE. The work function for HSW is i915_digport_work_func, so when it gets the IRQ_HANDLED return code, it doesn't fall through to the legacy i915_hotplug_work_func handler. This is important because this handler calls intel_hpd_irq_event which is where the ->detect connector function is called. And intel_dp_detect() is where all the happy goodness for Displayport begins.

Up until I discovered this, I had mistakenly propagated that problem forward in to the SST case in intel_dp_hot_pulse() in patch 6 by returning IRQ_HANDLED instead of IRQ_NONE, which is what the code was doing for SST prior to patch 6. With this problem corrected (as it is in the latest update in patch set V6) the work functions are now called as they should be. The point being that this opens up the possibility of using elements along this path to pass compliance testing, thereby creating a more valid test case.

Quoted 10 lines

With this in mind, I am not opposed to using elements along that path to satisfy compliance requirements (that's the spirit of the tests, anyways) but as I've indicated, there are cases where we need to take special steps (like the edid_corrupt flag) in order to do the right things to pass the tests. I have concerns about trying to do that at this point, as it requires substantial rework to that code path that have a significant chance of breaking things. So to avoid that, I propose that this patch be merged now so that a working solution is in place. This discussion should continue and we can decide where to put things in the hotplug_work path to satisfy the compliance requires over the course of some followup patches.

I've looked a bit at all this and I think the other issue here is the placement of intel_dp_handle_test_request in check_link_status. This has been done almost 4 years ago in

commit a60f0e38d72a5e24085d6e7e27a4cadc20ae268a

Author: Jesse Barnes <[email protected]>
Date:   Thu Oct 20 15:09:17 2011 -0700

drm/i915: add DP test request handling

but never contained (up to this point) any functional code. It was however dutifully moved around together with the other code. And way back the placement even made some sense - check_link_status was called unconditionally from our hot_plug handler. But since the MST rework (and a few other things) happened that has been changed pretty radically and the current place where the test request handler is called just doesn't seem to make that much sense any more.

While I started pulling in patches I also noticed other places where we duplicate existing logic (e.g. the dpcd read), so this isn't just about the edid.

The other aspect here is that nowadays we do cache the edid for dp ports aggressively, which means if we don't read the edid the kernel will indeed keep on using a stale one. Hence there's a good risk that we don't just have a minor piece of duct-tape to keep the somewhat strange expectations of DP compliance testers happy but might be hiding a real bug in our DP code. Giving how many we've had that seems fairly likely and I'm not happy with sweeping this under the rug. This definitely needs a solid analysis and explanation either way (i.e. whether this is a bug or just an overly strict dp compliance tester requirement).

Finally there's the dp hotplug handling. Ever since MST support was merged this has a been a lot of fun and took us a while to make it work correctly
- lots of deadlocks and other issues. And given the above we still seem to have regressions due to MST support, or at least evidence for such. Since this is a fairly fragile piece of code, which is also not that well-tested (we don't have any MST hw in our test matrix yet) I prefer to keep changes to a minimum. Merging these patches here first and then potentially undoing them again because the bug has been the (mis)placement of the test request handler in the MST patches feels too risky.

Given all that I'd like to hold off merging these patches that rework the code around the check_link_status function until we have clarity here.

I've pulled in the other patches meanwhile which are reviewed and ready. I also think we can pull in the drm_edid.c patch with the statistics code we need for compliance testing ahead of resolving the above opens. And it would be good to get some feedback from other (non-intel) drm developers beforehand, the changes are quite invasive in some parts.

I've chatted a lot with Todd and Paulo and I think my decision here and the rough plan laid out is the best choice I have from a technical point of view.

Thanks, Daniel

The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1 specifies that repeated AUX transactions after a failure (no response / invalid response) must have a minimum delay of 400us before the resend can occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.

Also, the check for DP_AUX_CH_CTL_TIME_OUT_ERROR has been moved out into a separate case. This case just continues with the next iteration of the loop as the HW has already waited the required amount of time.

V2:
- Changed udelay() to usleep_range()
V3:
- Removed extraneous check for timeout
- Updated comment to reflect this change
V4:
- Reformatted a comment
V5:
- Added separate check for HW timeout on AUX transactions. A message is logged upon detection of this case.
V6:
- Add continue statement to HW timeout detect case
- Remove the log message indicating a timeout has been detected (review feedback)
V7:
- Updated the commit message to remove verbage about the HW timeout case that is no longer valid.

Signed-off-by Todd Previte <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +11 -3

index a5dfaff..77b6b15 100644
@@ -877,9 +877,18 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
                                   DP_AUX_CH_CTL_TIME_OUT_ERROR |
                                   DP_AUX_CH_CTL_RECEIVE_ERROR);
 
-                       if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
-                                     DP_AUX_CH_CTL_RECEIVE_ERROR))
+                       if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR)
                                continue;
+
+                       /* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
+                        *   400us delay required for errors and timeouts
+                        *   Timeout errors from the HW already meet this
+                        *   requirement so skip to next iteration
+                        */
+                       if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
+                               usleep_range(400, 500);
+                               continue;
+                       }
                        if (status & DP_AUX_CH_CTL_DONE)
                                break;
                }
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting a corrupt EDID. The test specification states that the sink device sets up the EDID with an invalid checksum. To do this, the sink sets up an invalid EDID header, expecting the source device to generate the checksum and compare it to the value stored in the last byte of the block data.

Unfortunately, the DRM EDID reading and parsing functions are actually too good in this case; the header is fixed before the checksum is computed and thus the code never sees the invalid checksum. This results in a failure to pass the compliance test.

To correct this issue, a checksum is generated when the EDID header is detected as corrupted. If the checksum is invalid, it sets the header_corrupt flag and logs the errors. In the case of a more seriously damaged header (fixup score less than the threshold) the code does not generate the checksum but does set the header_corrupt flag.

V2:
- Removed the static bool global
- Added a bool to the drm_connector struct to reaplce the static one for holding the status of raw edid header corruption detection
- Modified the function signature of the is_valid function to take an additional parameter to store the corruption detected value
- Fixed the other callers of the above is_valid function
V3:
- Updated the commit message to be more clear about what and why this patch does what it does.
- Added comment in code to clarify the operations there
- Removed compliance variable and check_link_status update; those have been moved to a later patch
- Removed variable assignment from the bottom of the test handler

Signed-off-by Todd Previte <[email protected]>

Cc [email protected]

---
 drivers/gpu/drm/drm_edid.c        | 31 ++++++++++++++++++++++++++-----
 drivers/gpu/drm/drm_edid_load.c   |  7 +++++--
 drivers/gpu/drm/i2c/tda998x_drv.c |  4 ++--
 include/drm/drm_crtc.h            |  8 +++++++-
 4 files changed, 40 insertions(+), 10 deletions(-)

Hi Todd

Quoted 32 lines · Todd Previte

Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting a corrupt EDID. The test specification states that the sink device sets up the EDID with an invalid checksum. To do this, the sink sets up an invalid EDID header, expecting the source device to generate the checksum and compare it to the value stored in the last byte of the block data.

Unfortunately, the DRM EDID reading and parsing functions are actually too good in this case; the header is fixed before the checksum is computed and thus the code never sees the invalid checksum. This results in a failure to pass the compliance test.

To correct this issue, a checksum is generated when the EDID header is detected as corrupted. If the checksum is invalid, it sets the header_corrupt flag and logs the errors. In the case of a more seriously damaged header (fixup score less than the threshold) the code does not generate the checksum but does set the header_corrupt flag.

V2:
- Removed the static bool global
- Added a bool to the drm_connector struct to reaplce the static one for holding the status of raw edid header corruption detection
- Modified the function signature of the is_valid function to take an additional parameter to store the corruption detected value
- Fixed the other callers of the above is_valid function
V3:
- Updated the commit message to be more clear about what and why this patch does what it does.
- Added comment in code to clarify the operations there
- Removed compliance variable and check_link_status update; those have been moved to a later patch
- Removed variable assignment from the bottom of the test handler

Quoted 9 lines

Signed-off-by Todd Previte <[email protected]>

Cc [email protected]

---
  drivers/gpu/drm/drm_edid.c        | 31 ++++++++++++++++++++++++++-----
  drivers/gpu/drm/drm_edid_load.c   |  7 +++++--
  drivers/gpu/drm/i2c/tda998x_drv.c |  4 ++--
  include/drm/drm_crtc.h            |  8 +++++++-
  4 files changed, 40 insertions(+), 10 deletions(-)

Neither this nor patch 09/11 seems to be i915 specific. If you're doing another revision you might want to use just "drm:".

Cheers,
Emil

Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting a corrupt EDID. The test specification states that the sink device sets up the EDID with an invalid checksum. To do this, the sink sets up an invalid EDID header, expecting the source device to generate the checksum and compare it to the value stored in the last byte of the block data.

Unfortunately, the DRM EDID reading and parsing functions are actually too good in this case; the header is fixed before the checksum is computed and thus the code never sees the invalid checksum. This results in a failure to pass the compliance test.

To correct this issue, a checksum is generated when the EDID header is detected as corrupted. If the checksum is invalid, it sets the header_corrupt flag and logs the errors. In the case of a more seriously damaged header (fixup score less than the threshold) the code does not generate the checksum but does set the header_corrupt flag.

V2:
- Removed the static bool global
- Added a bool to the drm_connector struct to reaplce the static one for holding the status of raw edid header corruption detection
- Modified the function signature of the is_valid function to take an additional parameter to store the corruption detected value
- Fixed the other callers of the above is_valid function
V3:
- Updated the commit message to be more clear about what and why this patch does what it does.
- Added comment in code to clarify the operations there
- Removed compliance variable and check_link_status update; those have been moved to a later patch
- Removed variable assignment from the bottom of the test handler V4:
- Removed i915 tag from subject line as the patch is not i915-specific V5:
- Moved code causing a compilation error to this patch where the variable is actually declared

Signed-off-by Todd Previte <[email protected]>

Cc [email protected]

---
 drivers/gpu/drm/drm_edid.c        | 31 ++++++++++++++++++++++++++-----
 drivers/gpu/drm/drm_edid_load.c   |  7 +++++--
 drivers/gpu/drm/i2c/tda998x_drv.c |  4 ++--
 drivers/gpu/drm/i915/intel_dp.c   |  2 +-
 include/drm/drm_crtc.h            |  8 +++++++-
 5 files changed, 41 insertions(+), 11 deletions(-)

drivers/gpu/drm/drm_edid.c +0 -0

index 53bc7a6..12e5be7 100644

drivers/gpu/drm/drm_edid.c +1 -0

 {
        u8 csum;
        struct edid *edid = (struct edid *)raw_edid;

@@ -1062,9 +1062,27 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
                int score = drm_edid_header_is_valid(raw_edid);
                if (score == 8) ;
                else if (score >= edid_fixup) {
+                       /* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6

drivers/gpu/drm/drm_edid.c +3 -2

                        goto bad;
                }
        }

@@ -1129,7 +1147,7 @@ bool drm_edid_is_valid(struct edid *edid)
                return false;
 
        for (i = 0; i <= edid->extensions; i++)
-               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
+               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true, NULL))
                        return false;
 
        return true;

@@ -1232,7 +1250,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
        for (i = 0; i < 4; i++) {
                if (get_edid_block(data, block, 0, EDID_LENGTH))
                        goto out;
-               if (drm_edid_block_valid(block, 0, print_bad_edid))
+               if (drm_edid_block_valid(block, 0, print_bad_edid,
+                                        &connector->edid_header_corrupt))
                        break;
                if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
                        connector->null_edid_counter++;

drivers/gpu/drm/drm_edid.c +0 -0

                                valid_extensions++;
                                break;
                        }

drivers/gpu/drm/drm_edid_load.c +2 -1

index 732cb6f..1505494 100644
@@ -216,7 +216,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
                goto out;
        }
 
-       if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
+       if (!drm_edid_block_valid(edid, 0, print_bad_edid,
+                                 &connector->edid_header_corrupt)) {
                connector->bad_edid_counter++;
                DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
                    name);

drivers/gpu/drm/drm_edid_load.c +0 -0

                        valid_extensions++;
        }
 

drivers/gpu/drm/i2c/tda998x_drv.c +2 -2

index a9041d1..9c3d6b3 100644
@@ -1106,7 +1106,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
        if (read_edid_block(priv, block, 0))
                goto fail;
 
-       if (!drm_edid_block_valid(block, 0, print_bad_edid))
+       if (!drm_edid_block_valid(block, 0, print_bad_edid, NULL))
                goto fail;
 
        /* if there's no extensions, we're done */

@@ -1123,7 +1123,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
                if (read_edid_block(priv, ext_block, j))
                        goto fail;
 
-               if (!drm_edid_block_valid(ext_block, j, print_bad_edid))
+               if (!drm_edid_block_valid(ext_block, j, print_bad_edid, NULL))
                        goto fail;
 
                valid_extensions++;

drivers/gpu/drm/i915/intel_dp.c +1 -1

index e1d6e79..77b6b15 100644
@@ -3922,7 +3922,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
         * 4.2.2.1 - EDID read required for all HPD events
          */
         edid_read = drm_get_edid(connector, adapter);
-        if (!edid_read) {
+        if (!edid_read || connector->edid_header_corrupt == 1) {
                 DRM_DEBUG_DRIVER("Invalid EDID detected\n");
         }
 

include/drm/drm_crtc.h +7 -2

index 0261417..e31a4b3 100644
@@ -719,6 +719,11 @@ struct drm_connector {
        int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
        unsigned bad_edid_counter;
 
+       /* Flag for raw EDID header corruption - used in Displayport compliance testing
+        * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
+        */
+       bool edid_header_corrupt;
+
        struct dentry *debugfs_entry;
 
        struct drm_connector_state *state;

@@ -1436,7 +1441,8 @@ extern void drm_set_preferred_mode(struct drm_connector *connector,
                                   int hpref, int vpref);
 
 extern int drm_edid_header_is_valid(const u8 *raw_edid);
-extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid);
+extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, bool *header_corrupt);
+
 extern bool drm_edid_is_valid(struct edid *edid);
 
 extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
--

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

2015-04-13 11:53 GMT-03:00 Todd Previte <[email protected]>:

Quoted 47 lines

Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting a corrupt EDID. The test specification states that the sink device sets up the EDID with an invalid checksum. To do this, the sink sets up an invalid EDID header, expecting the source device to generate the checksum and compare it to the value stored in the last byte of the block data.

Unfortunately, the DRM EDID reading and parsing functions are actually too good in this case; the header is fixed before the checksum is computed and thus the code never sees the invalid checksum. This results in a failure to pass the compliance test.

To correct this issue, a checksum is generated when the EDID header is detected as corrupted. If the checksum is invalid, it sets the header_corrupt flag and logs the errors. In the case of a more seriously damaged header (fixup score less than the threshold) the code does not generate the checksum but does set the header_corrupt flag.

V2:
- Removed the static bool global
- Added a bool to the drm_connector struct to reaplce the static one for holding the status of raw edid header corruption detection
- Modified the function signature of the is_valid function to take an additional parameter to store the corruption detected value
- Fixed the other callers of the above is_valid function
V3:
- Updated the commit message to be more clear about what and why this patch does what it does.
- Added comment in code to clarify the operations there
- Removed compliance variable and check_link_status update; those have been moved to a later patch
- Removed variable assignment from the bottom of the test handler V4:
- Removed i915 tag from subject line as the patch is not i915-specific V5:
- Moved code causing a compilation error to this patch where the variable is actually declared

Signed-off-by Todd Previte <[email protected]>

Cc [email protected]

---
  drivers/gpu/drm/drm_edid.c        | 31 ++++++++++++++++++++++++++-----
  drivers/gpu/drm/drm_edid_load.c   |  7 +++++--
  drivers/gpu/drm/i2c/tda998x_drv.c |  4 ++--
  drivers/gpu/drm/i915/intel_dp.c   |  2 +-
  include/drm/drm_crtc.h            |  8 +++++++-
  5 files changed, 41 insertions(+), 11 deletions(-)

drivers/gpu/drm/drm_edid.c int drm_edid_header_is_valid(const u8 *raw_edid) · line 1010

Quoted 12 lines

drivers/gpu/drm/drm_edid.c +0 -1

index 53bc7a6..12e5be7 100644
@@ -1005,7 +1005,6 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
         for (i = 0; i < sizeof(edid_header); i++)
                 if (raw_edid[i] == edid_header[i])
                         score++;
-
         return score;
  }
  EXPORT_SYMBOL(drm_edid_header_is_valid);

Bad chunk...

drivers/gpu/drm/drm_edid.c static bool drm_edid_is_zero(const u8 *in_edid, int length) · line 1050

Quoted 8 lines

drivers/gpu/drm/drm_edid.c +2 -1

@@ -1047,7 +1046,8 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
   *
   * Return: True if the block is valid, false otherwise.
   */
-bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
+bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
+                         bool *header_corrupt)

Need to add the new parameter description to the documentation above.

Quoted 9 lines

drivers/gpu/drm/drm_edid.c +1 -0

  {
         u8 csum;
         struct edid *edid = (struct edid *)raw_edid;

@@ -1062,9 +1062,27 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
                 int score = drm_edid_header_is_valid(raw_edid);
                 if (score == 8) ;
                 else if (score >= edid_fixup) {
+                       /* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6

drivers/gpu/drm/drm_edid.c bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) · line 1075

Quoted 10 lines

drivers/gpu/drm/drm_edid.c +10 -0

+                        * In order to properly generate the invalid checksum
+                        * required for this test, it must be generated using
+                        * the raw EDID data. Otherwise, the fix-up code here
+                        * will correct the problem, the checksum is then correct
+                        * and the test fails
+                        */
+                       csum = drm_edid_block_checksum(raw_edid);
+                       if (csum) {
+                               DRM_DEBUG_DRIVER("Invalid EDID header, score = %d\n", score);
+                               DRM_DEBUG_DRIVER("Invalid EDID checksum %d\n", csum);

No one on this file uses DRM_DEBUG_DRIVER (you use 2 calls here and one below).

Also, during "normal operation" we try to calculate the checksum based on the fixed EDID header, so if we also print these messages here we're always going to have a message complaining about invalid
checksum: either this one or the other that's already there. My bikeshed would be to just remove the messages you added here and below to not confuse users. Let's assume that the bad header is due to some communication/corruption error, and the HW manufacturers did not program an EDID with a bad header and a correct checksum based on bad header :)

drivers/gpu/drm/drm_edid.c bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) · line 1085

Quoted 11 lines

drivers/gpu/drm/drm_edid.c +7 -0

+                               if (header_corrupt)
+                                       *header_corrupt = 1;
+                       }
                         DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
                         memcpy(raw_edid, edid_header, sizeof(edid_header));
                 } else {
+                       if (header_corrupt) {
+                               DRM_DEBUG_DRIVER("Invalid EDID header\n");
+                               *header_corrupt = 1;
+                       }

We don't ever set header_corrupt back to false, so if we ever get a corrupt header once, the header_corrupt flag will stay there even if it gets fixed somehow (such as a DP compliance tester starting to submit the correct header).

Quoted 24 lines

drivers/gpu/drm/drm_edid.c +3 -2

                         goto bad;
                 }
         }

@@ -1129,7 +1147,7 @@ bool drm_edid_is_valid(struct edid *edid)
                 return false;
 
         for (i = 0; i <= edid->extensions; i++)
-               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
+               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true, NULL))
                         return false;
 
         return true;

@@ -1232,7 +1250,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
         for (i = 0; i < 4; i++) {
                 if (get_edid_block(data, block, 0, EDID_LENGTH))
                         goto out;
-               if (drm_edid_block_valid(block, 0, print_bad_edid))
+               if (drm_edid_block_valid(block, 0, print_bad_edid,
+                                        &connector->edid_header_corrupt))
                         break;
                 if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
                         connector->null_edid_counter++;

@@ -1257,7 +1276,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,

drivers/gpu/drm/drm_edid.c struct edid *drm_do_get_edid(struct drm_connector *connector, · line 1281

Quoted 7 lines

drivers/gpu/drm/drm_edid.c +3 -1

                                   block + (valid_extensions + 1) * EDID_LENGTH,
                                   j, EDID_LENGTH))
                                 goto out;
-                       if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
+                       if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j,
+                                                print_bad_edid,
+                                                &connector->edid_header_corrupt)) {

You can use NULL here since we're not handling block 0 anymore.

Quoted 12 lines

drivers/gpu/drm/drm_edid.c +0 -0

                                 valid_extensions++;
                                 break;
                         }

drivers/gpu/drm/drm_edid_load.c +0 -0

index 732cb6f..1505494 100644
@@ -216,7 +216,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
                 goto out;
         }
 

Quoted 7 lines

drivers/gpu/drm/drm_edid_load.c +2 -1

-       if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
+       if (!drm_edid_block_valid(edid, 0, print_bad_edid,
+                                 &connector->edid_header_corrupt)) {
                 connector->bad_edid_counter++;
                 DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
                     name);

drivers/gpu/drm/drm_edid_load.c static void *edid_load(struct drm_connector *connector, const char *name, · line 235

Quoted 7 lines

drivers/gpu/drm/drm_edid_load.c +3 -1

@@ -229,7 +230,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
                 if (i != valid_extensions + 1)
                         memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
                             edid + i * EDID_LENGTH, EDID_LENGTH);
-               if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid))
+               if (drm_edid_block_valid(edid + i * EDID_LENGTH, i,
+                                        print_bad_edid,
+                                        &connector->edid_header_corrupt))

Same here: not iterating over block 0.

Btw, why can't we just use NULL on edid_load?

Quoted 64 lines

drivers/gpu/drm/drm_edid_load.c +0 -0

                         valid_extensions++;
         }
 

drivers/gpu/drm/i2c/tda998x_drv.c +2 -2

index a9041d1..9c3d6b3 100644
@@ -1106,7 +1106,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
         if (read_edid_block(priv, block, 0))
                 goto fail;
 
-       if (!drm_edid_block_valid(block, 0, print_bad_edid))
+       if (!drm_edid_block_valid(block, 0, print_bad_edid, NULL))
                 goto fail;
 
         /* if there's no extensions, we're done */

@@ -1123,7 +1123,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
                 if (read_edid_block(priv, ext_block, j))
                         goto fail;
 
-               if (!drm_edid_block_valid(ext_block, j, print_bad_edid))
+               if (!drm_edid_block_valid(ext_block, j, print_bad_edid, NULL))
                         goto fail;
 
                 valid_extensions++;

drivers/gpu/drm/i915/intel_dp.c +1 -1

index e1d6e79..77b6b15 100644
@@ -3922,7 +3922,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
          * 4.2.2.1 - EDID read required for all HPD events
           */
          edid_read = drm_get_edid(connector, adapter);
-        if (!edid_read) {
+        if (!edid_read || connector->edid_header_corrupt == 1) {
                  DRM_DEBUG_DRIVER("Invalid EDID detected\n");
          }
 

include/drm/drm_crtc.h +7 -1

index 0261417..e31a4b3 100644
@@ -719,6 +719,11 @@ struct drm_connector {
         int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
         unsigned bad_edid_counter;
 
+       /* Flag for raw EDID header corruption - used in Displayport compliance testing
+        * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
+        */
+       bool edid_header_corrupt;
+
         struct dentry *debugfs_entry;
 
         struct drm_connector_state *state;

@@ -1436,7 +1441,8 @@ extern void drm_set_preferred_mode(struct drm_connector *connector,
                                    int hpref, int vpref);
 
  extern int drm_edid_header_is_valid(const u8 *raw_edid);
-extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid);
+extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, bool *header_corrupt);
+
  extern bool drm_edid_is_valid(struct edid *edid);
 

Quoted 8 lines

include/drm/drm_crtc.h +0 -1

  extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
--

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Update the hot plug function to handle the SST case. Instead of placing the SST case within the long/short pulse block, it is now handled after determining that MST mode is not in use. This way, the topology management layer can handle any MST-related operations while SST operations are still correctly handled afterwards.

This patch also corrects the problem of SST mode only being handled in the case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes both short and long pulses are used by the different tests, thus both cases need to be addressed for SST.

This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the previous compliance testing patch sequence. Review feedback on that patch indicated that updating intel_dp_hot_plug() was not the correct place for the test handler.

For the SST case, the main stream is disabled for long HPD pulses as this generally indicates either a connect/disconnect event or link failure. For a number of case in compliance testing, the source is required to disable the main link upon detection of a long HPD.

V2:
- N/A
V3:
- Place the SST mode link status check into the mst_fail case
- Remove obsolete comment regarding SST mode operation
- Removed an erroneous line of code that snuck in during rebasing V4:
- Added a disable of the main stream (DP transport) for the long pulse case for SST to support compliance testing
V5:
- Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8 V6:
- Reformatted a comment

Signed-off-by Todd Previte <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +0 -1

index 77b6b15..ba2da44 100644
@@ -4572,29 +4572,26 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
                        if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
                                goto mst_fail;
                }
-

drivers/gpu/drm/i915/intel_dp.c +7 -1

                DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
                intel_dp->is_mst = false;
                drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
        }
 put_power:
+       /* SST mode - handle short/long pulses here */
+       if (!intel_dp->is_mst) {
+               drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+               intel_dp_check_link_status(intel_dp);
+               drm_modeset_unlock(&dev->mode_config.connection_mutex);
+               ret = IRQ_HANDLED;
+       }
        intel_display_power_put(dev_priv, power_domain);
 
        return ret;
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-04-10 13:12 GMT-03:00 Todd Previte <[email protected]>:

Quoted 56 lines

Update the hot plug function to handle the SST case. Instead of placing the SST case within the long/short pulse block, it is now handled after determining that MST mode is not in use. This way, the topology management layer can handle any MST-related operations while SST operations are still correctly handled afterwards.

This patch also corrects the problem of SST mode only being handled in the case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes both short and long pulses are used by the different tests, thus both cases need to be addressed for SST.

This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the previous compliance testing patch sequence. Review feedback on that patch indicated that updating intel_dp_hot_plug() was not the correct place for the test handler.

For the SST case, the main stream is disabled for long HPD pulses as this generally indicates either a connect/disconnect event or link failure. For a number of case in compliance testing, the source is required to disable the main link upon detection of a long HPD.

V2:
- N/A
V3:
- Place the SST mode link status check into the mst_fail case
- Remove obsolete comment regarding SST mode operation
- Removed an erroneous line of code that snuck in during rebasing V4:
- Added a disable of the main stream (DP transport) for the long pulse case for SST to support compliance testing
V5:
- Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8 V6:
- Reformatted a comment

Signed-off-by Todd Previte <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++-----------
  1 file changed, 8 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +0 -8

index 77b6b15..ba2da44 100644
@@ -4572,29 +4572,26 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
                         if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
                                 goto mst_fail;
                 }
-
-               if (!intel_dp->is_mst) {
-                       /*
-                        * we'll check the link status via the normal hot plug path later -
-                        * but for short hpds we should check it now
-                        */
-                       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-                       intel_dp_check_link_status(intel_dp);

drivers/gpu/drm/i915/intel_dp.c intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) · line 4575

Quoted 3 lines

drivers/gpu/drm/i915/intel_dp.c +0 -2

-                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
-               }
         }

Shouldn't the code be moved to exactly this spot instead of after the put_power label? Why would we want to call check_link_status in case we goto mst_fail? In case there is a valid reason, maybe it would be better to do a big reorganization of this function because it's going to start looking very weird - or at least rename the labels.

Also, for the long_hpd case, I see that check_link_status() will redo some of the stuff we already did on this function, such as get_dpcd(). And if you follow my advice on patch 2, you will end up having even more repeated code. I think you could try to do a careful analysis here to make sure we're not calling stuff twice here, especially since some of those operations are potentially slow.

drivers/gpu/drm/i915/intel_dp.c intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) · line 4582

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp.c +1 -1

         ret = IRQ_HANDLED;
 
         goto put_power;
  mst_fail:
-       /* if we were in MST mode, and device is not there get out of MST mode */
         if (intel_dp->is_mst) {
+               /* if we were in MST mode, and device is not there get out of MST mode */

I don't see the need for changes such as the one above - I saw similar cases in other patches you submitted. I often use git blame on comments in order to be able to see the whole context of the change, and a simple change like this makes it harder to blame. Also, you're not even fixing the 80 column problem here. And I do prefer the comment on top of the if statement.

Quoted 15 lines

drivers/gpu/drm/i915/intel_dp.c +7 -0

                 DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
                 intel_dp->is_mst = false;
                 drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
         }
  put_power:
+       /* SST mode - handle short/long pulses here */
+       if (!intel_dp->is_mst) {
+               drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+               intel_dp_check_link_status(intel_dp);
+               drm_modeset_unlock(&dev->mode_config.connection_mutex);
+               ret = IRQ_HANDLED;
+       }
         intel_display_power_put(dev_priv, power_domain);
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_dp.c +0 -1

         return ret;
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 74 lines

2015-04-10 13:12 GMT-03:00 Todd Previte <[email protected]>:

Quoted 72 lines

Update the hot plug function to handle the SST case. Instead of placing the SST case within the long/short pulse block, it is now handled after determining that MST mode is not in use. This way, the topology management
layer can handle any MST-related operations while SST operations are still
correctly handled afterwards.

This patch also corrects the problem of SST mode only being handled in the
case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes
both short and long pulses are used by the different tests, thus both cases
need to be addressed for SST.

This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
previous compliance testing patch sequence. Review feedback on that patch indicated that updating intel_dp_hot_plug() was not the correct place for the test handler.

For the SST case, the main stream is disabled for long HPD pulses as this generally indicates either a connect/disconnect event or link failure. For
a number of case in compliance testing, the source is required to disable the main link upon detection of a long HPD.

V2:
- N/A
V3:
- Place the SST mode link status check into the mst_fail case
- Remove obsolete comment regarding SST mode operation
- Removed an erroneous line of code that snuck in during rebasing V4:
- Added a disable of the main stream (DP transport) for the long pulse case
for SST to support compliance testing
V5:
- Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8 V6:
- Reformatted a comment

Signed-off-by Todd Previte <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++-----------
  1 file changed, 8 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +0 -5

index 77b6b15..ba2da44 100644
@@ -4572,29 +4572,26 @@ intel_dp_hpd_pulse(struct intel_digital_port
 *intel_dig_port, bool long_hpd)
                         if (intel_dp_check_mst_status(intel_dp) ==
-EINVAL)
                                 goto mst_fail;
                 }
-
-               if (!intel_dp->is_mst) {
-                       /*
-                        * we'll check the link status via the normal hot

plug path later -

drivers/gpu/drm/i915/intel_dp.c +0 -3

-                        * but for short hpds we should check it now
-                        */
-

drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);

drivers/gpu/drm/i915/intel_dp.c +0 -2

-                       intel_dp_check_link_status(intel_dp);
-

drm_modeset_unlock(&dev->mode_config.connection_mutex);

Quoted 8 lines

Quoted 2 lines

- }

         }

Shouldn't the code be moved to exactly this spot instead of after the put_power label? Why would we want to call check_link_status in case we goto mst_fail? In case there is a valid reason, maybe it would be better to do a big reorganization of this function because it's going to start looking very weird - or at least rename the labels.

No because then you don't get long pulses, only short ones.

The put_power
case is where this belongs, unless you want to duplicate code in both the long_pulse and the else clause. There is a separate mst_check_link_status call so this one is specific to SST mode. There is also a check to make sure it doesn't get called when MST is active and MST has hit a failure mode, so that is a non-issue.

Quoted 7 lines

Also, for the long_hpd case, I see that check_link_status() will redo some of the stuff we already did on this function, such as get_dpcd(). And if you follow my advice on patch 2, you will end up having even more repeated code. I think you could try to do a careful analysis here to make sure we're not calling stuff twice here, especially since some of those operations are potentially slow.

I see a couple places where the code is duplicated, specifically the connection check (which I encapsulated in a function and I'll likely roll forward into this one since it makes things more clear) and the DPCD read in the long pulse case. I removed the code in check_link_status for both of these things and it still passes compliance. Good catch Paulo. This has been fixed and tested and will be in the updated patch posted shortly.

Quoted 11 lines

Quoted 10 lines

         ret = IRQ_HANDLED;

         goto put_power;
  mst_fail:
-       /* if we were in MST mode, and device is not there get out of MST
mode */
         if (intel_dp->is_mst) {

+ /* if we were in MST mode, and device is not there get
out of MST mode */

Quoted 6 lines

I don't see the need for changes such as the one above - I saw similar cases in other patches you submitted. I often use git blame on comments in order to be able to see the whole context of the change, and a simple change like this makes it harder to blame. Also, you're not even fixing the 80 column problem here. And I do prefer the comment on top of the if statement.

This is just an artifact of moving things around, as it likely was in the other patches. The only reason I will move comments is to clarify what they pertain to if code is moving around it. It's back where it belongs now so it doesn't even show up in the patch. Fixed for the next version.

Quoted 29 lines

Quoted 26 lines

DRM_DEBUG_KMS("MST device may have disappeared %d vs

%d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
                 intel_dp->is_mst = false;
                 drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
intel_dp->is_mst);
         }

put_power:

drivers/gpu/drm/i915/intel_dp.c +3 -0

+       /* SST mode - handle short/long pulses here */
+       if (!intel_dp->is_mst) {
+               drm_modeset_lock(&dev->mode_config.connection_mutex,

NULL);

drivers/gpu/drm/i915/intel_dp.c +4 -1

+               intel_dp_check_link_status(intel_dp);
+               drm_modeset_unlock(&dev->mode_config.connection_mutex);
+               ret = IRQ_HANDLED;
+       }
         intel_display_power_put(dev_priv, power_domain);
 
         return ret;
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-04-14 14:36 GMT-03:00 Todd Previte <[email protected]>:

Quoted 81 lines · Paulo Zanoni

Quoted 71 lines

2015-04-10 13:12 GMT-03:00 Todd Previte <[email protected]>:

Quoted 70 lines

Update the hot plug function to handle the SST case. Instead of placing the SST case within the long/short pulse block, it is now handled after determining that MST mode is not in use. This way, the topology management
layer can handle any MST-related operations while SST operations are still
correctly handled afterwards.

This patch also corrects the problem of SST mode only being handled in the
case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes
both short and long pulses are used by the different tests, thus both cases
need to be addressed for SST.

This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
previous compliance testing patch sequence. Review feedback on that patch indicated that updating intel_dp_hot_plug() was not the correct place for the test handler.

For the SST case, the main stream is disabled for long HPD pulses as this generally indicates either a connect/disconnect event or link failure. For
a number of case in compliance testing, the source is required to disable the main link upon detection of a long HPD.

V2:
- N/A
V3:
- Place the SST mode link status check into the mst_fail case
- Remove obsolete comment regarding SST mode operation
- Removed an erroneous line of code that snuck in during rebasing V4:
- Added a disable of the main stream (DP transport) for the long pulse case
for SST to support compliance testing
V5:
- Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8 V6:
- Reformatted a comment

Signed-off-by Todd Previte <[email protected]>

---
   drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++-----------
   1 file changed, 8 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +0 -5

index 77b6b15..ba2da44 100644
@@ -4572,29 +4572,26 @@ intel_dp_hpd_pulse(struct intel_digital_port
 *intel_dig_port, bool long_hpd)
                          if (intel_dp_check_mst_status(intel_dp) ==
-EINVAL)
                                  goto mst_fail;
                  }
-
-               if (!intel_dp->is_mst) {
-                       /*
-                        * we'll check the link status via the normal hot

plug path later -

drivers/gpu/drm/i915/intel_dp.c +0 -3

-                        * but for short hpds we should check it now
-                        */
-

drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);

drivers/gpu/drm/i915/intel_dp.c +0 -2

-                       intel_dp_check_link_status(intel_dp);
-

Quoted 8 lines

Quoted 3 lines

drm_modeset_unlock(&dev->mode_config.connection_mutex);
- }

          }

Shouldn't the code be moved to exactly this spot instead of after the put_power label? Why would we want to call check_link_status in case we goto mst_fail? In case there is a valid reason, maybe it would be better to do a big reorganization of this function because it's going to start looking very weird - or at least rename the labels.

Quoted 1 line

No because then you don't get long pulses, only short ones.

No, what I mean is:

if (long_hpd) {
     ... code ...
} else {
     ... code ....
}

if (!intel_dp->is_mst) {
     drm_modeset_lock(...)
     ... code ...
}

mst_fail:

     ... code ...

The other problem I point is: imagine we're SST and we get a long_hpd. Then we run ibx_digital_port_connected(), and since the monitor is disconnected we "goto mst_fail". We'll end up running intel_dp_check_link_status() before returning, but we really shouldn't run it since we know the monitor is disconnected.

Quoted 59 lines

The put_power
case is where this belongs, unless you want to duplicate code in both the long_pulse and the else clause. There is a separate mst_check_link_status call so this one is specific to SST mode. There is also a check to make sure it doesn't get called when MST is active and MST has hit a failure mode, so that is a non-issue.

Quoted 6 lines

Also, for the long_hpd case, I see that check_link_status() will redo some of the stuff we already did on this function, such as get_dpcd(). And if you follow my advice on patch 2, you will end up having even more repeated code. I think you could try to do a careful analysis here to make sure we're not calling stuff twice here, especially since some of those operations are potentially slow.

I see a couple places where the code is duplicated, specifically the connection check (which I encapsulated in a function and I'll likely roll forward into this one since it makes things more clear) and the DPCD read in the long pulse case. I removed the code in check_link_status for both of these things and it still passes compliance. Good catch Paulo. This has been fixed and tested and will be in the updated patch posted shortly.

Quoted 9 lines

Quoted 9 lines

          ret = IRQ_HANDLED;

          goto put_power;
   mst_fail:
-       /* if we were in MST mode, and device is not there get out of MST
mode */
          if (intel_dp->is_mst) {

+ /* if we were in MST mode, and device is not there get
out of MST mode */

Quoted 6 lines

I don't see the need for changes such as the one above - I saw similar cases in other patches you submitted. I often use git blame on comments in order to be able to see the whole context of the change, and a simple change like this makes it harder to blame. Also, you're not even fixing the 80 column problem here. And I do prefer the comment on top of the if statement.

This is just an artifact of moving things around, as it likely was in the other patches. The only reason I will move comments is to clarify what they pertain to if code is moving around it. It's back where it belongs now so it doesn't even show up in the patch. Fixed for the next version.

Quoted 21 lines

Quoted 21 lines

DRM_DEBUG_KMS("MST device may have disappeared %d vs

%d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
                  intel_dp->is_mst = false;
                  drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
intel_dp->is_mst);
          }

put_power:

drivers/gpu/drm/i915/intel_dp.c +3 -0

+       /* SST mode - handle short/long pulses here */
+       if (!intel_dp->is_mst) {
+               drm_modeset_lock(&dev->mode_config.connection_mutex,

NULL);

drivers/gpu/drm/i915/intel_dp.c +4 -1

+               intel_dp_check_link_status(intel_dp);
+               drm_modeset_unlock(&dev->mode_config.connection_mutex);
+               ret = IRQ_HANDLED;
+       }
          intel_display_power_put(dev_priv, power_domain);
 
          return ret;
--

1.9.1

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c  | 46 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  4 ++++
 2 files changed, 50 insertions(+)

drivers/gpu/drm/i915/intel_dp.c +9 -0

index ba2da44..3bfdc40 100644
@@ -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 ||

drivers/gpu/drm/i915/intel_dp.c +27 -0

+       } 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;
+
        return test_result;
 }
 

@@ -3785,7 +3820,10 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
        uint8_t rxdata = 0;
        int status = 0;
 
+       intel_dp->compliance_test_active = 0;
        intel_dp->compliance_test_type = 0;
+       intel_dp->compliance_test_data = 0;
+
        intel_dp->aux.i2c_nack_count = 0;
        intel_dp->aux.i2c_defer_count = 0;
 

@@ -3920,11 +3958,19 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
        /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
         * 4.2.2.1 - EDID read required for all HPD events
+        * Also used for 4.2.2.3, 4.2.2.4 4.2.2.5 and 4.2.2.6
+        *   - compliance_edid_invalid flag used for 4.2.2.6
+        *   - compliance_edid_checksum used for 4.2.2.3
          */
+       intel_dp->compliance_edid_invalid = 0;
         edid_read = drm_get_edid(connector, adapter);
         if (!edid_read || connector->edid_header_corrupt == 1) {
                 DRM_DEBUG_DRIVER("Invalid EDID detected\n");
+                intel_dp->compliance_edid_invalid = 1;
+               intel_dp->compliance_edid_checksum = 0;
         }
+       else
+               intel_dp->compliance_edid_checksum = edid_read->checksum;
 
        /* Try to read the source of the interrupt */
        if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&

drivers/gpu/drm/i915/intel_drv.h +4 -1

index ab811e5..23030a1 100644
@@ -650,6 +650,10 @@ struct intel_dp {
 
        /* Displayport compliance testing */
        unsigned long compliance_test_type;
+       unsigned long compliance_test_data;
+       bool compliance_test_active;
+       bool compliance_edid_invalid;
+       uint8_t compliance_edid_checksum;
 };
 
 struct intel_digital_port {
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-04-10 14:42 GMT-03:00 Todd Previte <[email protected]>:

Quoted 81 lines

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 <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c  | 46 ++++++++++++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_drv.h |  4 ++++
  2 files changed, 50 insertions(+)

drivers/gpu/drm/i915/intel_dp.c +13 -0

index ba2da44..3bfdc40 100644
@@ -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)
+                */

drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3791

Quoted 6 lines

drivers/gpu/drm/i915/intel_dp.c +6 -0

+               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)

Quoted 65 lines

drivers/gpu/drm/i915/intel_dp.c +27 -0

+       } 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;
+
         return test_result;
  }
 

@@ -3785,7 +3820,10 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
         uint8_t rxdata = 0;
         int status = 0;
 
+       intel_dp->compliance_test_active = 0;
         intel_dp->compliance_test_type = 0;
+       intel_dp->compliance_test_data = 0;
+
         intel_dp->aux.i2c_nack_count = 0;
         intel_dp->aux.i2c_defer_count = 0;
 

@@ -3920,11 +3958,19 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
         /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
          * 4.2.2.1 - EDID read required for all HPD events
+        * Also used for 4.2.2.3, 4.2.2.4 4.2.2.5 and 4.2.2.6
+        *   - compliance_edid_invalid flag used for 4.2.2.6
+        *   - compliance_edid_checksum used for 4.2.2.3
           */
+       intel_dp->compliance_edid_invalid = 0;
          edid_read = drm_get_edid(connector, adapter);
          if (!edid_read || connector->edid_header_corrupt == 1) {
                  DRM_DEBUG_DRIVER("Invalid EDID detected\n");
+                intel_dp->compliance_edid_invalid = 1;
+               intel_dp->compliance_edid_checksum = 0;
          }
+       else
+               intel_dp->compliance_edid_checksum = edid_read->checksum;
 
         /* Try to read the source of the interrupt */
         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&

drivers/gpu/drm/i915/intel_drv.h +4 -0

index ab811e5..23030a1 100644
@@ -650,6 +650,10 @@ struct intel_dp {
 
         /* Displayport compliance testing */
         unsigned long compliance_test_type;
+       unsigned long compliance_test_data;
+       bool compliance_test_active;
+       bool compliance_edid_invalid;
+       uint8_t compliance_edid_checksum;
  };
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_drv.h +0 -1

  struct intel_digital_port {
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx