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

[Kieran Bingham]

Hi Laurent, Jacopo,

On 21/08/18 09:08, Laurent Pinchart wrote:
> 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 ? :-)

I'd probably write:

Store the parameters if the resulting frequency is better than any
previously calculated value.


>> 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
--
Kieran