And for the records, I forgot to add
On Tue, Aug 21, 2018 at 12:35:48PM +0200, jacopo mondi wrote:
> Hi Laurent,
> I run some tests, and here below there's a summary of what I see
>
> On Tue, Aug 21, 2018 at 01:12:40AM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > 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.
>
> The following table compares results obtained with the latest
> renesas-drivers, with and without this series applied on top.
>
> -------------------------------------------------------------------------------
> Legend:
> A = image badly aligned: not all 4 blue/red borders visible
> F = flickering: disturbance in the shown image
> B = broken: mode is not displayed
>
> Results: I = improvement
> R = regression
>
> renesas-drivers du_clk result
> 1024x768
> 1920x1200 F A I
> 1920x1200 A A
> 1920x1080 A A
> 1600x1200 B I
> 1680x1050
> 1680x1050
> 1400x1050
> 1400x1050
> 1600x900 A A
> 1280x1024
> 1440x900
> 1440x900
> 1280x960
> 1366x768 A A
> 1366x768 A A
> 1360x768 A R
> 1280x800
> 1280x768 A A
> 1280x768 A A
> 1280x768 A A
> 1280x720 A A
> 800x600
> 800x600
> 848x480
> 640x480
> -------------------------------------------------------------------------------
>
> Overall I see two modes that were broken or unusable due to flickering
> (1600x1200 and 1920x1200 respectively) to be now (almost) fixed.
>
> There are visible alignement problems on some modes on both versions,
> but I only see one 'regression' (the last 1360x768 that is now
> slightly not aligned).
>
> I guess monitors play a role here, with each one being different, but
> overall I guess our test results match.
>
>
> >
> > > 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>
Acked-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
To your re-worked version of the patch!
Thanks
j
> > > ---
> > > 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)
> > > +{
> > > + 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,
> > > + * store the parameters.
> > > + */
> > > + if (diff < params->diff) {
> > > + params->clk = clk;
> > > + params->rate = rate;
> > > + params->diff = diff;
> > > + params->escr = escr | div;
> > > + }
> > > +}
> > > +
> > > static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
> > > { .soc_id = "r8a7795", .revision = "ES1.*" },
> > > { /* sentinel */ }
> > > @@ -225,42 +267,24 @@ static void rcar_du_crtc_set_display_timing(struct
> > > rcar_du_crtc *rcrtc)
> > >
> > > escr = ESCR_DCLKSEL_DCLKIN | div;
> > > } else {
> > > - unsigned long clk;
> > > - u32 div;
> > > -
> > > - /*
> > > - * Compute the clock divisor and select the internal or external
> > > - * dot clock based on the requested frequency.
> > > - */
> > > - clk = clk_get_rate(rcrtc->clock);
> > > - div = DIV_ROUND_CLOSEST(clk, mode_clock);
> > > - div = clamp(div, 1U, 64U) - 1;
> > > -
> > > - escr = ESCR_DCLKSEL_CLKS | div;
> > > -
> > > - if (rcrtc->extclock) {
> > > - unsigned long extclk;
> > > - unsigned long extrate;
> > > - unsigned long rate;
> > > - u32 extdiv;
> > > + struct du_clk_params params = { .diff = (unsigned long)-1 };
> > >
> > > - extclk = clk_get_rate(rcrtc->extclock);
> > > - extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
> > > - extdiv = clamp(extdiv, 1U, 64U) - 1;
> > > + rcar_du_escr_divider(rcrtc, rcrtc->clock, mode_clock,
> > > + ESCR_DCLKSEL_CLKS, ¶ms);
> > > + if (rcrtc->extclock)
> > > + rcar_du_escr_divider(rcrtc, rcrtc->extclock, mode_clock,
> > > + ESCR_DCLKSEL_DCLKIN, ¶ms);
> > >
> > > - extrate = extclk / (extdiv + 1);
> > > - rate = clk / (div + 1);
> > > + dev_dbg(rcrtc->group->dev->dev, "mode clock %lu %s rate %lu\n",
> > > + mode_clock, params.clk == rcrtc->clock ? "cpg" : "ext",
> > > + params.rate);
> > >
> > > - if (abs((long)extrate - (long)mode_clock) <
> > > - abs((long)rate - (long)mode_clock))
> > > - escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> > > -
> > > - dev_dbg(rcrtc->group->dev->dev,
> > > - "mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> > > - mode_clock, extrate, rate, escr);
> > > - }
> > > + clk_set_rate(params.clk, params.rate);
> > > + escr = params.escr;
> > > }
> > >
> > > + dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
> > > +
> > > rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
> > > escr);
> > > rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> >
> >
Attachment:
signature.asc
Description: PGP signature
