On Sat, 27 Aug 2022, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxx> > --- > 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@xxxxxxxxx> > + > + 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