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@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 ? :-)
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
