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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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, &params);
> > > +		if (rcrtc->extclock)
> > > +			rcar_du_escr_divider(rcrtc, rcrtc->extclock, mode_clock,
> > > +					     ESCR_DCLKSEL_DCLKIN, &params);
> > >
> > > -			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


[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux