diff mbox series

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

Message ID ec2dc2a7b3d4bd44f7a2a6e1c1813f92449a7310.1551191081.git-series.maxime.ripard@bootlin.com (mailing list archive)
State New, archived
Headers show
Series [1/4] drm/sun4i: Move the panel pointer from the TCON to the encoders | expand

Commit Message

Maxime Ripard Feb. 26, 2019, 2:25 p.m. UTC 
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@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 37 ++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

Comments

kernel test robot Feb. 27, 2019, 3:55 a.m. UTC | #1 
Hi Maxime,

I love your patch! Yet something to improve:

[auto build test ERROR on ]

url:    https://github.com/0day-ci/linux/commits/Maxime-Ripard/drm-sun4i-rgb-Relax-the-pixel-clock-check/20190227-012757
base:    
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.2.0 make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

>> drivers/gpu//drm/sun4i/sun4i_rgb.c:62:1: error: unknown type name 'DEFINE'; did you mean 'EMFILE'?
    DEFINE SUN4I_RGB_DOTCLOCK_TOLERANCE 5
    ^~~~~~
    EMFILE
   drivers/gpu//drm/sun4i/sun4i_rgb.c:62:37: error: expected '=', ',', ';', 'asm' or '__attribute__' before numeric constant
    DEFINE SUN4I_RGB_DOTCLOCK_TOLERANCE 5
                                        ^
   drivers/gpu//drm/sun4i/sun4i_rgb.c:191:16: error: 'sun4i_rgb_mode_valid' undeclared here (not in a function); did you mean 'sun4i_rgb_con_funcs'?
     .mode_valid = sun4i_rgb_mode_valid,
                   ^~~~~~~~~~~~~~~~~~~~
                   sun4i_rgb_con_funcs

vim +62 drivers/gpu//drm/sun4i/sun4i_rgb.c

    55	
    56	/*
    57	 * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
    58	 * CVT spec reuses that tolerance in its examples, so it looks to be a
    59	 * good default tolerance for the EDID-based modes. Define it to 5 per
    60	 * mille to avoid floating point operations.
    61	 */
  > 62	DEFINE SUN4I_RGB_DOTCLOCK_TOLERANCE	5
    63	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Paul Kocialkowski March 7, 2019, 1:36 p.m. UTC | #2 
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@bootlin.com>

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

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@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;
diff mbox series

Patch

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
+
 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;