[Intel-gfx] [PATCH 6/6] drm/i915: Filter out modes that don't match the fixed mode vrefresh

[Chris Wilson]

Quoting Daniel Vetter (2018-07-03 08:03:09)
> On Mon, Jul 02, 2018 at 12:52:31PM +0300, Ville Syrjälä wrote:
> > On Mon, Jul 02, 2018 at 09:46:23AM +0200, Daniel Vetter wrote:
> > > On Thu, Jun 28, 2018 at 10:43:01PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > 
> > > > We only ever drive the panel with the fixed mode, hence we don't
> > > > want to advertize any modes that have a different vertical refresh rate.
> > > > 
> > > > We tried to allow a second lower clocked mode to used for eDP but
> > > > that was reverted in commit d93fa1b47b8f ("Revert "drm/i915/edp:
> > > > Allow alternate fixed mode for eDP if available.""). To do that properly
> > > > I think we should probably just keep all (or just some?) of the probed
> > > > modes around (presumably all of them actually work if the panel
> > > > advertizes them), and we'd just pick the closest match based on the
> > > > user mode.
> > > > 
> > > > For now we don't have any of that so we shouldn't lie to the user
> > > > that they can drive the panel at different refresh rate. Note that
> > > > we still don't reject any user mode with bad refresh rate. That's
> > > > going to be step two.
> > > > 
> > > > TODO: Should we allow for a larger error margin?
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > Not sure the current vrefresh is any good, since it's HZ resolution only.
> > > That's not precise enough for the video aficionados, who insist on that
> > > entire 1001/1000 business.
> > > 
> > > I did look around a bit with a quick git grep, and most places only use
> > > ->vrefresh for debug printing. Then there's a few that use it as an index
> > > (e.g. i915 DRRS code), and then there's the totally misguided code in
> > > adv7511 which thinks vrefresh is in kHz.
> > > 
> > > There's also quite a pile of mode->vrefresh ? mode->vrefresh :
> > > drm_mode_vrefresh(mode) around, which really doesn't inspire confidence
> > > that we'll ever catch them all. It feels like those "recompute vrefresh"
> > > things have been cargo-culted for ages.
> > > 
> > > I think we need to clean this up harder:
> > > - switch everyone over to drm_mode_vrefresh()
> > > - nuke drm_display_mode->vrefresh
> > > - add a new drm_mode_vrefresh_mHz (for milli-Hz, i.e. fixed point math
> > >   ftw)
> > > - use that new function in the mode filtering, with a bit of fudge that
> > >   keeps the 1001/1000 modes separate, but papers over rounding errors. I
> > >   think we could even do a drm_mode_compare_vrefresh for that, we're
> > >   probably not the only ones who'd like to have such a function.
> > > 
> > > Probably a bit more work to type (but cocci might help here), definitely
> > > less painful to make sure it stays correct I think. Thoughts?
> > 
> > Yeah, would probably make sense. But apart from the "make sure this
> > don't break" angle it doesn't matter for this case unless we want
> > to make the filtering even more strict than the 1 Hz resolution.
> 
> Well it's generally video folks who care about this, and those folks will
> spot the 1 Hz mismatch. I just realized that you can implement the
> filtering without all the refactoring:

You mean the same ones who complain about 24Hz != 23.976Hz, and the one
frame out of sync every 40s.
-Chris