Re: [Intel-gfx] [PATCH 08/11] drm/edid: Use the correct formula for standard timings

[Jani Nikula <jani.nikula@linux.intel.com>]

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;
>  }
>  
>  /*

-- 
Jani Nikula, Intel Open Source Graphics Center