diff mbox series

drm/rockchip: Round up _before_ giving to the clock framework

Message ID 20191003102003.1.Ib233b3e706cf6317858384264d5b0ed35657456e@changeid (mailing list archive)
State Mainlined
Commit 287422a95fe28e05c1952de0472e0dfdffa6caae
Headers show
Series drm/rockchip: Round up _before_ giving to the clock framework | expand

Commit Message

Doug Anderson Oct. 3, 2019, 5:20 p.m. UTC
I'm embarassed to say that even though I've touched
vop_crtc_mode_fixup() twice and I swear I tested it, there's still a
stupid glaring bug in it.  Specifically, on veyron_minnie (with all
the latest display timings) we want to be setting our pixel clock to
66,666,666.67 Hz and we tell userspace that's what we set, but we're
actually choosing 66,000,000 Hz.  This is confirmed by looking at the
clock tree.

The problem is that in drm_display_mode_from_videomode() we convert
from Hz to kHz with:

  dmode->clock = vm->pixelclock / 1000;

...so when the device tree specifies a clock of 66666667 for the panel
then DRM translates that to 66666000.  The clock framework will always
pick a clock that is _lower_ than the one requested, so it will refuse
to pick 66666667 and we'll end up at 66000000.

While we could try to fix drm_display_mode_from_videomode() to round
to the nearest kHz and it would fix our problem, it wouldn't help if
the clock we actually needed was 60,000,001 Hz.  We could
alternatively have DRM always round up, but maybe this would break
someone else who already baked in the assumption that DRM rounds down.

Let's solve this by just adding 999 Hz before calling
clk_round_rate().  This should be safe and work everywhere.

NOTE: if this is picked to stable, it's probably easiest to first pick
commit 527e4ca3b6d1 ("drm/rockchip: Base adjustments of the mode based
on prev adjustments") which shouldn't hurt in stable.

Fixes: b59b8de31497 ("drm/rockchip: return a true clock rate to adjusted_mode")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 37 +++++++++++++++++++--
 1 file changed, 34 insertions(+), 3 deletions(-)

Comments

Sean Paul Oct. 3, 2019, 6:36 p.m. UTC | #1
On Thu, Oct 3, 2019 at 1:20 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> I'm embarassed to say that even though I've touched
> vop_crtc_mode_fixup() twice and I swear I tested it, there's still a
> stupid glaring bug in it.  Specifically, on veyron_minnie (with all
> the latest display timings) we want to be setting our pixel clock to
> 66,666,666.67 Hz and we tell userspace that's what we set, but we're
> actually choosing 66,000,000 Hz.  This is confirmed by looking at the
> clock tree.
>
> The problem is that in drm_display_mode_from_videomode() we convert
> from Hz to kHz with:
>
>   dmode->clock = vm->pixelclock / 1000;
>
> ...so when the device tree specifies a clock of 66666667 for the panel
> then DRM translates that to 66666000.  The clock framework will always
> pick a clock that is _lower_ than the one requested, so it will refuse
> to pick 66666667 and we'll end up at 66000000.
>
> While we could try to fix drm_display_mode_from_videomode() to round
> to the nearest kHz and it would fix our problem,

I got a bit confused reading this and Doug straightened me out in a
sideband conversation.

To summarize, the drm_display_mode_from_videomode() call referenced
above is from panel-simple, and this downslotting is specific to
rockchip's clock driver. So I've asked to clarify these 2 points so
it's clear from the commit message that this patch is the best
solution. With that addressed,

Reviewed-by: Sean Paul <seanpaul@chromium.org>


> it wouldn't help if
> the clock we actually needed was 60,000,001 Hz.  We could
> alternatively have DRM always round up, but maybe this would break
> someone else who already baked in the assumption that DRM rounds down.
>
> Let's solve this by just adding 999 Hz before calling
> clk_round_rate().  This should be safe and work everywhere.
>
> NOTE: if this is picked to stable, it's probably easiest to first pick
> commit 527e4ca3b6d1 ("drm/rockchip: Base adjustments of the mode based
> on prev adjustments") which shouldn't hurt in stable.
>
> Fixes: b59b8de31497 ("drm/rockchip: return a true clock rate to adjusted_mode")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 37 +++++++++++++++++++--
>  1 file changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 613404f86668..84e3decb17b1 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1040,10 +1040,41 @@ static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
>                                 struct drm_display_mode *adjusted_mode)
>  {
>         struct vop *vop = to_vop(crtc);
> +       unsigned long rate;
>
> -       adjusted_mode->clock =
> -               DIV_ROUND_UP(clk_round_rate(vop->dclk,
> -                                           adjusted_mode->clock * 1000), 1000);
> +       /*
> +        * Clock craziness.
> +        *
> +        * Key points:
> +        *
> +        * - DRM works in in kHz.
> +        * - Clock framework works in Hz.
> +        * - Rockchip's clock driver picks the clock rate that is the
> +        *   same _OR LOWER_ than the one requested.
> +        *
> +        * Action plan:
> +        *
> +        * 1. When DRM gives us a mode, we should add 999 Hz to it.  That way
> +        *    if the clock we need is 60000001 Hz (~60 MHz) and DRM tells us to
> +        *    make 60000 kHz then the clock framework will actually give us
> +        *    the right clock.
> +        *
> +        *    NOTE: if the PLL (maybe through a divider) could actually make
> +        *    a clock rate 999 Hz higher instead of the one we want then this
> +        *    could be a problem.  Unfortunately there's not much we can do
> +        *    since it's baked into DRM to use kHz.  It shouldn't matter in
> +        *    practice since Rockchip PLLs are controlled by tables and
> +        *    even if there is a divider in the middle I wouldn't expect PLL
> +        *    rates in the table that are just a few kHz different.
> +        *
> +        * 2. Get the clock framework to round the rate for us to tell us
> +        *    what it will actually make.
> +        *
> +        * 3. Store the rounded up rate so that we don't need to worry about
> +        *    this in the actual clk_set_rate().
> +        */
> +       rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 + 999);
> +       adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
>
>         return true;
>  }
> --
> 2.23.0.444.g18eeb5a265-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Doug Anderson Oct. 3, 2019, 6:48 p.m. UTC | #2
Hi,

On Thu, Oct 3, 2019 at 11:37 AM Sean Paul <sean@poorly.run> wrote:
>
> On Thu, Oct 3, 2019 at 1:20 PM Douglas Anderson <dianders@chromium.org> wrote:
> >
> > I'm embarassed to say that even though I've touched
> > vop_crtc_mode_fixup() twice and I swear I tested it, there's still a
> > stupid glaring bug in it.  Specifically, on veyron_minnie (with all
> > the latest display timings) we want to be setting our pixel clock to
> > 66,666,666.67 Hz and we tell userspace that's what we set, but we're
> > actually choosing 66,000,000 Hz.  This is confirmed by looking at the
> > clock tree.
> >
> > The problem is that in drm_display_mode_from_videomode() we convert
> > from Hz to kHz with:
> >
> >   dmode->clock = vm->pixelclock / 1000;
> >
> > ...so when the device tree specifies a clock of 66666667 for the panel
> > then DRM translates that to 66666000.  The clock framework will always
> > pick a clock that is _lower_ than the one requested, so it will refuse
> > to pick 66666667 and we'll end up at 66000000.
> >
> > While we could try to fix drm_display_mode_from_videomode() to round
> > to the nearest kHz and it would fix our problem,
>
> I got a bit confused reading this and Doug straightened me out in a
> sideband conversation.
>
> To summarize, the drm_display_mode_from_videomode() call referenced
> above is from panel-simple, and this downslotting is specific to
> rockchip's clock driver. So I've asked to clarify these 2 points so
> it's clear from the commit message that this patch is the best
> solution. With that addressed,
>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>

Thanks for the review!  Hopefully people don't mind the quick spin...

https://lore.kernel.org/r/20191003114726.v2.1.Ib233b3e706cf6317858384264d5b0ed35657456e@changeid

-Doug
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 613404f86668..84e3decb17b1 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1040,10 +1040,41 @@  static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
 				struct drm_display_mode *adjusted_mode)
 {
 	struct vop *vop = to_vop(crtc);
+	unsigned long rate;
 
-	adjusted_mode->clock =
-		DIV_ROUND_UP(clk_round_rate(vop->dclk,
-					    adjusted_mode->clock * 1000), 1000);
+	/*
+	 * Clock craziness.
+	 *
+	 * Key points:
+	 *
+	 * - DRM works in in kHz.
+	 * - Clock framework works in Hz.
+	 * - Rockchip's clock driver picks the clock rate that is the
+	 *   same _OR LOWER_ than the one requested.
+	 *
+	 * Action plan:
+	 *
+	 * 1. When DRM gives us a mode, we should add 999 Hz to it.  That way
+	 *    if the clock we need is 60000001 Hz (~60 MHz) and DRM tells us to
+	 *    make 60000 kHz then the clock framework will actually give us
+	 *    the right clock.
+	 *
+	 *    NOTE: if the PLL (maybe through a divider) could actually make
+	 *    a clock rate 999 Hz higher instead of the one we want then this
+	 *    could be a problem.  Unfortunately there's not much we can do
+	 *    since it's baked into DRM to use kHz.  It shouldn't matter in
+	 *    practice since Rockchip PLLs are controlled by tables and
+	 *    even if there is a divider in the middle I wouldn't expect PLL
+	 *    rates in the table that are just a few kHz different.
+	 *
+	 * 2. Get the clock framework to round the rate for us to tell us
+	 *    what it will actually make.
+	 *
+	 * 3. Store the rounded up rate so that we don't need to worry about
+	 *    this in the actual clk_set_rate().
+	 */
+	rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 + 999);
+	adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
 
 	return true;
 }