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.Ib233b3e706cf6317858384264d5b0...
-Doug