[PATCH 4/4] drm/sun4i: rgb: Change the pixel clock validation check

[Paul Kocialkowski]

Hi,

On Tue, 2019-02-26 at 15:25 +0100, Maxime Ripard wrote:
> The current code, since commit bb43d40d7c83 ("drm/sun4i: rgb: Validate the
> clock rate"), perform some validation on the pixel clock to filter out the
> EDID modes provided by monitors (through bridges) that we wouldn't be able
> to reach. For the usual modes, we're able to generate a perfect clock rate,
> so a strict check was enough.
> 
> However, this had the side effect of preventing displays that would work
> otherwise to operate properly, since we would pretty much never be able to
> generate an exact rate for those displays, even though we would fall within
> that panel tolerance.
> 
> This was also shown to happen for unusual modes exposed through EDIDs, for
> example on eDP panels.
> 
> We can work around this by simplifying a bit the problem: no panels we've
> encountered so far actually needed that check. All of them are tied to a
> particular board when it is produced, and made to work with the Allwinner
> BSP. That pretty much guarantees that we never have a pixel clock out of
> reach.
> 
> On the other hand, the EDIDs modes that needed to be validated have always
> been exposed through bridges.
> 
> Let's just use that metric to instead of validating all modes, only
> validate modes when we have a bridge attached. It should be good enough for
> now, while we still have room for improvements or refinements using the
> display_timings structure for example for panels.
> 
> We also add a tolerance for EDID-based modes instead of doing a strict
> check. This tolerance is of 0.5% which is the one advertised in the VESA
> DVT and CVT specs. If that needed to be extended in the future, we can add
> a custom module parameter to relax it a bit.
> 
> Fixes: bb43d40d7c83 ("drm/sun4i: rgb: Validate the clock rate")
> Signed-off-by: Maxime Ripard <maxime.ripard at bootlin.com>

With the define fixed and given the comment below, this is:

Reviewed-by: Paul Kocialkowski <paul.kocialkowski at bootlin.com>

> ---
>  drivers/gpu/drm/sun4i/sun4i_rgb.c | 37 ++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> index 893b6e6d4d85..05beefe93989 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> @@ -53,6 +53,14 @@ static int sun4i_rgb_get_modes(struct drm_connector *connector)
>  	return drm_panel_get_modes(rgb->panel);
>  }
>  
> +/*
> + * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
> + * CVT spec reuses that tolerance in its examples, so it looks to be a
> + * good default tolerance for the EDID-based modes. Define it to 5 per
> + * mille to avoid floating point operations.
> + */
> +DEFINE SUN4I_RGB_DOTCLOCK_TOLERANCE	5

It could be nice to have some kind of unit made explicit in the define.
Maybe something like SUN4I_RGB_DOTCLOCK_TOLERANCE_PER_THOUSAND.

That's just a suggestion, feel free to take it or leave it :)

Cheers,

Paul

> +
>  static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
>  						 const struct drm_display_mode *mode)
>  {
> @@ -61,6 +69,7 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
>  	u32 hsync = mode->hsync_end - mode->hsync_start;
>  	u32 vsync = mode->vsync_end - mode->vsync_start;
>  	unsigned long long rate = mode->clock * 1000;
> +	unsigned long long lowest, highest;
>  	unsigned long long rounded_rate;
>  
>  	DRM_DEBUG_DRIVER("Validating modes...\n");
> @@ -93,15 +102,39 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
>  
>  	DRM_DEBUG_DRIVER("Vertical parameters OK\n");
>  
> +	/*
> +	 * TODO: We should use the struct display_timing if available
> +	 * and / or trying to stretch the timings within that
> +	 * tolerancy to take care of panels that we wouldn't be able
> +	 * to have a exact match for.
> +	 */
> +	if (rgb->panel) {
> +		DRM_DEBUG_DRIVER("RGB panel used, skipping clock rate checks");
> +		goto out;
> +	}
> +
> +	/*
> +	 * That shouldn't ever happen unless something is really wrong, but it
> +	 * doesn't harm to check.
> +	 */
> +	if (!rgb->bridge)
> +		goto out;
> +
>  	tcon->dclk_min_div = 6;
>  	tcon->dclk_max_div = 127;
>  	rounded_rate = clk_round_rate(tcon->dclk, rate);
> -	if (rounded_rate < rate)
> +
> +	lowest = rate * (1000 - SUN4I_RGB_DOTCLOCK_TOLERANCE);
> +	do_div(lowest, 1000);
> +	if (rounded_rate < lowest)
>  		return MODE_CLOCK_LOW;
>  
> -	if (rounded_rate > rate)
> +	highest = rate * (1000 + SUN4I_RGB_DOTCLOCK_TOLERANCE);
> +	do_div(highest, 1000);
> +	if (rounded_rate > highest)
>  		return MODE_CLOCK_HIGH;
>  
> +out:
>  	DRM_DEBUG_DRIVER("Clock rate OK\n");
>  
>  	return MODE_OK;
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com