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

[Ville Syrjälä]

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.

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