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@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@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