diff mbox series

[08/11] drm/edid: Use the correct formula for standard timings

Message ID 20220826213501.31490-9-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/edid: Range descriptor stuff | expand

Commit Message

Ville Syrjala Aug. 26, 2022, 9:34 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Prefer the timing formula indicated by the range
descriptor for generating the non-DMT standard timings.

Previously we just used CVT for all EDID 1.4 continuous
frequency displays without even checking if the range
descriptor indicates otherwise. Now we check the range
descriptor first, and fall back to CVT if nothing else
was indicated. EDID 1.4 more or less deprecates GTF/GTF2
but there are still a lot of 1.4 EDIDs out there that
don't advertise CVT support, so seems safer to use the
formula the EDID actually reports as supported.

For EDID 1.3 we use GTF2 if indicated (as before), and for
EDID 1.2+ we now just use GTF without even checking the
feature flag. There seem to be quite a few EDIDs out there that
don't set the GTF feature flag but still include a GTF range
descriptor and non-DMT standard timings.

This to me seems to be roughly what appendix B of EDID 1.4
suggests should be done.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c | 49 +++++++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 8 deletions(-)

Comments

Jani Nikula Sept. 2, 2022, 1:41 p.m. UTC | #1 
On Sat, 27 Aug 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Prefer the timing formula indicated by the range
> descriptor for generating the non-DMT standard timings.
>
> Previously we just used CVT for all EDID 1.4 continuous
> frequency displays without even checking if the range
> descriptor indicates otherwise. Now we check the range
> descriptor first, and fall back to CVT if nothing else
> was indicated. EDID 1.4 more or less deprecates GTF/GTF2
> but there are still a lot of 1.4 EDIDs out there that
> don't advertise CVT support, so seems safer to use the
> formula the EDID actually reports as supported.
>
> For EDID 1.3 we use GTF2 if indicated (as before), and for
> EDID 1.2+ we now just use GTF without even checking the
> feature flag. There seem to be quite a few EDIDs out there that
> don't set the GTF feature flag but still include a GTF range
> descriptor and non-DMT standard timings.
>
> This to me seems to be roughly what appendix B of EDID 1.4
> suggests should be done.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 49 +++++++++++++++++++++++++++++++-------
>  1 file changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index fed2bdd55c09..c1c85b9e1208 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3077,20 +3077,53 @@ drm_gtf2_2j(const struct drm_edid *drm_edid)
>  	return descriptor ? descriptor->data.other_data.data.range.formula.gtf2.j : 0;
>  }
>  
> +static void
> +get_timing_level(const struct detailed_timing *descriptor, void *data)
> +{
> +	int *res = data;
> +
> +	if (!is_display_descriptor(descriptor, EDID_DETAIL_MONITOR_RANGE))
> +		return;
> +
> +	BUILD_BUG_ON(offsetof(typeof(*descriptor), data.other_data.data.range.flags) != 10);
> +
> +	switch (descriptor->data.other_data.data.range.flags) {
> +	case DRM_EDID_DEFAULT_GTF_SUPPORT_FLAG:
> +		*res = LEVEL_GTF;
> +		break;
> +	case DRM_EDID_SECONDARY_GTF_SUPPORT_FLAG:
> +		*res = LEVEL_GTF2;
> +		break;
> +	case DRM_EDID_CVT_SUPPORT_FLAG:
> +		*res = LEVEL_CVT;
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>  /* Get standard timing level (CVT/GTF/DMT). */
>  static int standard_timing_level(const struct drm_edid *drm_edid)
>  {
>  	const struct edid *edid = drm_edid->edid;
>  
> -	if (edid->revision >= 2) {
> -		if (edid->revision >= 4 && (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF))
> -			return LEVEL_CVT;
> -		if (drm_gtf2_hbreak(drm_edid))
> -			return LEVEL_GTF2;
> -		if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF)
> -			return LEVEL_GTF;
> +	if (edid->revision >= 4) {
> +		/*
> +		 * If the range descriptor doesn't
> +		 * indicate otherwise default to CVT
> +		 */
> +		int ret = LEVEL_CVT;
> +
> +		drm_for_each_detailed_block(drm_edid, get_timing_level, &ret);

Please remind me why it's okay to iterate all of them and pick the last?
I mean ret gets overwritten by the last block.

Otherwise it all seems okay to me, though I admit my confidence level in
this review is considerably lower than for the other patches.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> +
> +		return ret;
> +	} else if (edid->revision >= 3 && drm_gtf2_hbreak(drm_edid)) {
> +		return LEVEL_GTF2;
> +	} else if (edid->revision >= 2) {
> +		return LEVEL_GTF;
> +	} else {
> +		return LEVEL_DMT;
>  	}
> -	return LEVEL_DMT;
>  }
>  
>  /*
Ville Syrjala Sept. 2, 2022, 2:02 p.m. UTC | #2 
On Fri, Sep 02, 2022 at 04:41:36PM +0300, Jani Nikula wrote:
> On Sat, 27 Aug 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Prefer the timing formula indicated by the range
> > descriptor for generating the non-DMT standard timings.
> >
> > Previously we just used CVT for all EDID 1.4 continuous
> > frequency displays without even checking if the range
> > descriptor indicates otherwise. Now we check the range
> > descriptor first, and fall back to CVT if nothing else
> > was indicated. EDID 1.4 more or less deprecates GTF/GTF2
> > but there are still a lot of 1.4 EDIDs out there that
> > don't advertise CVT support, so seems safer to use the
> > formula the EDID actually reports as supported.
> >
> > For EDID 1.3 we use GTF2 if indicated (as before), and for
> > EDID 1.2+ we now just use GTF without even checking the
> > feature flag. There seem to be quite a few EDIDs out there that
> > don't set the GTF feature flag but still include a GTF range
> > descriptor and non-DMT standard timings.
> >
> > This to me seems to be roughly what appendix B of EDID 1.4
> > suggests should be done.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c | 49 +++++++++++++++++++++++++++++++-------
> >  1 file changed, 41 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index fed2bdd55c09..c1c85b9e1208 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -3077,20 +3077,53 @@ drm_gtf2_2j(const struct drm_edid *drm_edid)
> >  	return descriptor ? descriptor->data.other_data.data.range.formula.gtf2.j : 0;
> >  }
> >  
> > +static void
> > +get_timing_level(const struct detailed_timing *descriptor, void *data)
> > +{
> > +	int *res = data;
> > +
> > +	if (!is_display_descriptor(descriptor, EDID_DETAIL_MONITOR_RANGE))
> > +		return;
> > +
> > +	BUILD_BUG_ON(offsetof(typeof(*descriptor), data.other_data.data.range.flags) != 10);
> > +
> > +	switch (descriptor->data.other_data.data.range.flags) {
> > +	case DRM_EDID_DEFAULT_GTF_SUPPORT_FLAG:
> > +		*res = LEVEL_GTF;
> > +		break;
> > +	case DRM_EDID_SECONDARY_GTF_SUPPORT_FLAG:
> > +		*res = LEVEL_GTF2;
> > +		break;
> > +	case DRM_EDID_CVT_SUPPORT_FLAG:
> > +		*res = LEVEL_CVT;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> >  /* Get standard timing level (CVT/GTF/DMT). */
> >  static int standard_timing_level(const struct drm_edid *drm_edid)
> >  {
> >  	const struct edid *edid = drm_edid->edid;
> >  
> > -	if (edid->revision >= 2) {
> > -		if (edid->revision >= 4 && (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF))
> > -			return LEVEL_CVT;
> > -		if (drm_gtf2_hbreak(drm_edid))
> > -			return LEVEL_GTF2;
> > -		if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF)
> > -			return LEVEL_GTF;
> > +	if (edid->revision >= 4) {
> > +		/*
> > +		 * If the range descriptor doesn't
> > +		 * indicate otherwise default to CVT
> > +		 */
> > +		int ret = LEVEL_CVT;
> > +
> > +		drm_for_each_detailed_block(drm_edid, get_timing_level, &ret);
> 
> Please remind me why it's okay to iterate all of them and pick the last?
> I mean ret gets overwritten by the last block.

I think there is an implied assumption that there is only
zero or one of these included in the EDID. While not explicitly
stated in the spec, there is no mention anywhere what it would
mean to have multiple different types of range descriptors.
And so far I didn't come across any EDIDs that would disagree with
that.

> 
> Otherwise it all seems okay to me, though I admit my confidence level in
> this review is considerably lower than for the other patches.
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> > +
> > +		return ret;
> > +	} else if (edid->revision >= 3 && drm_gtf2_hbreak(drm_edid)) {
> > +		return LEVEL_GTF2;
> > +	} else if (edid->revision >= 2) {
> > +		return LEVEL_GTF;
> > +	} else {
> > +		return LEVEL_DMT;
> >  	}
> > -	return LEVEL_DMT;
> >  }
> >  
> >  /*
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index fed2bdd55c09..c1c85b9e1208 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3077,20 +3077,53 @@  drm_gtf2_2j(const struct drm_edid *drm_edid)
 	return descriptor ? descriptor->data.other_data.data.range.formula.gtf2.j : 0;
 }
 
+static void
+get_timing_level(const struct detailed_timing *descriptor, void *data)
+{
+	int *res = data;
+
+	if (!is_display_descriptor(descriptor, EDID_DETAIL_MONITOR_RANGE))
+		return;
+
+	BUILD_BUG_ON(offsetof(typeof(*descriptor), data.other_data.data.range.flags) != 10);
+
+	switch (descriptor->data.other_data.data.range.flags) {
+	case DRM_EDID_DEFAULT_GTF_SUPPORT_FLAG:
+		*res = LEVEL_GTF;
+		break;
+	case DRM_EDID_SECONDARY_GTF_SUPPORT_FLAG:
+		*res = LEVEL_GTF2;
+		break;
+	case DRM_EDID_CVT_SUPPORT_FLAG:
+		*res = LEVEL_CVT;
+		break;
+	default:
+		break;
+	}
+}
+
 /* Get standard timing level (CVT/GTF/DMT). */
 static int standard_timing_level(const struct drm_edid *drm_edid)
 {
 	const struct edid *edid = drm_edid->edid;
 
-	if (edid->revision >= 2) {
-		if (edid->revision >= 4 && (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF))
-			return LEVEL_CVT;
-		if (drm_gtf2_hbreak(drm_edid))
-			return LEVEL_GTF2;
-		if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF)
-			return LEVEL_GTF;
+	if (edid->revision >= 4) {
+		/*
+		 * If the range descriptor doesn't
+		 * indicate otherwise default to CVT
+		 */
+		int ret = LEVEL_CVT;
+
+		drm_for_each_detailed_block(drm_edid, get_timing_level, &ret);
+
+		return ret;
+	} else if (edid->revision >= 3 && drm_gtf2_hbreak(drm_edid)) {
+		return LEVEL_GTF2;
+	} else if (edid->revision >= 2) {
+		return LEVEL_GTF;
+	} else {
+		return LEVEL_DMT;
 	}
-	return LEVEL_DMT;
 }
 
 /*