[PATCH] drm: rcar-du: Improve non-DPLL clock selection

[Laurent Pinchart]

Hi Jacopo,

(CC'ing Kieran)

On Tuesday, 21 August 2018 10:33:57 EEST jacopo mondi wrote:
> On Tue, Aug 21, 2018 at 01:12:40AM +0300, Laurent Pinchart wrote:
> > On Tuesday, 21 August 2018 00:49:41 EEST Laurent Pinchart wrote:
> >> From: Jacopo Mondi <jacopo at jmondi.org>
> >> 
> >> DU channels not equipped with a DPLL use an SoC internal (provided by
> >> the CPG) or external clock source combined with a DU internal divider to
> >> generate the desired output dot clock frequency.
> >> 
> >> The current clock selection procedure does not fully exploit the ability
> >> of external clock sources to generate the exact dot clock frequency by
> >> themselves, but relies instead on tuning the internal DU clock divider
> >> only, resulting in a less precise clock generation process.
> >> 
> >> When possible, and desirable, ask the external clock source for the
> >> exact output dot clock frequency, and select the clock source that
> >> produces the frequency closest to the desired output dot clock.
> >> 
> >> This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
> >> where the DU's input dotclock.in is generated by the versaclock VC5
> >> clock source, which is capable of generating the exact rate the DU needs
> >> as pixel clock output.
> >> 
> >> This patch fixes higher resolution modes which requires an high pixel
> >> clock output currently not working on non-HDMI DU channel (such as
> >> 1920x1080 at 60Hz on the VGA output).
> > 
> > Just for the record, with this patch the following modes (as printed by
> > 
> > modetest) on the VGA output now produce correct result with my monitor:
> >   1920x1080 60 1920 2008 2052 2200 1080 1084 1089 1125 flags: nhsync,
> >   nvsync
> >   1600x1200 60 1600 1664 1856 2160 1200 1201 1204 1250 flags: phsync,
> >   pvsync
> >   1360x768 60 1360 1424 1536 1792 768 771 777 795 flags: phsync, pvsync
> > 
> > The second mode used to not display at all, with a message telling that
> > timings were out of range, and the other two modes used to produce a
> > displayed image partly shifted or scaled out of the screen boundaries.
> > 
> > The following modes still produce an image partly out of the screen
> > boundaries.
> > 
> >   1600x900 60 1600 1624 1704 1800 900 901 904 1000 flags: phsync, pvsync
> >   1280x960 60 1280 1376 1488 1800 960 961 964 1000 flags: phsync, pvsync
> >   1366x768 60 1366 1436 1579 1792 768 771 774 798 flags: phsync, pvsync
> >   1366x768 60 1366 1380 1436 1500 768 769 772 800 flags: phsync, pvsync
> >   1280x720 60 1280 1390 1430 1650 720 725 730 750 flags: phsync, pvsync
> > 
> > And this one is reported to be out of range.
> > 
> >   1920x1200 60 1920 2056 2256 2592 1200 1203 1209 1245 flags: nhsync,
> >   pvsync
> > 
> > There is thus still room for improvement (some of the issues are possibly
> > due to my monitor though), but there's also an improvement, and no
> > noticeable regression.
> > 
> >> Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel
> >> clock")
> >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >> [Factor out code to a helper function]
> >> Signed-off-by: Laurent Pinchart
> >> <laurent.pinchart+renesas at ideasonboard.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 +++++++++++++++++++---------
> >>  1 file changed, 55 insertions(+), 31 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index
> >> f8068170905a..2c9405458bbf
> >> 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> @@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct
> >> rcar_du_crtc *rcrtc,
> >>  	best_diff);
> >>  }
> >> 
> >> +struct du_clk_params {
> >> +	struct clk *clk;
> >> +	unsigned long rate;
> >> +	unsigned long diff;
> >> +	u32 escr;
> >> +};
> >> +
> >> +static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk
> >> *clk,
> >> +				 unsigned long target, u32 escr,
> >> +				 struct du_clk_params *params)
> 
> I don't see the rcrtc parameter ever being used in this function.
> Do you want to keep it anyhow?

You're right, I'll remove it.

> >> +{
> >> +	unsigned long rate;
> >> +	unsigned long diff;
> >> +	u32 div;
> >> +
> >> +	/*
> >> +	 * If the target rate has already been achieved perfectly we can't do
> >> +	 * better.
> >> +	 */
> >> +	if (params->diff == 0)
> >> +		return;
> >> +
> >> +	/*
> >> +	 * Compute the input clock rate and internal divisor values to obtain
> >> +	 * the clock rate closest to the target frequency.
> >> +	 */
> >> +	rate = clk_round_rate(clk, target);
> >> +	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
> >> +	diff = abs(rate / (div + 1) - target);
> >> +
> >> +	/*
> >> +	 * If the resulting frequency is better than any previously obtained,
> 
> s/obtained,/obtained one,/ ?

Any opinion from a native English speaker ? :-)

> Will get back with some testing results on a different VGA monitor...

Thank you.

> >> +	 * store the parameters.
> >> +	 */
> >> +	if (diff < params->diff) {
> >> +		params->clk = clk;
> >> +		params->rate = rate;
> >> +		params->diff = diff;
> >> +		params->escr = escr | div;
> >> +	}
> >> +}

[snip]

-- 
Regards,

Laurent Pinchart