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.rip...@bootlin.com>
With the define fixed and given the comment below, this is:
Reviewed-by: Paul Kocialkowski <paul.kocialkow...@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
