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

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

[Linus Walleij]

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>
---
 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));
 	dev_dbg(d->dev, "picoseconds between two pixels: %llu\n",
 		pclk);
 
-- 
2.31.1


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

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

[Stephan Gerhold]

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