[PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing

[PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing

[Todd Previte]

Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting
a corrupt EDID. To do this, the test sets up an invalid EDID header to be read by the source
device. Unfortunately, the DRM EDID reading and parsing functions are actually too good in
this case and prevent the source from reading the corrupted EDID. The result is a failed
compliance test.

In order to successfully pass the test, the raw EDID header must be checked on each read
to see if has been "corrupted". If an invalid raw header is detected, a flag is set that
allows the compliance testing code to acknowledge that fact and react appropriately. The
flag is automatically cleared on read.

This code is designed to expressly work for compliance testing without disrupting normal
operations for EDID reading and parsing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 include/drm/drm_edid.h           |  5 +++++
 4 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 53bc7a6..3d4f473 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -990,6 +990,32 @@ static const u8 edid_header[] = {
 	0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
 };
 
+
+/* Flag for EDID corruption testing
+ * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+ */
+static bool raw_edid_header_corrupted;
+
+/**
+ * drm_raw_edid_header_valid - check to see if the raw header is
+ * corrupt or not. Used solely for Displayport compliance
+ * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
+ * @raw_edid: pointer to raw base EDID block
+ *
+ * Indicates whether the original EDID header as read from the
+ * device was corrupt or not. Clears on read.
+ *
+ * Return: true if the raw header was corrupt, otherwise false
+ */
+bool drm_raw_edid_header_corrupt(void)
+{
+	bool corrupted = raw_edid_header_corrupted;
+
+	raw_edid_header_corrupted = 0;
+	return corrupted;
+}
+EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
+
 /**
  * drm_edid_header_is_valid - sanity check the header of the base EDID block
  * @raw_edid: pointer to raw base EDID block
@@ -1006,6 +1032,13 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
 		if (raw_edid[i] == edid_header[i])
 			score++;
 
+	if (score != 8) {
+		/* Log and set flag here for EDID corruption testing
+		 * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+		 */
+		DRM_DEBUG_DRIVER("Raw EDID header invalid\n");
+		raw_edid_header_corrupted = 1;
+	}
 	return score;
 }
 EXPORT_SYMBOL(drm_edid_header_is_valid);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index dc87276..57f8e43 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3824,6 +3824,9 @@ update_status:
 				   &response, 1);
 	if (status <= 0)
 		DRM_DEBUG_KMS("Could not write test response to sink\n");
+
+	/* Clear flag here, after testing is complete*/
+	intel_dp->compliance_edid_invalid = 0;
 }
 
 static int
@@ -3896,6 +3899,10 @@ 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];
 
@@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		return;
 	}
 
+	/* Compliance testing requires an EDID read for all HPD events
+	 * Link CTS Core 1.2 rev 1.1: Test 4.2.2.1
+	 * Flag set here will be handled in the EDID test function
+	 */
+	edid_read = drm_get_edid(connector, adapter);
+	if (!edid_read || drm_raw_edid_header_corrupt() == 1) {
+		DRM_DEBUG_DRIVER("EDID invalid, setting flag\n");
+		intel_dp->compliance_edid_invalid = 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)) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e7b62be..42e4251 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -651,6 +651,7 @@ struct intel_dp {
 	/* Displayport compliance testing */
 	unsigned long compliance_test_type;
 	bool compliance_testing_active;
+	bool compliance_edid_invalid;
 };
 
 struct intel_digital_port {
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 87d85e8..8a7eb22 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 			      size_t len),
 	void *data);
 
+/* Check for corruption in raw EDID header - Displayport compliance
+  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+ */
+bool drm_raw_edid_header_corrupt(void);
+
 #endif /* __DRM_EDID_H__ */
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing

[Paulo Zanoni]

2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting
> a corrupt EDID. To do this, the test sets up an invalid EDID header to be read by the source
> device. Unfortunately, the DRM EDID reading and parsing functions are actually too good in
> this case and prevent the source from reading the corrupted EDID. The result is a failed
> compliance test.
>
> In order to successfully pass the test, the raw EDID header must be checked on each read
> to see if has been "corrupted". If an invalid raw header is detected, a flag is set that
> allows the compliance testing code to acknowledge that fact and react appropriately. The
> flag is automatically cleared on read.
>
> This code is designed to expressly work for compliance testing without disrupting normal
> operations for EDID reading and parsing.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  include/drm/drm_edid.h           |  5 +++++
>  4 files changed, 56 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 53bc7a6..3d4f473 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -990,6 +990,32 @@ static const u8 edid_header[] = {
>         0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
>  };
>
> +
> +/* Flag for EDID corruption testing
> + * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
> + */
> +static bool raw_edid_header_corrupted;

A static variable like this is not a good design, especially for a
module like drm.ko. If you really need this, please store it inside
some struct. But see below first.


> +
> +/**
> + * drm_raw_edid_header_valid - check to see if the raw header is
> + * corrupt or not. Used solely for Displayport compliance
> + * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
> + * @raw_edid: pointer to raw base EDID block
> + *
> + * Indicates whether the original EDID header as read from the
> + * device was corrupt or not. Clears on read.
> + *
> + * Return: true if the raw header was corrupt, otherwise false
> + */
> +bool drm_raw_edid_header_corrupt(void)
> +{
> +       bool corrupted = raw_edid_header_corrupted;
> +
> +       raw_edid_header_corrupted = 0;
> +       return corrupted;
> +}
> +EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
> +
>  /**
>   * drm_edid_header_is_valid - sanity check the header of the base EDID block
>   * @raw_edid: pointer to raw base EDID block
> @@ -1006,6 +1032,13 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
>                 if (raw_edid[i] == edid_header[i])
>                         score++;
>
> +       if (score != 8) {
> +               /* Log and set flag here for EDID corruption testing
> +                * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
> +                */
> +               DRM_DEBUG_DRIVER("Raw EDID header invalid\n");
> +               raw_edid_header_corrupted = 1;
> +       }

The problem is that here we're limiting ourselves to just a bad edid
header, not a bad edid in general, so there are many things which we
might not get - such as a simple wrong checksum edid value. I remember
that on the previous patch you calculated the whole checksum manually,
but I don't see that code anymore. What was the reason for the change?

Also, while reviewing the patch I just discovered
connector->bad_edid_counter. Can't we just use it instead of this
patch? I mean: grab the current counter, check edid, see if the
counter moved.

>         return score;
>  }
>  EXPORT_SYMBOL(drm_edid_header_is_valid);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index dc87276..57f8e43 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3824,6 +3824,9 @@ update_status:
>                                    &response, 1);
>         if (status <= 0)
>                 DRM_DEBUG_KMS("Could not write test response to sink\n");
> +
> +       /* Clear flag here, after testing is complete*/
> +       intel_dp->compliance_edid_invalid = 0;
>  }
>
>  static int
> @@ -3896,6 +3899,10 @@ 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];
>
> @@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>                 return;
>         }
>
> +       /* Compliance testing requires an EDID read for all HPD events
> +        * Link CTS Core 1.2 rev 1.1: Test 4.2.2.1
> +        * Flag set here will be handled in the EDID test function
> +        */
> +       edid_read = drm_get_edid(connector, adapter);
> +       if (!edid_read || drm_raw_edid_header_corrupt() == 1) {
> +               DRM_DEBUG_DRIVER("EDID invalid, setting flag\n");
> +               intel_dp->compliance_edid_invalid = 1;
> +       }

I see that on the next patch you also add a drm_get_edid() call, so we
have apparently added 2 calls for the edid test. Do we really need
both? Why is this one needed? Why is that one needed?

Also, some more ideas:

I also thought that we already automatically issued get_edid() calls
on the normal hotplug code path, so it would be a "third" call on the
codepath for the test. Can't we just rely on this one?

Another idea would be: instead of getting the edid from inside the
Kernel, we could try to get it from the user-space, using the
GetResources/GetConnector IOCTLs, and also maybe look at the EDID
properties to possibly validate the EDID (in case that edid did not
get "fixed" by the Kernel). The nice thing about this is that it would
make the test be more like a real driver usage. Do you see any
possible problems with this approach?

> +
>         /* 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)) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e7b62be..42e4251 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -651,6 +651,7 @@ struct intel_dp {
>         /* Displayport compliance testing */
>         unsigned long compliance_test_type;
>         bool compliance_testing_active;
> +       bool compliance_edid_invalid;
>  };
>
>  struct intel_digital_port {
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 87d85e8..8a7eb22 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>                               size_t len),
>         void *data);
>
> +/* Check for corruption in raw EDID header - Displayport compliance
> +  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
> + */
> +bool drm_raw_edid_header_corrupt(void);
> +
>  #endif /* __DRM_EDID_H__ */
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing

[Todd Previte]

On 4/8/2015 9:51 AM, Paulo Zanoni wrote:
> 2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting
>> a corrupt EDID. To do this, the test sets up an invalid EDID header to be read by the source
>> device. Unfortunately, the DRM EDID reading and parsing functions are actually too good in
>> this case and prevent the source from reading the corrupted EDID. The result is a failed
>> compliance test.
>>
>> In order to successfully pass the test, the raw EDID header must be checked on each read
>> to see if has been "corrupted". If an invalid raw header is detected, a flag is set that
>> allows the compliance testing code to acknowledge that fact and react appropriately. The
>> flag is automatically cleared on read.
>>
>> This code is designed to expressly work for compliance testing without disrupting normal
>> operations for EDID reading and parsing.
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h |  1 +
>>   include/drm/drm_edid.h           |  5 +++++
>>   4 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 53bc7a6..3d4f473 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -990,6 +990,32 @@ static const u8 edid_header[] = {
>>          0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
>>   };
>>
>> +
>> +/* Flag for EDID corruption testing
>> + * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>> + */
>> +static bool raw_edid_header_corrupted;
> A static variable like this is not a good design, especially for a
> module like drm.ko. If you really need this, please store it inside
> some struct. But see below first.
Per our discussion this morning, I concur. This has been removed in 
favor of a different solution that uses a new boolean flag in the 
drm_connector struct.

Capturing more of the discussion here, the static boolean was a bad idea 
to begin with and needed to be removed. One solution was to make the 
flag non-static and non-clear-on-read, then add a separate clear() 
function. But it still had the problem of potential misuse other places 
in the code. The current solution (which will be posted with V5) 
modifies the is_valid() function and adds a flag in the drm_connector 
struct that can be used to detect this low-level header corruption.

>
>> +
>> +/**
>> + * drm_raw_edid_header_valid - check to see if the raw header is
>> + * corrupt or not. Used solely for Displayport compliance
>> + * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
>> + * @raw_edid: pointer to raw base EDID block
>> + *
>> + * Indicates whether the original EDID header as read from the
>> + * device was corrupt or not. Clears on read.
>> + *
>> + * Return: true if the raw header was corrupt, otherwise false
>> + */
>> +bool drm_raw_edid_header_corrupt(void)
>> +{
>> +       bool corrupted = raw_edid_header_corrupted;
>> +
>> +       raw_edid_header_corrupted = 0;
>> +       return corrupted;
>> +}
>> +EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
>> +
>>   /**
>>    * drm_edid_header_is_valid - sanity check the header of the base EDID block
>>    * @raw_edid: pointer to raw base EDID block
>> @@ -1006,6 +1032,13 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
>>                  if (raw_edid[i] == edid_header[i])
>>                          score++;
>>
>> +       if (score != 8) {
>> +               /* Log and set flag here for EDID corruption testing
>> +                * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>> +                */
>> +               DRM_DEBUG_DRIVER("Raw EDID header invalid\n");
>> +               raw_edid_header_corrupted = 1;
>> +       }
> The problem is that here we're limiting ourselves to just a bad edid
> header, not a bad edid in general, so there are many things which we
> might not get - such as a simple wrong checksum edid value. I remember
> that on the previous patch you calculated the whole checksum manually,
> but I don't see that code anymore. What was the reason for the change?
So this code is specifically for the 4.2.2.6 compliance test that is 
looking for nothing more than an invalid EDID header. In fact, the test 
unit only sets that header as invalid once, so if you miss it on the 
first read, you can't go back and check it again later - the test will 
now fail. So catching the general case isn't really what this is about - 
it's about being able to detect a corrupt EDID header even if it only 
happens once.

Honestly, the DRM EDID code is VERY good about catching corruption cases 
and in the case of corrupted headers, fixing them and moving on. I had 
to tie into it at a fairly low level in order to catch the invalid 
header before the code fixed it.

With respect to the checksum code, for quite a while the checksum 
computation was incorrect in the DRM code. Somewhere along in November 
of last year or 2013 (I remember the month, not the year, go figure) 
someone came along and added a checksum computation that actually 
worked. So that rendered that original code I wrote unnecessary.

> Also, while reviewing the patch I just discovered
> connector->bad_edid_counter. Can't we just use it instead of this
> patch? I mean: grab the current counter, check edid, see if the
> counter moved.
I think the above description highlights why using this counter really 
isn't an option. Since the code only gets one shot at catching that 
invalid header, it's essential to make sure it's captured specifically. 
Comparing before and after values of this counter doesn't specifically 
say that the header was invalid, only that SOMEthing in the EDID was 
invalid.
>>          return score;
>>   }
>>   EXPORT_SYMBOL(drm_edid_header_is_valid);
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index dc87276..57f8e43 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3824,6 +3824,9 @@ update_status:
>>                                     &response, 1);
>>          if (status <= 0)
>>                  DRM_DEBUG_KMS("Could not write test response to sink\n");
>> +
>> +       /* Clear flag here, after testing is complete*/
>> +       intel_dp->compliance_edid_invalid = 0;
>>   }
>>
>>   static int
>> @@ -3896,6 +3899,10 @@ 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];
>>
>> @@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>                  return;
>>          }
>>
>> +       /* Compliance testing requires an EDID read for all HPD events
>> +        * Link CTS Core 1.2 rev 1.1: Test 4.2.2.1
>> +        * Flag set here will be handled in the EDID test function
>> +        */
>> +       edid_read = drm_get_edid(connector, adapter);
>> +       if (!edid_read || drm_raw_edid_header_corrupt() == 1) {
>> +               DRM_DEBUG_DRIVER("EDID invalid, setting flag\n");
>> +               intel_dp->compliance_edid_invalid = 1;
>> +       }
> I see that on the next patch you also add a drm_get_edid() call, so we
> have apparently added 2 calls for the edid test. Do we really need
> both? Why is this one needed? Why is that one needed?
So there's two issues here - first is the same one mentioned above, 
catching that single instance of a corrupted EDID header. The second is 
that the checksum from the test device differs between the two reads. If 
you remove either one of them, one test or the other will fail.

> Also, some more ideas:
>
> I also thought that we already automatically issued get_edid() calls
> on the normal hotplug code path, so it would be a "third" call on the
> codepath for the test. Can't we just rely on this one?
Same issue as above.
>
> Another idea would be: instead of getting the edid from inside the
> Kernel, we could try to get it from the user-space, using the
> GetResources/GetConnector IOCTLs, and also maybe look at the EDID
> properties to possibly validate the EDID (in case that edid did not
> get "fixed" by the Kernel). The nice thing about this is that it would
> make the test be more like a real driver usage. Do you see any
> possible problems with this approach?
I don't really see this as a valid option in light of the descriptions 
I've given above. This has a good chance of introducing latency problems 
which may adversely affect the tests as well.

>> +
>>          /* 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)) {
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index e7b62be..42e4251 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -651,6 +651,7 @@ struct intel_dp {
>>          /* Displayport compliance testing */
>>          unsigned long compliance_test_type;
>>          bool compliance_testing_active;
>> +       bool compliance_edid_invalid;
>>   };
>>
>>   struct intel_digital_port {
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 87d85e8..8a7eb22 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>>                                size_t len),
>>          void *data);
>>
>> +/* Check for corruption in raw EDID header - Displayport compliance
>> +  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>> + */
>> +bool drm_raw_edid_header_corrupt(void);
>> +
>>   #endif /* __DRM_EDID_H__ */
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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

Re: [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing

[Paulo Zanoni]

2015-04-08 18:43 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>
>
> On 4/8/2015 9:51 AM, Paulo Zanoni wrote:
>>
>> 2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>>
>>> Displayport compliance test 4.2.2.6 requires that a source device be
>>> capable of detecting
>>> a corrupt EDID. To do this, the test sets up an invalid EDID header to be
>>> read by the source
>>> device. Unfortunately, the DRM EDID reading and parsing functions are
>>> actually too good in
>>> this case and prevent the source from reading the corrupted EDID. The
>>> result is a failed
>>> compliance test.
>>>
>>> In order to successfully pass the test, the raw EDID header must be
>>> checked on each read
>>> to see if has been "corrupted". If an invalid raw header is detected, a
>>> flag is set that
>>> allows the compliance testing code to acknowledge that fact and react
>>> appropriately. The
>>> flag is automatically cleared on read.
>>>
>>> This code is designed to expressly work for compliance testing without
>>> disrupting normal
>>> operations for EDID reading and parsing.
>>>
>>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>>> Cc: dri-devel@lists.freedesktop.org
>>> ---
>>>   drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_drv.h |  1 +
>>>   include/drm/drm_edid.h           |  5 +++++
>>>   4 files changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index 53bc7a6..3d4f473 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -990,6 +990,32 @@ static const u8 edid_header[] = {
>>>          0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
>>>   };
>>>
>>> +
>>> +/* Flag for EDID corruption testing
>>> + * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>>> + */
>>> +static bool raw_edid_header_corrupted;
>>
>> A static variable like this is not a good design, especially for a
>> module like drm.ko. If you really need this, please store it inside
>> some struct. But see below first.
>
> Per our discussion this morning, I concur. This has been removed in favor of
> a different solution that uses a new boolean flag in the drm_connector
> struct.
>
> Capturing more of the discussion here, the static boolean was a bad idea to
> begin with and needed to be removed. One solution was to make the flag
> non-static and non-clear-on-read, then add a separate clear() function. But
> it still had the problem of potential misuse other places in the code. The
> current solution (which will be posted with V5) modifies the is_valid()
> function and adds a flag in the drm_connector struct that can be used to
> detect this low-level header corruption.
>
>
>>
>>> +
>>> +/**
>>> + * drm_raw_edid_header_valid - check to see if the raw header is
>>> + * corrupt or not. Used solely for Displayport compliance
>>> + * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
>>> + * @raw_edid: pointer to raw base EDID block
>>> + *
>>> + * Indicates whether the original EDID header as read from the
>>> + * device was corrupt or not. Clears on read.
>>> + *
>>> + * Return: true if the raw header was corrupt, otherwise false
>>> + */
>>> +bool drm_raw_edid_header_corrupt(void)
>>> +{
>>> +       bool corrupted = raw_edid_header_corrupted;
>>> +
>>> +       raw_edid_header_corrupted = 0;
>>> +       return corrupted;
>>> +}
>>> +EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
>>> +
>>>   /**
>>>    * drm_edid_header_is_valid - sanity check the header of the base EDID
>>> block
>>>    * @raw_edid: pointer to raw base EDID block
>>> @@ -1006,6 +1032,13 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
>>>                  if (raw_edid[i] == edid_header[i])
>>>                          score++;
>>>
>>> +       if (score != 8) {
>>> +               /* Log and set flag here for EDID corruption testing
>>> +                * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>>> +                */
>>> +               DRM_DEBUG_DRIVER("Raw EDID header invalid\n");
>>> +               raw_edid_header_corrupted = 1;
>>> +       }
>>
>> The problem is that here we're limiting ourselves to just a bad edid
>> header, not a bad edid in general, so there are many things which we
>> might not get - such as a simple wrong checksum edid value. I remember
>> that on the previous patch you calculated the whole checksum manually,
>> but I don't see that code anymore. What was the reason for the change?
>
> So this code is specifically for the 4.2.2.6 compliance test that is looking
> for nothing more than an invalid EDID header.

On the version of the spec I have (1.2 Core, Aug 22 2011), 4.2.2.6 is
"EDID Corruption Detection", and it mentions "EDID corruption" without
really getting into the details of header corruption. On the "Test
procedure" description, it mentions "Reference Sink sets up EDID with
incorrect checksum", which we don't check. Of course, changing the
header may produce an incorrect checksum, but maybe the wrong header
is just a particular detail of the compliance testing device you have,
while others could potentially have other forms of corruption, such as
just a bad checksum?

In the paragraphs below you elaborate even more on the assumption of a
bad header instead of just a bad checksum, so maybe we have different
versions of the spec? (I still remember when I used version 1.0 of a
certain non-backwards-compatible spec to review a patch made against
version 0.8 of the same spec)

> In fact, the test unit only
> sets that header as invalid once, so if you miss it on the first read, you
> can't go back and check it again later - the test will now fail. So catching
> the general case isn't really what this is about - it's about being able to
> detect a corrupt EDID header even if it only happens once.
>
> Honestly, the DRM EDID code is VERY good about catching corruption cases and
> in the case of corrupted headers, fixing them and moving on. I had to tie
> into it at a fairly low level in order to catch the invalid header before
> the code fixed it.
>
> With respect to the checksum code, for quite a while the checksum
> computation was incorrect in the DRM code. Somewhere along in November of
> last year or 2013 (I remember the month, not the year, go figure) someone
> came along and added a checksum computation that actually worked. So that
> rendered that original code I wrote unnecessary.
>
>> Also, while reviewing the patch I just discovered
>> connector->bad_edid_counter. Can't we just use it instead of this
>> patch? I mean: grab the current counter, check edid, see if the
>> counter moved.
>
> I think the above description highlights why using this counter really isn't
> an option. Since the code only gets one shot at catching that invalid
> header, it's essential to make sure it's captured specifically. Comparing
> before and after values of this counter doesn't specifically say that the
> header was invalid, only that SOMEthing in the EDID was invalid.

Which is, according to the way I read the spec, not a problem.


>
>>>          return score;
>>>   }
>>>   EXPORT_SYMBOL(drm_edid_header_is_valid);
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index dc87276..57f8e43 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -3824,6 +3824,9 @@ update_status:
>>>                                     &response, 1);
>>>          if (status <= 0)
>>>                  DRM_DEBUG_KMS("Could not write test response to
>>> sink\n");
>>> +
>>> +       /* Clear flag here, after testing is complete*/
>>> +       intel_dp->compliance_edid_invalid = 0;
>>>   }
>>>
>>>   static int
>>> @@ -3896,6 +3899,10 @@ 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];
>>>
>>> @@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp
>>> *intel_dp)
>>>                  return;
>>>          }
>>>
>>> +       /* Compliance testing requires an EDID read for all HPD events
>>> +        * Link CTS Core 1.2 rev 1.1: Test 4.2.2.1
>>> +        * Flag set here will be handled in the EDID test function
>>> +        */
>>> +       edid_read = drm_get_edid(connector, adapter);
>>> +       if (!edid_read || drm_raw_edid_header_corrupt() == 1) {
>>> +               DRM_DEBUG_DRIVER("EDID invalid, setting flag\n");
>>> +               intel_dp->compliance_edid_invalid = 1;
>>> +       }
>>
>> I see that on the next patch you also add a drm_get_edid() call, so we
>> have apparently added 2 calls for the edid test. Do we really need
>> both? Why is this one needed? Why is that one needed?
>
> So there's two issues here - first is the same one mentioned above, catching
> that single instance of a corrupted EDID header. The second is that the
> checksum from the test device differs between the two reads. If you remove
> either one of them, one test or the other will fail.

But then why not keep both at the same place? The one here is going to
affect a lot more than just compliance testing, while the other is
contained to DP compliance code.


>
>> Also, some more ideas:
>>
>> I also thought that we already automatically issued get_edid() calls
>> on the normal hotplug code path, so it would be a "third" call on the
>> codepath for the test. Can't we just rely on this one?
>
> Same issue as above.
>>
>>
>> Another idea would be: instead of getting the edid from inside the
>> Kernel, we could try to get it from the user-space, using the
>> GetResources/GetConnector IOCTLs, and also maybe look at the EDID
>> properties to possibly validate the EDID (in case that edid did not
>> get "fixed" by the Kernel). The nice thing about this is that it would
>> make the test be more like a real driver usage. Do you see any
>> possible problems with this approach?
>
> I don't really see this as a valid option in light of the descriptions I've
> given above. This has a good chance of introducing latency problems which
> may adversely affect the tests as well.

We have 5 seconds, that's way more than enough.

>
>
>>> +
>>>          /* 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)) {
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>> b/drivers/gpu/drm/i915/intel_drv.h
>>> index e7b62be..42e4251 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -651,6 +651,7 @@ struct intel_dp {
>>>          /* Displayport compliance testing */
>>>          unsigned long compliance_test_type;
>>>          bool compliance_testing_active;
>>> +       bool compliance_edid_invalid;
>>>   };
>>>
>>>   struct intel_digital_port {
>>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>> index 87d85e8..8a7eb22 100644
>>> --- a/include/drm/drm_edid.h
>>> +++ b/include/drm/drm_edid.h
>>> @@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector
>>> *connector,
>>>                                size_t len),
>>>          void *data);
>>>
>>> +/* Check for corruption in raw EDID header - Displayport compliance
>>> +  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>>> + */
>>> +bool drm_raw_edid_header_corrupt(void);
>>> +
>>>   #endif /* __DRM_EDID_H__ */
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>



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

Re: [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing

[Todd Previte]

On 4/8/2015 3:37 PM, Paulo Zanoni wrote:
> 2015-04-08 18:43 GMT-03:00 Todd Previte<tprevite@gmail.com>:
>> On 4/8/2015 9:51 AM, Paulo Zanoni wrote:
>>> 2015-03-31 14:15 GMT-03:00 Todd Previte<tprevite@gmail.com>:
>>>> Displayport compliance test 4.2.2.6 requires that a source device be
>>>> capable of detecting
>>>> a corrupt EDID. To do this, the test sets up an invalid EDID header to be
>>>> read by the source
>>>> device. Unfortunately, the DRM EDID reading and parsing functions are
>>>> actually too good in
>>>> this case and prevent the source from reading the corrupted EDID. The
>>>> result is a failed
>>>> compliance test.
>>>>
>>>> In order to successfully pass the test, the raw EDID header must be
>>>> checked on each read
>>>> to see if has been "corrupted". If an invalid raw header is detected, a
>>>> flag is set that
>>>> allows the compliance testing code to acknowledge that fact and react
>>>> appropriately. The
>>>> flag is automatically cleared on read.
>>>>
>>>> This code is designed to expressly work for compliance testing without
>>>> disrupting normal
>>>> operations for EDID reading and parsing.
>>>>
>>>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>>>> Cc:dri-devel@lists.freedesktop.org
>>>> ---
>>>>    drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
>>>>    drivers/gpu/drm/i915/intel_drv.h |  1 +
>>>>    include/drm/drm_edid.h           |  5 +++++
>>>>    4 files changed, 56 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>> index 53bc7a6..3d4f473 100644
>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>> @@ -990,6 +990,32 @@ static const u8 edid_header[] = {
>>>>           0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
>>>>    };
>>>>
>>>> +
>>>> +/* Flag for EDID corruption testing
>>>> + * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>>>> + */
>>>> +static bool raw_edid_header_corrupted;
>>> A static variable like this is not a good design, especially for a
>>> module like drm.ko. If you really need this, please store it inside
>>> some struct. But see below first.
>> Per our discussion this morning, I concur. This has been removed in favor of
>> a different solution that uses a new boolean flag in the drm_connector
>> struct.
>>
>> Capturing more of the discussion here, the static boolean was a bad idea to
>> begin with and needed to be removed. One solution was to make the flag
>> non-static and non-clear-on-read, then add a separate clear() function. But
>> it still had the problem of potential misuse other places in the code. The
>> current solution (which will be posted with V5) modifies the is_valid()
>> function and adds a flag in the drm_connector struct that can be used to
>> detect this low-level header corruption.
>>
>>
>>>> +
>>>> +/**
>>>> + * drm_raw_edid_header_valid - check to see if the raw header is
>>>> + * corrupt or not. Used solely for Displayport compliance
>>>> + * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
>>>> + * @raw_edid: pointer to raw base EDID block
>>>> + *
>>>> + * Indicates whether the original EDID header as read from the
>>>> + * device was corrupt or not. Clears on read.
>>>> + *
>>>> + * Return: true if the raw header was corrupt, otherwise false
>>>> + */
>>>> +bool drm_raw_edid_header_corrupt(void)
>>>> +{
>>>> +       bool corrupted = raw_edid_header_corrupted;
>>>> +
>>>> +       raw_edid_header_corrupted = 0;
>>>> +       return corrupted;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
>>>> +
>>>>    /**
>>>>     * drm_edid_header_is_valid - sanity check the header of the base EDID
>>>> block
>>>>     * @raw_edid: pointer to raw base EDID block
>>>> @@ -1006,6 +1032,13 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
>>>>                   if (raw_edid[i] == edid_header[i])
>>>>                           score++;
>>>>
>>>> +       if (score != 8) {
>>>> +               /* Log and set flag here for EDID corruption testing
>>>> +                * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>>>> +                */
>>>> +               DRM_DEBUG_DRIVER("Raw EDID header invalid\n");
>>>> +               raw_edid_header_corrupted = 1;
>>>> +       }
>>> The problem is that here we're limiting ourselves to just a bad edid
>>> header, not a bad edid in general, so there are many things which we
>>> might not get - such as a simple wrong checksum edid value. I remember
>>> that on the previous patch you calculated the whole checksum manually,
>>> but I don't see that code anymore. What was the reason for the change?
>> So this code is specifically for the 4.2.2.6 compliance test that is looking
>> for nothing more than an invalid EDID header.
> On the version of the spec I have (1.2 Core, Aug 22 2011), 4.2.2.6 is
> "EDID Corruption Detection", and it mentions "EDID corruption" without
> really getting into the details of header corruption. On the "Test
> procedure" description, it mentions "Reference Sink sets up EDID with
> incorrect checksum", which we don't check. Of course, changing the
> header may produce an incorrect checksum, but maybe the wrong header
> is just a particular detail of the compliance testing device you have,
> while others could potentially have other forms of corruption, such as
> just a bad checksum?
It could very well be particular this unit. So with a different test 
device, we might be able to get away with just checking the checksum. 
For this one, however, we don't appear to have that option. I added the 
checksum computation into the header fixup code just to make sure.

> In the paragraphs below you elaborate even more on the assumption of a
> bad header instead of just a bad checksum, so maybe we have different
> versions of the spec? (I still remember when I used version 1.0 of a
> certain non-backwards-compatible spec to review a patch made against
> version 0.8 of the same spec)
I do have a later version of the spec, but description of this test 
seems to be the same between the two.
>> In fact, the test unit only
>> sets that header as invalid once, so if you miss it on the first read, you
>> can't go back and check it again later - the test will now fail. So catching
>> the general case isn't really what this is about - it's about being able to
>> detect a corrupt EDID header even if it only happens once.
>>
>> Honestly, the DRM EDID code is VERY good about catching corruption cases and
>> in the case of corrupted headers, fixing them and moving on. I had to tie
>> into it at a fairly low level in order to catch the invalid header before
>> the code fixed it.
>>
>> With respect to the checksum code, for quite a while the checksum
>> computation was incorrect in the DRM code. Somewhere along in November of
>> last year or 2013 (I remember the month, not the year, go figure) someone
>> came along and added a checksum computation that actually worked. So that
>> rendered that original code I wrote unnecessary.
>>
>>> Also, while reviewing the patch I just discovered
>>> connector->bad_edid_counter. Can't we just use it instead of this
>>> patch? I mean: grab the current counter, check edid, see if the
>>> counter moved.
>> I think the above description highlights why using this counter really isn't
>> an option. Since the code only gets one shot at catching that invalid
>> header, it's essential to make sure it's captured specifically. Comparing
>> before and after values of this counter doesn't specifically say that the
>> header was invalid, only that SOMEthing in the EDID was invalid.
> Which is, according to the way I read the spec, not a problem.
I completely agree with you. Unfortunately, coding directly to the spec 
isn't enough in this case.

>
>>>>           return score;
>>>>    }
>>>>    EXPORT_SYMBOL(drm_edid_header_is_valid);
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>> index dc87276..57f8e43 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -3824,6 +3824,9 @@ update_status:
>>>>                                      &response, 1);
>>>>           if (status <= 0)
>>>>                   DRM_DEBUG_KMS("Could not write test response to
>>>> sink\n");
>>>> +
>>>> +       /* Clear flag here, after testing is complete*/
>>>> +       intel_dp->compliance_edid_invalid = 0;
>>>>    }
>>>>
>>>>    static int
>>>> @@ -3896,6 +3899,10 @@ 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];
>>>>
>>>> @@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp
>>>> *intel_dp)
>>>>                   return;
>>>>           }
>>>>
>>>> +       /* Compliance testing requires an EDID read for all HPD events
>>>> +        * Link CTS Core 1.2 rev 1.1: Test 4.2.2.1
>>>> +        * Flag set here will be handled in the EDID test function
>>>> +        */
>>>> +       edid_read = drm_get_edid(connector, adapter);
>>>> +       if (!edid_read || drm_raw_edid_header_corrupt() == 1) {
>>>> +               DRM_DEBUG_DRIVER("EDID invalid, setting flag\n");
>>>> +               intel_dp->compliance_edid_invalid = 1;
>>>> +       }
>>> I see that on the next patch you also add a drm_get_edid() call, so we
>>> have apparently added 2 calls for the edid test. Do we really need
>>> both? Why is this one needed? Why is that one needed?
>> So there's two issues here - first is the same one mentioned above, catching
>> that single instance of a corrupted EDID header. The second is that the
>> checksum from the test device differs between the two reads. If you remove
>> either one of them, one test or the other will fail.
> But then why not keep both at the same place? The one here is going to
> affect a lot more than just compliance testing, while the other is
> contained to DP compliance code.
I was able to find a solution that removed the duplicate EDID read. I 
had to add a checksum storage variable in the intel_dp struct, but 
that's infinitely better than having another EDID read.

Unfortunately though, the one that has to say is in the 
check_link_status. There's just no way around it because of the test 
4.2.2.1 that requires it to happen for a hot plug event. There's no test 
request bit set for that, or any other indicator. It simply has to 
happen for every HPD plug event.

>>> Also, some more ideas:
>>>
>>> I also thought that we already automatically issued get_edid() calls
>>> on the normal hotplug code path, so it would be a "third" call on the
>>> codepath for the test. Can't we just rely on this one?
>> Same issue as above.
>>> Another idea would be: instead of getting the edid from inside the
>>> Kernel, we could try to get it from the user-space, using the
>>> GetResources/GetConnector IOCTLs, and also maybe look at the EDID
>>> properties to possibly validate the EDID (in case that edid did not
>>> get "fixed" by the Kernel). The nice thing about this is that it would
>>> make the test be more like a real driver usage. Do you see any
>>> possible problems with this approach?
>> I don't really see this as a valid option in light of the descriptions I've
>> given above. This has a good chance of introducing latency problems which
>> may adversely affect the tests as well.
> We have 5 seconds, that's way more than enough.
The test has a 5 second timeout for the entire operation. I'm less 
concerned with timing out and more concerned about not being able to 
catch things fast enough or react fast enough to parameter or value 
changes. It may or may not be an issue for processing the EDID (I'd lean 
more towards the not case) but it's something that has to be kept in 
mind here, as this has caused problems in the past when building out the 
test interfaces.

In any case, this sounds like this is a suggestion rather than a 
blocking issue. My main concern with moving all this stuff into 
userspace is that it's moving towards building a Displayport-compliant 
user app versus a Displayport-compliant driver. But this is something 
that I can look into sometime down the road.


>>>> +
>>>>           /* 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)) {
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>>> b/drivers/gpu/drm/i915/intel_drv.h
>>>> index e7b62be..42e4251 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -651,6 +651,7 @@ struct intel_dp {
>>>>           /* Displayport compliance testing */
>>>>           unsigned long compliance_test_type;
>>>>           bool compliance_testing_active;
>>>> +       bool compliance_edid_invalid;
>>>>    };
>>>>
>>>>    struct intel_digital_port {
>>>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>>> index 87d85e8..8a7eb22 100644
>>>> --- a/include/drm/drm_edid.h
>>>> +++ b/include/drm/drm_edid.h
>>>> @@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector
>>>> *connector,
>>>>                                 size_t len),
>>>>           void *data);
>>>>
>>>> +/* Check for corruption in raw EDID header - Displayport compliance
>>>> +  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>>>> + */
>>>> +bool drm_raw_edid_header_corrupt(void);
>>>> +
>>>>    #endif /* __DRM_EDID_H__ */
>>>> --
>>>> 1.9.1
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>

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

[PATCH 7/9] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Todd Previte]

For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
device must attempt at least 7 times to read the EDID when it receives an
I2C defer. The normal DRM code makes only 7 retries, regardless of whether
or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
since there are native defers interspersed with the I2C defers which
results in less than 7 EDID read attempts.

The solution is to decrement the retry counter when an I2C DEFER is returned
such that another read attempt will be made. This situation should normally
only occur in compliance testing, however, as a worse case real-world
scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
for a single transaction to complete. The net result is a slightly slower
response to an EDID read that shouldn't significantly impact overall
performance.

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_dp_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 79968e3..0539758 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -469,6 +469,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 		case DP_AUX_I2C_REPLY_DEFER:
 			DRM_DEBUG_KMS("I2C defer\n");
 			aux->i2c_defer_count++;
+			/* DP Compliance Test 4.2.2.5 Requirement:
+			 * Must have at least 7 retries for I2C defers on the
+			 * transaction to pass this test
+			 */
+			retry--;
 			usleep_range(400, 500);
 			continue;
 
-- 
1.9.1

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

Re: [PATCH 7/9] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Paulo Zanoni]

2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
> device must attempt at least 7 times to read the EDID when it receives an
> I2C defer. The normal DRM code makes only 7 retries, regardless of whether
> or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
> since there are native defers interspersed with the I2C defers which
> results in less than 7 EDID read attempts.
>
> The solution is to decrement the retry counter when an I2C DEFER is returned
> such that another read attempt will be made. This situation should normally
> only occur in compliance testing, however, as a worse case real-world
> scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
> for a single transaction to complete. The net result is a slightly slower
> response to an EDID read that shouldn't significantly impact overall
> performance.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 79968e3..0539758 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -469,6 +469,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>                 case DP_AUX_I2C_REPLY_DEFER:
>                         DRM_DEBUG_KMS("I2C defer\n");
>                         aux->i2c_defer_count++;
> +                       /* DP Compliance Test 4.2.2.5 Requirement:
> +                        * Must have at least 7 retries for I2C defers on the
> +                        * transaction to pass this test
> +                        */
> +                       retry--;

I wouldn't be surprised if someone discovers a monitor or some sort of
dongle that keeps sending I2C defer errors forever, keeping us in an
infinite loop. Shouldn't we count each error in separate? Or maybe
just loop up to 14 times, in case that doesn't violate any spec (I
didn't check)?


>                         usleep_range(400, 500);
>                         continue;
>
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Paulo Zanoni
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/9] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Todd Previte]

On 4/6/15 5:05 PM, Paulo Zanoni wrote:
> 2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
>> device must attempt at least 7 times to read the EDID when it receives an
>> I2C defer. The normal DRM code makes only 7 retries, regardless of whether
>> or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
>> since there are native defers interspersed with the I2C defers which
>> results in less than 7 EDID read attempts.
>>
>> The solution is to decrement the retry counter when an I2C DEFER is returned
>> such that another read attempt will be made. This situation should normally
>> only occur in compliance testing, however, as a worse case real-world
>> scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
>> for a single transaction to complete. The net result is a slightly slower
>> response to an EDID read that shouldn't significantly impact overall
>> performance.
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/drm_dp_helper.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> index 79968e3..0539758 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -469,6 +469,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>>                  case DP_AUX_I2C_REPLY_DEFER:
>>                          DRM_DEBUG_KMS("I2C defer\n");
>>                          aux->i2c_defer_count++;
>> +                       /* DP Compliance Test 4.2.2.5 Requirement:
>> +                        * Must have at least 7 retries for I2C defers on the
>> +                        * transaction to pass this test
>> +                        */
>> +                       retry--;
> I wouldn't be surprised if someone discovers a monitor or some sort of
> dongle that keeps sending I2C defer errors forever, keeping us in an
> infinite loop. Shouldn't we count each error in separate? Or maybe
> just loop up to 14 times, in case that doesn't violate any spec (I
> didn't check)?
I think the safest thing to do would be to put a failsafe on the 
i2c_defer_counter. That would ensure that the compliance test gets its 7 
retries and that if we do encounter a misbehaving device, the driver 
won't let the unending defers cause an infinite loop. Updated patch 
shortly.

>>                          usleep_range(400, 500);
>>                          continue;
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

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

[PATCH 07/11] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Todd Previte]

For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
device must attempt at least 7 times to read the EDID when it receives an
I2C defer. The normal DRM code makes only 7 retries, regardless of whether
or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
since there are native defers interspersed with the I2C defers which
results in less than 7 EDID read attempts.

The solution is to decrement the retry counter when an I2C DEFER is returned
such that another read attempt will be made. This situation should normally
only occur in compliance testing, however, as a worse case real-world
scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
for a single transaction to complete. The net result is a slightly slower
response to an EDID read that shouldn't significantly impact overall
performance.

V2:
- Added a check on the number of I2C Defers to limit the number
  of times that the retries variable will be decremented. This
  is to address review feedback regarding possible infinite loops
  from misbehaving sink devices.

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_dp_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 79968e3..23025cf 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -469,6 +469,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 		case DP_AUX_I2C_REPLY_DEFER:
 			DRM_DEBUG_KMS("I2C defer\n");
 			aux->i2c_defer_count++;
+			/* DP Compliance Test 4.2.2.5 Requirement:
+			 * Must have at least 7 retries for I2C defers on the
+			 * transaction to pass this test
+			 */
+			if (aux->i2c_defer_count < 8)
+				retry--;
 			usleep_range(400, 500);
 			continue;
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/11] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Paulo Zanoni]

2015-04-06 23:11 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
> device must attempt at least 7 times to read the EDID when it receives an
> I2C defer. The normal DRM code makes only 7 retries, regardless of whether
> or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
> since there are native defers interspersed with the I2C defers which
> results in less than 7 EDID read attempts.
>
> The solution is to decrement the retry counter when an I2C DEFER is returned
> such that another read attempt will be made. This situation should normally
> only occur in compliance testing, however, as a worse case real-world
> scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
> for a single transaction to complete. The net result is a slightly slower
> response to an EDID read that shouldn't significantly impact overall
> performance.
>
> V2:
> - Added a check on the number of I2C Defers to limit the number
>   of times that the retries variable will be decremented. This
>   is to address review feedback regarding possible infinite loops
>   from misbehaving sink devices.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 79968e3..23025cf 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -469,6 +469,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>                 case DP_AUX_I2C_REPLY_DEFER:
>                         DRM_DEBUG_KMS("I2C defer\n");
>                         aux->i2c_defer_count++;
> +                       /* DP Compliance Test 4.2.2.5 Requirement:
> +                        * Must have at least 7 retries for I2C defers on the
> +                        * transaction to pass this test
> +                        */
> +                       if (aux->i2c_defer_count < 8)

I don't think this is the way to go. During normal
(non-compliance-testing) operation we never zero i2c_defer_count, so
we can't expect this to work, since we may start drm_dp_i2c_do_msg
with a i2c_defer_count value different than zero. Also, during i915.ko
DP compliance we only zero i2c_defer_count at the very beginning of
each test, not at every aux transaction, and I really think we need a
solution that is not specific to compliance testing.


> +                               retry--;
>                         usleep_range(400, 500);
>                         continue;
>
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Paulo Zanoni
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/11] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Ville Syrjälä]

On Tue, Apr 07, 2015 at 11:29:43AM -0300, Paulo Zanoni wrote:
> 2015-04-06 23:11 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> > For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
> > device must attempt at least 7 times to read the EDID when it receives an
> > I2C defer. The normal DRM code makes only 7 retries, regardless of whether
> > or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
> > since there are native defers interspersed with the I2C defers which
> > results in less than 7 EDID read attempts.
> >
> > The solution is to decrement the retry counter when an I2C DEFER is returned
> > such that another read attempt will be made. This situation should normally
> > only occur in compliance testing, however, as a worse case real-world
> > scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
> > for a single transaction to complete. The net result is a slightly slower
> > response to an EDID read that shouldn't significantly impact overall
> > performance.
> >
> > V2:
> > - Added a check on the number of I2C Defers to limit the number
> >   of times that the retries variable will be decremented. This
> >   is to address review feedback regarding possible infinite loops
> >   from misbehaving sink devices.
> >
> > Signed-off-by: Todd Previte <tprevite@gmail.com>
> > Cc: dri-devel@lists.freedesktop.org
> > ---
> >  drivers/gpu/drm/drm_dp_helper.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 79968e3..23025cf 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -469,6 +469,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> >                 case DP_AUX_I2C_REPLY_DEFER:
> >                         DRM_DEBUG_KMS("I2C defer\n");
> >                         aux->i2c_defer_count++;
> > +                       /* DP Compliance Test 4.2.2.5 Requirement:
> > +                        * Must have at least 7 retries for I2C defers on the
> > +                        * transaction to pass this test
> > +                        */
> > +                       if (aux->i2c_defer_count < 8)
> 
> I don't think this is the way to go. During normal
> (non-compliance-testing) operation we never zero i2c_defer_count, so
> we can't expect this to work, since we may start drm_dp_i2c_do_msg
> with a i2c_defer_count value different than zero. Also, during i915.ko
> DP compliance we only zero i2c_defer_count at the very beginning of
> each test, not at every aux transaction, and I really think we need a
> solution that is not specific to compliance testing.

What I was suggesting earlier (or trying to at least) would be
simply something like this:

int defer_native = 0, defer_i2c = 0;
while (defer_native < 7 && defer_i2c < 7) {
	...
	case DP_AUX_NATIVE_REPLY_NACK:
		...
		defer_native++;
		continue;
	}
	...
	case DP_AUX_I2C_REPLY_DEFER:
		...
		defer_i2c++;
		continue;
	}
	...
}

> 
> 
> > +                               retry--;
> >                         usleep_range(400, 500);
> >                         continue;
> >
> > --
> > 1.9.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/11] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Todd Previte]

[-- Attachment #1.1: Type: text/plain, Size: 5221 bytes --]



On 4/7/15 7:29 AM, Paulo Zanoni wrote:
> 2015-04-06 23:11 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
>> device must attempt at least 7 times to read the EDID when it receives an
>> I2C defer. The normal DRM code makes only 7 retries, regardless of whether
>> or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
>> since there are native defers interspersed with the I2C defers which
>> results in less than 7 EDID read attempts.
>>
>> The solution is to decrement the retry counter when an I2C DEFER is returned
>> such that another read attempt will be made. This situation should normally
>> only occur in compliance testing, however, as a worse case real-world
>> scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
>> for a single transaction to complete. The net result is a slightly slower
>> response to an EDID read that shouldn't significantly impact overall
>> performance.
>>
>> V2:
>> - Added a check on the number of I2C Defers to limit the number
>>    of times that the retries variable will be decremented. This
>>    is to address review feedback regarding possible infinite loops
>>    from misbehaving sink devices.
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/drm_dp_helper.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> index 79968e3..23025cf 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -469,6 +469,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>>                  case DP_AUX_I2C_REPLY_DEFER:
>>                          DRM_DEBUG_KMS("I2C defer\n");
>>                          aux->i2c_defer_count++;
>> +                       /* DP Compliance Test 4.2.2.5 Requirement:
>> +                        * Must have at least 7 retries for I2C defers on the
>> +                        * transaction to pass this test
>> +                        */
>> +                       if (aux->i2c_defer_count < 8)
> I don't think this is the way to go. During normal
> (non-compliance-testing) operation we never zero i2c_defer_count, so
> we can't expect this to work, since we may start drm_dp_i2c_do_msg
> with a i2c_defer_count value different than zero. Also, during i915.ko
> DP compliance we only zero i2c_defer_count at the very beginning of
> each test, not at every aux transaction, and I really think we need a
> solution that is not specific to compliance testing.
To capture the discussion from IRC:

The primary issue previously was the potential for an infinite loop when 
decrementing the retry count. That is clearly addressed by this code, by 
only decrementing the loop while the defer count is below 8. In 
actuality, that needs to be 7, so a followup patch with that change will 
be posted shortly. There are other solutions (Ville has one in his 
reply), but this one works correctly, is minimally invasive and doesn't 
require changes to the loop structure.

The defer counter isn't used anywhere outside of Displayport compliance 
testing. In fact, the counters in the aux struct didn't even exist until 
I put them in there in a previous patch to support this exact test case. 
So there really isn't a non-DP compliance test specific solution to be 
had here. It's also unlikely that this has any significant effect on 
normal operations. In the event that a device misbehaves and begins 
issuing loads of defers, this code would only delay it by at most 7 
retries before the counter exceeded its value.

Since this value is not used outside of compliance testing, a reset 
per-transaction is unnecessary. Additionally, that would break the EDID 
compliance testing code as it would have no way of detecting that the 
read had failed due to continuous defers. As long as the counter is 
reset for each test request (which it is), this code will function 
properly for both real world and compliance operations.

As indicated in the comments, this code exists to ensure compliance with 
test 4.2.2.5 from the Displayport Link CTS Core 1.2 rev1.1. This test 
verifies that the source device is capable of responding to a failure to 
read the EDID when the sink device continuously defers the transaction. 
It ensures that at least 7 I2C defers are received before calling it 
quits, versus simply retrying the transaction 7 times regardless of the 
failure mode.

Since I'm going to post an updated patch anyways, I will likely roll the 
defer count increment into the if-statement to remove one line of code. 
That also requires moving the increment to a pre- versus post- 
condition, as follows:

     ...
     if (++aux->i2c_defer_count < 7)
         ...
     ...

>
>> +                               retry--;
>>                          usleep_range(400, 500);
>>                          continue;
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>


[-- Attachment #1.2: Type: text/html, Size: 6694 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 9/9] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg()

[Todd Previte]

The debug message is missing a newline at the end and it makes the
logs hard to read when a device defers a lot. Simple 2-character fix
adds the newline at the end.

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_dp_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 0539758..281bb67 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -433,7 +433,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 			return -EREMOTEIO;
 
 		case DP_AUX_NATIVE_REPLY_DEFER:
-			DRM_DEBUG_KMS("native defer");
+			DRM_DEBUG_KMS("native defer\n");
 			/*
 			 * We could check for I2C bit rate capabilities and if
 			 * available adjust this interval. We could also be
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 9/9] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg()

[Paulo Zanoni]

2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> The debug message is missing a newline at the end and it makes the
> logs hard to read when a device defers a lot. Simple 2-character fix
> adds the newline at the end.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> Cc: dri-devel@lists.freedesktop.org

Why in some logs there is in fact a newline, such as here:
https://bugs.freedesktop.org/attachment.cgi?id=110049 ?

Anyway, it looks correct, so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/drm_dp_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 0539758..281bb67 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -433,7 +433,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>                         return -EREMOTEIO;
>
>                 case DP_AUX_NATIVE_REPLY_DEFER:
> -                       DRM_DEBUG_KMS("native defer");
> +                       DRM_DEBUG_KMS("native defer\n");
>                         /*
>                          * We could check for I2C bit rate capabilities and if
>                          * available adjust this interval. We could also be
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing

[Todd Previte]

Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting
a corrupt EDID. To do this, the test sets up an invalid EDID header to be read by the source
device. Unfortunately, the DRM EDID reading and parsing functions are actually too good in
this case and prevent the source from reading the corrupted EDID. The result is a failed
compliance test.

In order to successfully pass the test, the raw EDID header must be checked on each read
to see if has been "corrupted". If an invalid raw header is detected, a flag is set that
allows the compliance testing code to acknowledge that fact and react appropriately. The
flag is automatically cleared on read.

This code is designed to expressly work for compliance testing without disrupting normal
operations for EDID reading and parsing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 include/drm/drm_edid.h           |  5 +++++
 4 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 53bc7a6..3d4f473 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -990,6 +990,32 @@ static const u8 edid_header[] = {
 	0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
 };
 
+
+/* Flag for EDID corruption testing
+ * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+ */
+static bool raw_edid_header_corrupted;
+
+/**
+ * drm_raw_edid_header_valid - check to see if the raw header is
+ * corrupt or not. Used solely for Displayport compliance
+ * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
+ * @raw_edid: pointer to raw base EDID block
+ *
+ * Indicates whether the original EDID header as read from the
+ * device was corrupt or not. Clears on read.
+ *
+ * Return: true if the raw header was corrupt, otherwise false
+ */
+bool drm_raw_edid_header_corrupt(void)
+{
+	bool corrupted = raw_edid_header_corrupted;
+
+	raw_edid_header_corrupted = 0;
+	return corrupted;
+}
+EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
+
 /**
  * drm_edid_header_is_valid - sanity check the header of the base EDID block
  * @raw_edid: pointer to raw base EDID block
@@ -1006,6 +1032,13 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
 		if (raw_edid[i] == edid_header[i])
 			score++;
 
+	if (score != 8) {
+		/* Log and set flag here for EDID corruption testing
+		 * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+		 */
+		DRM_DEBUG_DRIVER("Raw EDID header invalid\n");
+		raw_edid_header_corrupted = 1;
+	}
 	return score;
 }
 EXPORT_SYMBOL(drm_edid_header_is_valid);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index dc87276..57f8e43 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3824,6 +3824,9 @@ update_status:
 				   &response, 1);
 	if (status <= 0)
 		DRM_DEBUG_KMS("Could not write test response to sink\n");
+
+	/* Clear flag here, after testing is complete*/
+	intel_dp->compliance_edid_invalid = 0;
 }
 
 static int
@@ -3896,6 +3899,10 @@ 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];
 
@@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		return;
 	}
 
+	/* Compliance testing requires an EDID read for all HPD events
+	 * Link CTS Core 1.2 rev 1.1: Test 4.2.2.1
+	 * Flag set here will be handled in the EDID test function
+	 */
+	edid_read = drm_get_edid(connector, adapter);
+	if (!edid_read || drm_raw_edid_header_corrupt() == 1) {
+		DRM_DEBUG_DRIVER("EDID invalid, setting flag\n");
+		intel_dp->compliance_edid_invalid = 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)) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e7b62be..42e4251 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -651,6 +651,7 @@ struct intel_dp {
 	/* Displayport compliance testing */
 	unsigned long compliance_test_type;
 	bool compliance_testing_active;
+	bool compliance_edid_invalid;
 };
 
 struct intel_digital_port {
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 87d85e8..8a7eb22 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 			      size_t len),
 	void *data);
 
+/* Check for corruption in raw EDID header - Displayport compliance
+  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+ */
+bool drm_raw_edid_header_corrupt(void);
+
 #endif /* __DRM_EDID_H__ */
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel