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@xxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
