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.syrj...@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.syrj...@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: - Add drm_mode_vrefresh_mHz. - Use it to filter modes. Since it's entirely new there's no need to clean up the entire confusion around mode->vrefresh first at all :-) -Daniel > > > > > Cheers, Daniel > > > > > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 3 +++ > > > drivers/gpu/drm/i915/intel_dsi.c | 2 ++ > > > drivers/gpu/drm/i915/intel_dvo.c | 2 ++ > > > drivers/gpu/drm/i915/intel_lvds.c | 2 ++ > > > drivers/gpu/drm/i915/intel_sdvo.c | 2 ++ > > > 5 files changed, 11 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index 5be07e1d816d..22feb0b144f6 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -446,6 +446,9 @@ intel_dp_mode_valid(struct drm_connector *connector, > > > if (mode->vdisplay > fixed_mode->vdisplay) > > > return MODE_PANEL; > > > > > > + if (mode->vrefresh != fixed_mode->vrefresh) > > > + return MODE_PANEL; > > > + > > > target_clock = fixed_mode->clock; > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c > > > b/drivers/gpu/drm/i915/intel_dsi.c > > > index 3b7acb5a70b3..3d0758cec85a 100644 > > > --- a/drivers/gpu/drm/i915/intel_dsi.c > > > +++ b/drivers/gpu/drm/i915/intel_dsi.c > > > @@ -1277,6 +1277,8 @@ intel_dsi_mode_valid(struct drm_connector > > > *connector, > > > return MODE_PANEL; > > > if (mode->vdisplay > fixed_mode->vdisplay) > > > return MODE_PANEL; > > > + if (mode->vrefresh != fixed_mode->vrefresh) > > > + return MODE_PANEL; > > > if (fixed_mode->clock > max_dotclk) > > > return MODE_CLOCK_HIGH; > > > } > > > diff --git a/drivers/gpu/drm/i915/intel_dvo.c > > > b/drivers/gpu/drm/i915/intel_dvo.c > > > index 4e142ff49708..078bb848a49a 100644 > > > --- a/drivers/gpu/drm/i915/intel_dvo.c > > > +++ b/drivers/gpu/drm/i915/intel_dvo.c > > > @@ -225,6 +225,8 @@ intel_dvo_mode_valid(struct drm_connector *connector, > > > return MODE_PANEL; > > > if (mode->vdisplay > fixed_mode->vdisplay) > > > return MODE_PANEL; > > > + if (mode->vrefresh != fixed_mode->vrefresh) > > > + return MODE_PANEL; > > > > > > target_clock = fixed_mode->clock; > > > } > > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c > > > b/drivers/gpu/drm/i915/intel_lvds.c > > > index bb06744d28a4..d312a8911b89 100644 > > > --- a/drivers/gpu/drm/i915/intel_lvds.c > > > +++ b/drivers/gpu/drm/i915/intel_lvds.c > > > @@ -384,6 +384,8 @@ intel_lvds_mode_valid(struct drm_connector *connector, > > > return MODE_PANEL; > > > if (mode->vdisplay > fixed_mode->vdisplay) > > > return MODE_PANEL; > > > + if (mode->vrefresh != fixed_mode->vrefresh) > > > + return MODE_PANEL; > > > if (fixed_mode->clock > max_pixclk) > > > return MODE_CLOCK_HIGH; > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c > > > b/drivers/gpu/drm/i915/intel_sdvo.c > > > index cd5d5e800486..4be4bae51652 100644 > > > --- a/drivers/gpu/drm/i915/intel_sdvo.c > > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > > > @@ -1650,6 +1650,8 @@ intel_sdvo_mode_valid(struct drm_connector > > > *connector, > > > return MODE_PANEL; > > > > > > if (mode->vdisplay > fixed_mode->vdisplay) > > > + > > > + if (mode->vrefresh != fixed_mode->vrefresh) > > > return MODE_PANEL; > > > } > > > > > > -- > > > 2.16.4 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Ville Syrjälä > Intel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel