drm/i915/dp: set fail-safe mode if EDID corrupt
diff mbox series

Message ID 20200121125613.21138-1-shawn.c.lee@intel.com
State New
Headers show
Series
  • drm/i915/dp: set fail-safe mode if EDID corrupt
Related show

Commit Message

Lee Shawn C Jan. 21, 2020, 12:56 p.m. UTC
According to chapter 5.1.1.2 in DP spec. When the Sink
device capability is unknown, for example due to corruption
of an EDID. The Source device may fall back to a set of
fall-back video timing formats its choice. When none of
the fall-back video timing formats is acceptable, the
Source device must fall back to the fail safe mode,
which is 640 x 480 at 60Hz.

This change set source driver output fail-safe mode automatically
if EDID corrupt. It may also benefit link layer compliance
test case "4.2.2.6  EDID Corruption Detection".

Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Cooper Chiou <cooper.chiou@intel.com>
Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Comments

Jani Nikula Jan. 21, 2020, 8:44 a.m. UTC | #1
On Tue, 21 Jan 2020, Lee Shawn C <shawn.c.lee@intel.com> wrote:
> According to chapter 5.1.1.2 in DP spec. When the Sink
> device capability is unknown, for example due to corruption
> of an EDID. The Source device may fall back to a set of
> fall-back video timing formats its choice. When none of
> the fall-back video timing formats is acceptable, the
> Source device must fall back to the fail safe mode,
> which is 640 x 480 at 60Hz.
>
> This change set source driver output fail-safe mode automatically
> if EDID corrupt. It may also benefit link layer compliance
> test case "4.2.2.6  EDID Corruption Detection".

Are you fixing a real user visible bug here, trying to pass compliance
tests, or just trying harder to comply with the spec?

I am wondering under what circumstances we could actually display a
640x480 image when everything else fails.

BR,
Jani.

>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index c27d3e7ac219..7e072db4a530 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5659,9 +5659,18 @@ intel_dp_get_edid(struct intel_dp *intel_dp)
>  			return NULL;
>  
>  		return drm_edid_duplicate(intel_connector->edid);
> -	} else
> -		return drm_get_edid(&intel_connector->base,
> +	} else {
> +		struct edid *edid;
> +
> +		edid = drm_get_edid(&intel_connector->base,
>  				    &intel_dp->aux.ddc);
> +
> +		if (intel_connector->base.edid_corrupt || !edid)
> +			if (drm_add_modes_noedid(&intel_connector->base, 640, 480))
> +				drm_set_preferred_mode(&intel_connector->base, 640, 480);
> +
> +		return edid;
> +	}
>  }
>  
>  static void
Lee Shawn C Jan. 21, 2020, 9:54 a.m. UTC | #2
On Tue, 21 Jan 2020, Jani Nikula wrote:
>On Tue, 21 Jan 2020, Lee Shawn C <shawn.c.lee@intel.com> wrote:
>> According to chapter 5.1.1.2 in DP spec. When the Sink device 
>> capability is unknown, for example due to corruption of an EDID. The 
>> Source device may fall back to a set of fall-back video timing formats 
>> its choice. When none of the fall-back video timing formats is 
>> acceptable, the Source device must fall back to the fail safe mode, 
>> which is 640 x 480 at 60Hz.
>>
>> This change set source driver output fail-safe mode automatically if 
>> EDID corrupt. It may also benefit link layer compliance test case 
>> "4.2.2.6  EDID Corruption Detection".
>
>Are you fixing a real user visible bug here, trying to pass compliance tests, or just trying harder to comply with the spec?
>
>I am wondering under what circumstances we could actually display a
>640x480 image when everything else fails.
>
>BR,
>Jani.
>

Yes! We try to run link layer compliance with UCD-400. 3rd party
lab used UCD-400 to run DP 1.4 link layer compliance instead of
DPR-120. Its behavior is a little differnt to DPR-120 for test
case 4.2.2.6. DPR-120 would assert HPD. Sent test request to source.
Then source driver got corrupt EDID from DPR-120.

But UCD-400 will not. It just assert HPD to source. Then driver
got corrupt EDID but without test request coming. So this change
can handle this case because of igt tool (intel_dp_compliance)
would not get test request and set resolution to 640*480 to pass
this case.

Best regards,
Shawn

>>
>> Cc: Manasi Navare <manasi.d.navare@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Cooper Chiou <cooper.chiou@intel.com>
>> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
>> b/drivers/gpu/drm/i915/display/intel_dp.c
>> index c27d3e7ac219..7e072db4a530 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -5659,9 +5659,18 @@ intel_dp_get_edid(struct intel_dp *intel_dp)
>>  			return NULL;
>>  
>>  		return drm_edid_duplicate(intel_connector->edid);
>> -	} else
>> -		return drm_get_edid(&intel_connector->base,
>> +	} else {
>> +		struct edid *edid;
>> +
>> +		edid = drm_get_edid(&intel_connector->base,
>>  				    &intel_dp->aux.ddc);
>> +
>> +		if (intel_connector->base.edid_corrupt || !edid)
>> +			if (drm_add_modes_noedid(&intel_connector->base, 640, 480))
>> +				drm_set_preferred_mode(&intel_connector->base, 640, 480);
>> +
>> +		return edid;
>> +	}
>>  }
>>  
>>  static void
>
>--
>Jani Nikula, Intel Open Source Graphics Center

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index c27d3e7ac219..7e072db4a530 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5659,9 +5659,18 @@  intel_dp_get_edid(struct intel_dp *intel_dp)
 			return NULL;
 
 		return drm_edid_duplicate(intel_connector->edid);
-	} else
-		return drm_get_edid(&intel_connector->base,
+	} else {
+		struct edid *edid;
+
+		edid = drm_get_edid(&intel_connector->base,
 				    &intel_dp->aux.ddc);
+
+		if (intel_connector->base.edid_corrupt || !edid)
+			if (drm_add_modes_noedid(&intel_connector->base, 640, 480))
+				drm_set_preferred_mode(&intel_connector->base, 640, 480);
+
+		return edid;
+	}
 }
 
 static void