linux-kernel - Re: [PATCH 1/2] drm/atmel-hlcdc: prefer a higher rate clock as pixel-clock base

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180824105939.24fe8013@bbrezillon>
Date:   Fri, 24 Aug 2018 10:59:39 +0200
From:   Boris Brezillon <boris.brezillon@...tlin.com>
To:     Peter Rosin <peda@...ntia.se>
Cc:     linux-kernel@...r.kernel.org, David Airlie <airlied@...ux.ie>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        dri-devel@...ts.freedesktop.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/2] drm/atmel-hlcdc: prefer a higher rate clock as
 pixel-clock base

On Fri, 24 Aug 2018 10:55:00 +0200
Peter Rosin <peda@...ntia.se> wrote:

> If the divider used to get the pixel-clock is small, the granularity
> of the frequencies possible for the pixel-clock is quite coarse. E.g.
> requesting a pixel-clock of 65MHz with a sys_clk of 132MHz results
> in the divider being set to 3 ending up with 44MHz.
> 
> By preferring the doubled sys_clk as base, the divider instead ends
> up as 5 yielding a pixel-clock of 52.8Mhz, which is a definite
> improvement.
> 
> While at it, clamp the divider so that it does not overflow in case
> it gets big.
> 
> Signed-off-by: Peter Rosin <peda@...ntia.se>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index c38a479ada98..71c9cd90d2ae 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -101,18 +101,22 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>  		     (adj->crtc_hdisplay - 1) |
>  		     ((adj->crtc_vdisplay - 1) << 16));
>  
> -	cfg = 0;
> +	cfg = ATMEL_HLCDC_CLKSEL;
>  
> -	prate = clk_get_rate(crtc->dc->hlcdc->sys_clk);
> +	prate = 2 * clk_get_rate(crtc->dc->hlcdc->sys_clk);
>  	mode_rate = adj->crtc_clock * 1000;
> -	if ((prate / 2) < mode_rate) {
> -		prate *= 2;
> -		cfg |= ATMEL_HLCDC_CLKSEL;
> -	}
>  
>  	div = DIV_ROUND_UP(prate, mode_rate);
>  	if (div < 2)
>  		div = 2;

I'm nitpicking, but can you add braces around the if() block?

Looks good otherwise.

> +	else if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK) {
> +		/* the divider ended up too big, try a lower base rate */
> +		cfg &= ~ATMEL_HLCDC_CLKSEL;
> +		prate /= 2;
> +		div = DIV_ROUND_UP(prate, mode_rate);
> +		if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK)
> +			div = ATMEL_HLCDC_CLKDIV_MASK;
> +	}
>  
>  	cfg |= ATMEL_HLCDC_CLKDIV(div);
>