• Douglas Anderson's avatar
    drm/rockchip: Round up _before_ giving to the clock framework · 287422a9
    Douglas Anderson authored
    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;
    
    ...and drm_display_mode_from_videomode() is called from panel-simple
    when we have an "override_mode" like we do on veyron_minnie.  See
    commit 123643e5 ("ARM: dts: rockchip: Specify
    rk3288-veyron-minnie's display timings").
    
    ...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.
    Specifically note that clock drivers are not consistent about whether
    they round up or round down when you call clk_set_rate().  We know how
    Rockchip's clock driver works, but (for instance) you can see that on
    most Qualcomm clocks the default is clk_rcg2_ops which rounds up.
    
    Let's solve this by just adding 999 Hz before calling
    clk_round_rate().  This should be safe and work everywhere.  As
    discussed in more detail in comments in the commit, Rockchip's PLLs
    are configured in a way that there shouldn't be another PLL setting
    that is only a few kHz off so we won't get mixed up.
    
    NOTE: if this is picked to stable, it's probably easiest to first pick
    commit 527e4ca3 ("drm/rockchip: Base adjustments of the mode based
    on prev adjustments") which shouldn't hurt in stable.
    
    Fixes: b59b8de3 ("drm/rockchip: return a true clock rate to adjusted_mode")
    Signed-off-by: default avatarDouglas Anderson <dianders@chromium.org>
    Reviewed-by: default avatarSean Paul <seanpaul@chromium.org>
    Signed-off-by: default avatarSean Paul <seanpaul@chromium.org>
    Link: https://patchwork.freedesktop.org/patch/msgid/20191003114726.v2.1.Ib233b3e706cf6317858384264d5b0ed35657456e@changeid
    287422a9
rockchip_drm_vop.c 50.3 KB