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

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

 



HI Laurent,
   thanks for the patch rework

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.
>
> > 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?

> > +{
> > +	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,/ ?

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

Thanks
  j

> > +	 * 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