On Sat, 27 Aug 2022, Ville Syrjala <ville.syrj...@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrj...@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.syrj...@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.nik...@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
