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

[Daniel Vetter]

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?

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