Re: [Intel-gfx] [PATCH 07/11] drm/edid: Use GTF2 for inferred modes

Ville Syrjälä
2022-09-02T12:46:20Z

On Fri, Sep 02, 2022 at 03:25:39PM +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>
> >
> > For some resaon we only use the secondary GTF curve for the
> > standard timings. Use it for inferred modes as well.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > drivers/gpu/drm/drm_edid.c | 35 ++++++++++++++++++++++++++++++++++-
> > 1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 0c7cbe9b44f5..fed2bdd55c09 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -3546,6 +3546,35 @@ static int drm_gtf_modes_for_range(struct drm_connector *connector,
> > return modes;
> > }
> >
> > +static int drm_gtf2_modes_for_range(struct drm_connector *connector,
> > + const struct drm_edid *drm_edid,
> > + const struct detailed_timing *timing)
> > +{
> > + int i, modes = 0;
> > + struct drm_display_mode *newmode;
> > + struct drm_device *dev = connector->dev;
> > +
> > + for (i = 0; i < ARRAY_SIZE(extra_modes); i++) {
> > + const struct minimode *m = &extra_modes[i];
> > +
> > + newmode = drm_gtf2_mode(dev, drm_edid, m->w, m->h, m->r);
> > + if (!newmode)
> > + return modes;
> > +
> > + drm_mode_fixup_1366x768(newmode);
> > + if (!mode_in_range(newmode, drm_edid, timing) ||
> > + !valid_inferred_mode(connector, newmode)) {
> > + drm_mode_destroy(dev, newmode);
> > + continue;
> > + }
> > +
> > + drm_mode_probed_add(connector, newmode);
> > + modes++;
> > + }
> > +
> > + return modes;
> > +}
> > +
> > static int drm_cvt_modes_for_range(struct drm_connector *connector,
> > const struct drm_edid *drm_edid,
> > const struct detailed_timing *timing)
> > @@ -3594,7 +3623,11 @@ do_inferred_modes(const struct detailed_timing *timing, void *c)
> > return; /* GTF not defined yet */
> >
> > switch (range->flags) {
> > - case DRM_EDID_SECONDARY_GTF_SUPPORT_FLAG: /* XXX could do more */
> > + case DRM_EDID_SECONDARY_GTF_SUPPORT_FLAG:
> > + closure->modes += drm_gtf2_modes_for_range(closure->connector,
> > + closure->drm_edid,
> > + timing);
> > + break;
> > case DRM_EDID_DEFAULT_GTF_SUPPORT_FLAG:
>
> Additionally, per spec:
>
> * Default GTF supported if bit 0 in Feature Support Byte at address 18h = 1
>
> * Secondary GTF supported --- requires support for Default GTF
>
> So I guess both of these would need the edid->features &
> DRM_EDID_FEATURE_DEFAULT_GTF check?

There is one actually
if (drm_edid->edid->features & DRM_EDID_FEATURE_DEFAULT_GTF)
num_modes += add_inferred_modes(connector, drm_edid);

Though as I think I mentioned in some of these patches a lot of
real world EDIDs don't set the default gtf/continuous frequency
bit but still include a range descriptor. While illegal, I think
a reasonable interpretation might be that they want us to use
the formula specified in the range descriptor for the non-DMT
standard timings, while still indicating that other timings
generated by said formula are not supported.

>
> Other than that,
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
>
>
> > closure->modes += drm_gtf_modes_for_range(closure->connector,
> > closure->drm_edid,
>
> --
> Jani Nikula, Intel Open Source Graphics Center

--
Ville Syrjälä
Intel