Re: [PATCH 3/3] drm: Store the calculated vrefresh in the user mode

Ville Syrjälä
2018-03-14T14:55:33Z

On Tue, Mar 13, 2018 at 08:04:03PM +0100, Maarten Lankhorst wrote:
> Op 13-03-18 om 16:07 schreef Ville Syrjala:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Ignore the vrefresh in the mode the user passed in and instead
> > calculate the value based on the actual timings. This way we can
> > actually trust mode->vrefresh to some degree.
> >
> > Or should we compare the user's idea of vrefresh with the one
> > we get from the timings and return an error if they differ? We
> > can't really be sure what the user is asking in that case.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > drivers/gpu/drm/drm_modes.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index f6b7c0e36a1a..021526ec6fa0 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -1609,7 +1609,11 @@ int drm_mode_convert_umode(struct drm_device *dev,
> > out->vsync_end = in->vsync_end;
> > out->vtotal = in->vtotal;
> > out->vscan = in->vscan;
> > - out->vrefresh = in->vrefresh;
> > + /*
> > + * Ignore what the user is saying here and instead
> > + * calculate vrefresh based on the actual timings.
> > + */
> > + out->vrefresh = 0;
> > out->flags = in->flags;
> > out->type = in->type;
> > strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> > @@ -1619,6 +1623,8 @@ int drm_mode_convert_umode(struct drm_device *dev,
> > if (out->status != MODE_OK)
> > return -EINVAL;
> >
> > + out->vrefresh = drm_mode_vrefresh(out);
> > +
> > drm_mode_set_crtcinfo(out, CRTC_INTERLACE_HALVE_V);
> >
> > return 0;
>
> Could we also get this with dim fixes tags dc911f5bd8aa, so we can backport the alt mode handling patch?

Do we want/need to backport it actually?

>
> And update the documentation about vrefresh, that you can retrieve it with drm_mode_vrefresh?

The whole "return cached value if present, otherwise calculate but don't
update the cached value" apporach always seemed off to me. Might be a
good idea to change it somehow. Maybe just always calculate it, or do
the cached value updates in sensible places so that you only have to
calculate once. But I haven't actually checked how much work that
would entail.

--
Ville Syrjälä
Intel OTC