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@xxxxxxxxxx>
> >>
> >> 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@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@xxxxxxxxxx>
> >> [Factor out code to a helper function]
> >> Signed-off-by: Laurent Pinchart
> >> <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> >> ---
> >>
> >> 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
