Re: [PATCH] Added a lower limit for the watermark setting

[Thomas Richter <thor@math.tu-berlin.de>]

Am 20.11.2013 10:27, schrieb Daniel Vetter:
> What I've meant to say is that I want to split up the watermark code
> more anyway, so that there's no need to fill in the 0 all over the
> place where we don't care one bit. I.e. all the gen4+ platforms.

Ok - well, I guess I cannot judge whether that's necessary or required. 
Given that the i830 is the only chipset
that comes with a unified FIFO, it might be actually necessary.
>
>>> I've pushed out my current (and rather broken) wip branch with my idea on
>>> the take to
>>>
>>> http://cgit.freedesktop.org/~danvet/drm/log/?h=for-thomas
>>
>> Could you please help me here how to apply it? I'm not very experienced with
>> git, and it does not seem to fit to the sources of
>> intel_pm.c I have. Do I first need to instruct git to download another
>> branch? I'm currently at drm-intel-nightly.
> It's more just to read through the patches for ideas. Atm the branch
> doesn't even compile - I'll try to clean it up and beat it into shape
> in the next few days. The core idea is to have a minimal wm of 4
> (lowest burst setting) and then set the actual burst setting to the
> watermark rounded down to the next multiple of 4, up to the
> recommended value in Bspec (which is 16 fifo cachelines). But I've
> fumbled the job a bit and broke a few things ;-)

Ah, that explains something, thank you. Probably two things: The way how the
current code is organized, the wm handling of the i830 is handled in two 
functions,
not one. In i830_update_wm and i9xx_update_wm. The former seems to be 
used during bootstrap
when only one pipe is active, the latter as soon as I activate the 
second pipe. This is probably needlessly
complex, and it would be better to have this just in one place, not two 
(found that when manually
adding a lower limit as first attempt).

The second is concerned the lower limit: Four is not enough, six gives a 
"reasonably stable image" most the time,
but if scrolling wildly, still some hickups. Eight is better, but still 
not perfect. If you try really, really hard, one can
still provoke a crash if a video overlay is active. Unclear why this is 
- if it happens, the screen seems to go blank for
just one frame, then recovers. Given that the plane pointers are only 
shadow registers that are automatically updated
by the chip on a vblank, I have no good idea why that happens.

I also see that your code seems to use a modified formula for the 
"latency". Given that I cannot compile the code, it's
at this time hard to say whether that's better or worse. The current one 
is "too pessimistic". I tried to find the minimum
permissible latency and find that for a pixel clock of 135Mhz, the 
watermark should be at most 30, and for a pixel clock
of 108 Mhz, the watermark should be at most 32. From this one can 
compute the latencies in ns. If I did everything
right, I get values of approximately 1185 ns and 1066 ns, thus 
approximately in the same ball park. A value of 1500
should likely be fine, but requires testing on additional hardware (I've 
currently no access to the Fujistu, the other laptop
with a i830 chipset and the strange DVO).

BTW, even with the maximal watermarks (minimum latency), I do get 
"hickups" if I try really really hard. So they are
likely provoked by something else, not the wrong watermark.

Thanks!

Thomas