[PATCH 1/2] drm/arm/hdlcd: Actually validate CRTC modes

[PATCH 1/2] drm/arm/hdlcd: Actually validate CRTC modes

[Robin Murphy]

Rather than allowing any old mode through, then subsequently refusing
unmatchable clock rates in atomic_check when it's too late to back out
and pick a different mode, let's do that validation up-front where it
will cause unsupported modes to be correctly pruned in the first place.

This also eliminates an issue whereby a perceived clock rate of 0 would
cause atomic disable to fail and prevent the module from being unloaded.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

This supersedes my previous patch here:
https://patchwork.freedesktop.org/patch/288553/
---
 drivers/gpu/drm/arm/hdlcd_crtc.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 0b2b62f8fa3c..ecac6fe0b213 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -186,20 +186,19 @@ static void hdlcd_crtc_atomic_disable(struct drm_crtc *crtc,
 	clk_disable_unprepare(hdlcd->clk);
 }
 
-static int hdlcd_crtc_atomic_check(struct drm_crtc *crtc,
-				   struct drm_crtc_state *state)
+static enum drm_mode_status hdlcd_crtc_mode_valid(struct drm_crtc *crtc,
+		const struct drm_display_mode *mode)
 {
 	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
-	struct drm_display_mode *mode = &state->adjusted_mode;
 	long rate, clk_rate = mode->clock * 1000;
 
 	rate = clk_round_rate(hdlcd->clk, clk_rate);
 	if (rate != clk_rate) {
 		/* clock required by mode not supported by hardware */
-		return -EINVAL;
+		return MODE_NOCLOCK;
 	}
 
-	return 0;
+	return MODE_OK;
 }
 
 static void hdlcd_crtc_atomic_begin(struct drm_crtc *crtc,
@@ -220,7 +219,7 @@ static void hdlcd_crtc_atomic_begin(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs hdlcd_crtc_helper_funcs = {
-	.atomic_check	= hdlcd_crtc_atomic_check,
+	.mode_valid	= hdlcd_crtc_mode_valid,
 	.atomic_begin	= hdlcd_crtc_atomic_begin,
 	.atomic_enable	= hdlcd_crtc_atomic_enable,
 	.atomic_disable	= hdlcd_crtc_atomic_disable,
-- 
2.21.0.dirty

[PATCH 2/2] drm/arm/hdlcd: Allow a bit of clock tolerance

[Robin Murphy]

On the Arm Juno platform, the HDLCD pixel clock is constrained to 250KHz
resolution in order to avoid the tiny System Control Processor spending
aeons trying to calculate exact PLL coefficients. This means that modes
like my oddball 1600x1200 with 130.89MHz clock get rejected since the
rate cannot be matched exactly. In practice, though, this mode works
quite happily with the clock at 131MHz, so let's relax the check to
allow a little bit of slop.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/gpu/drm/arm/hdlcd_crtc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index ecac6fe0b213..a3efa28436ea 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -193,7 +193,8 @@ static enum drm_mode_status hdlcd_crtc_mode_valid(struct drm_crtc *crtc,
 	long rate, clk_rate = mode->clock * 1000;
 
 	rate = clk_round_rate(hdlcd->clk, clk_rate);
-	if (rate != clk_rate) {
+	/* 0.1% seems a close enough tolerance for the TDA19988 on Juno */
+	if (abs(rate - clk_rate) * 1000 > clk_rate) {
 		/* clock required by mode not supported by hardware */
 		return MODE_NOCLOCK;
 	}
-- 
2.21.0.dirty

Re: [PATCH 1/2] drm/arm/hdlcd: Actually validate CRTC modes

[Liviu Dudau]

On Fri, May 17, 2019 at 05:37:21PM +0100, Robin Murphy wrote:
> Rather than allowing any old mode through, then subsequently refusing
> unmatchable clock rates in atomic_check when it's too late to back out
> and pick a different mode, let's do that validation up-front where it
> will cause unsupported modes to be correctly pruned in the first place.
> 
> This also eliminates an issue whereby a perceived clock rate of 0 would
> cause atomic disable to fail and prevent the module from being unloaded.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

Thanks for the patch!

Best regards,
Liviu

> ---
> 
> This supersedes my previous patch here:
> https://patchwork.freedesktop.org/patch/288553/
> ---
>  drivers/gpu/drm/arm/hdlcd_crtc.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index 0b2b62f8fa3c..ecac6fe0b213 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -186,20 +186,19 @@ static void hdlcd_crtc_atomic_disable(struct drm_crtc *crtc,
>  	clk_disable_unprepare(hdlcd->clk);
>  }
>  
> -static int hdlcd_crtc_atomic_check(struct drm_crtc *crtc,
> -				   struct drm_crtc_state *state)
> +static enum drm_mode_status hdlcd_crtc_mode_valid(struct drm_crtc *crtc,
> +		const struct drm_display_mode *mode)
>  {
>  	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
> -	struct drm_display_mode *mode = &state->adjusted_mode;
>  	long rate, clk_rate = mode->clock * 1000;
>  
>  	rate = clk_round_rate(hdlcd->clk, clk_rate);
>  	if (rate != clk_rate) {
>  		/* clock required by mode not supported by hardware */
> -		return -EINVAL;
> +		return MODE_NOCLOCK;
>  	}
>  
> -	return 0;
> +	return MODE_OK;
>  }
>  
>  static void hdlcd_crtc_atomic_begin(struct drm_crtc *crtc,
> @@ -220,7 +219,7 @@ static void hdlcd_crtc_atomic_begin(struct drm_crtc *crtc,
>  }
>  
>  static const struct drm_crtc_helper_funcs hdlcd_crtc_helper_funcs = {
> -	.atomic_check	= hdlcd_crtc_atomic_check,
> +	.mode_valid	= hdlcd_crtc_mode_valid,
>  	.atomic_begin	= hdlcd_crtc_atomic_begin,
>  	.atomic_enable	= hdlcd_crtc_atomic_enable,
>  	.atomic_disable	= hdlcd_crtc_atomic_disable,
> -- 
> 2.21.0.dirty
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH 2/2] drm/arm/hdlcd: Allow a bit of clock tolerance

[Liviu Dudau]

On Fri, May 17, 2019 at 05:37:22PM +0100, Robin Murphy wrote:
> On the Arm Juno platform, the HDLCD pixel clock is constrained to 250KHz
> resolution in order to avoid the tiny System Control Processor spending
> aeons trying to calculate exact PLL coefficients. This means that modes
> like my oddball 1600x1200 with 130.89MHz clock get rejected since the
> rate cannot be matched exactly. In practice, though, this mode works
> quite happily with the clock at 131MHz, so let's relax the check to
> allow a little bit of slop.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

I've pull the two patches into my malidp-fixes branch and I will send a pull
request today.

Best regards,
Liviu

> ---
>  drivers/gpu/drm/arm/hdlcd_crtc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index ecac6fe0b213..a3efa28436ea 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -193,7 +193,8 @@ static enum drm_mode_status hdlcd_crtc_mode_valid(struct drm_crtc *crtc,
>  	long rate, clk_rate = mode->clock * 1000;
>  
>  	rate = clk_round_rate(hdlcd->clk, clk_rate);
> -	if (rate != clk_rate) {
> +	/* 0.1% seems a close enough tolerance for the TDA19988 on Juno */
> +	if (abs(rate - clk_rate) * 1000 > clk_rate) {
>  		/* clock required by mode not supported by hardware */
>  		return MODE_NOCLOCK;
>  	}
> -- 
> 2.21.0.dirty
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯