Re: [PATCH] drm/mcde: Fix off by 10^3 in calculation

From: Stephan Gerhold <stephan@gerhold.net>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: dri-devel@lists.freedesktop.org,
	"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Sean Paul" <sean@poorly.run>,
	linux-arm-kernel@lists.infradead.org,
	"Ville Syrjälä" <ville.syrjala@linux.intel.com>
Subject: Re: [PATCH] drm/mcde: Fix off by 10^3 in calculation
Date: Wed, 9 Jun 2021 12:04:52 +0200	[thread overview]
Message-ID: <YMCSRLsPBfvlwt1e@gerhold.net> (raw)
In-Reply-To: <20210608213318.3897858-1-linus.walleij@linaro.org>

On Tue, Jun 08, 2021 at 11:33:18PM +0200, Linus Walleij wrote:
> The calclulation of how many bytes we stuff into the
> DSI pipeline for video mode panels is off by three
> orders of magnitude because we did not account for the
> fact that the DRM mode clock is in kilohertz rather
> than hertz.
> 
> This used to be:
> drm_mode_vrefresh(mode) * mode->htotal * mode->vtotal
> which would become for example for s6e63m0:
> 60 x 514 x 831 = 25628040 Hz, but mode->clock is
> 25628 as it is in kHz.
> 
> This affects only the Samsung GT-I8190 "Golden" phone
> right now since it is the only MCDE device with a video
> mode display.
> 
> Curiously some specimen work with this code and wild
> settings in the EOL and empty packets at the end of the
> display, but I have noticed an eeire flicker until now.
> Others were not so lucky and got black screens.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reported-by: Stephan Gerhold <stephan@gerhold.net>
> Fixes: 920dd1b1425b ("drm/mcde: Use mode->clock instead of reverse calculating it from the vrefresh")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Can confirm this makes things much better, thanks :)
There is some garbage on the screen for a short moment, but overall it
works really well now.

> ---
>  drivers/gpu/drm/mcde/mcde_dsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c
> index b3fd3501c412..5275b2723293 100644
> --- a/drivers/gpu/drm/mcde/mcde_dsi.c
> +++ b/drivers/gpu/drm/mcde/mcde_dsi.c
> @@ -577,7 +577,7 @@ static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
>  	 * porches and sync.
>  	 */
>  	/* (ps/s) / (pixels/s) = ps/pixels */
> -	pclk = DIV_ROUND_UP_ULL(1000000000000, mode->clock);
> +	pclk = DIV_ROUND_UP_ULL(1000000000000, (mode->clock * 1000));

Removing three 0 in the dividend might be slightly more efficient, i.e.

	pclk = DIV_ROUND_UP(1000000000, mode->clock);

since then we don't need 64-bit division (_ULL) anymore
(1000000000 < 4294967296 = 2^32).

but that's more nitpick level. I tested both, so for both options:

Tested-by: Stephan Gerhold <stephan@gerhold.net>
Reviewed-by: Stephan Gerhold <stephan@gerhold.net>

Thanks!
Stephan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel