Watermark computation on i830 - potential bug?

Watermark computation on i830 - potential bug?

[Thomas Richter]

Hi Daniel, dear intel experts,

as reported yesterday, the watermark levels - the values of the FW_BLC 
register are completely off on my R31. This renders the display unusable 
after Daniel's patch from Friday, both the internal and the external. 
The fwater_lo values for planes A and B need to be *at least* 6, while 
the current algorithm sets them to one. Thus, the display flickers (now 
constantly) because the watermark values are wrong.

I now checked the code in intel_pm.c and I wonder how that actually 
works. The display becomes in my experiments *more stable* if I increase 
the watermark register value (i.e. FW_BLC), thus higher values refer to 
a higher watermark, i.e. the chipset starts fetching data earlier. So 
far my observation.

However, the way how intel_calculate_wm is written, it subtracts the 
number of necessary entries from the fifo size, and thus is written 
under the assumption that the FIFO drains in the direction of increasing 
entries. Thus, for the model used in intel_calculate_wm, *higher values* 
indicate a *lower watermark*, i.e. would instruct the
DMA engine to fetch data later.

This is in contradiction to my observation where higher values indicate
an *earlier* (and not a later) fetch.

Thus, is the definition of the FW_BLC register possibly simply wrong? Or 
is the subtraction in intel_calculate_wm possibly wrong?

Thanks,
	Thomas

Re: Watermark computation on i830 - potential bug?

[Daniel Vetter]

On Sat, Nov 16, 2013 at 9:57 PM, Thomas Richter <thor@math.tu-berlin.de> wrote:
> Hi Daniel, dear intel experts,
>
> as reported yesterday, the watermark levels - the values of the FW_BLC
> register are completely off on my R31. This renders the display unusable
> after Daniel's patch from Friday, both the internal and the external. The
> fwater_lo values for planes A and B need to be *at least* 6, while the
> current algorithm sets them to one. Thus, the display flickers (now
> constantly) because the watermark values are wrong.
>
> I now checked the code in intel_pm.c and I wonder how that actually works.
> The display becomes in my experiments *more stable* if I increase the
> watermark register value (i.e. FW_BLC), thus higher values refer to a higher
> watermark, i.e. the chipset starts fetching data earlier. So far my
> observation.
>
> However, the way how intel_calculate_wm is written, it subtracts the number
> of necessary entries from the fifo size, and thus is written under the
> assumption that the FIFO drains in the direction of increasing entries.
> Thus, for the model used in intel_calculate_wm, *higher values* indicate a
> *lower watermark*, i.e. would instruct the
> DMA engine to fetch data later.
>
> This is in contradiction to my observation where higher values indicate
> an *earlier* (and not a later) fetch.
>
> Thus, is the definition of the FW_BLC register possibly simply wrong? Or is
> the subtraction in intel_calculate_wm possibly wrong?

First: Have you tried my little patch, since the current watermark
code for i830M is clearly completely busted?

Otherwise the spec is fairly clear that lower values means to fetch
earlier. One issue though is that atm we hardcode the burst-lenght to
0x3, which means 8*32 bytes. If the watermark is lower than 8*32 then
the spec says that hilarity will ensue. I'll wip up a quick patch to
add some #defines to i915_reg.h and use them in the code for better
documentation.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: Watermark computation on i830 - potential bug?

[Thomas Richter]

On 17.11.2013 12:19, Daniel Vetter wrote:

>> as reported yesterday, the watermark levels - the values of the FW_BLC
>> register are completely off on my R31. This renders the display unusable
>> after Daniel's patch from Friday, both the internal and the external. The
>> fwater_lo values for planes A and B need to be *at least* 6, while the
>> current algorithm sets them to one. Thus, the display flickers (now
>> constantly) because the watermark values are wrong.
>>
>> I now checked the code in intel_pm.c and I wonder how that actually works.
>> The display becomes in my experiments *more stable* if I increase the
>> watermark register value (i.e. FW_BLC), thus higher values refer to a higher
>> watermark, i.e. the chipset starts fetching data earlier. So far my
>> observation.
>>
>> However, the way how intel_calculate_wm is written, it subtracts the number
>> of necessary entries from the fifo size, and thus is written under the
>> assumption that the FIFO drains in the direction of increasing entries.
>> Thus, for the model used in intel_calculate_wm, *higher values* indicate a
>> *lower watermark*, i.e. would instruct the
>> DMA engine to fetch data later.
>>
>> This is in contradiction to my observation where higher values indicate
>> an *earlier* (and not a later) fetch.
>>
>> Thus, is the definition of the FW_BLC register possibly simply wrong? Or is
>> the subtraction in intel_calculate_wm possibly wrong?
>
> First: Have you tried my little patch, since the current watermark
> code for i830M is clearly completely busted?

Yes. And it broke things completely. I send a report on Friday night. 
Now neither the display on the internal nor on the external screen are 
stable. I also mailed the register value of the watermark register, and 
the correct value.

> Otherwise the spec is fairly clear that lower values means to fetch
> earlier. One issue though is that atm we hardcode the burst-lenght to
> 0x3, which means 8*32 bytes. If the watermark is lower than 8*32 then
> the spec says that hilarity will ensue. I'll wip up a quick patch to
> add some #defines to i915_reg.h and use them in the code for better
> documentation.

Daniel, don't mind about the specs for the moment. I can only tell you 
what I see. The modified patch sets the watermark values to 1, the old 
code set them to five and left the watermark value for pipe B 
unmodified, which was the only reason why the internal display kept 
working with the old code. According to your understanding, this should 
be "better" (earlier refetch) but it is actually worse, not to say unusable.

With the new code, nothing works anymore. You need to set the watermark 
value to *at least* 6 on pipe A and pipe B to get a stable image, and 
the *lower* I set it, the less stable the display becomes. And not the 
*higher*. Thus, something is severely wrong here. Do you have the 
precise wording of the specs handy? This is probably a misinterpretation 
of what is stated there because it does not at all fit to the outcome of 
experiments.

If you like to, I can try to set the watermark to 0x3f (maximum) 
tomorrow and report back. According to the specs, this is the worst 
possible case, though my guess is that this will just work.

Greetings,
	Thomas

Re: Watermark computation on i830 - potential bug?

[Daniel Vetter]

On Sun, Nov 17, 2013 at 12:48 PM, Thomas Richter <thor@math.tu-berlin.de> wrote:
>> First: Have you tried my little patch, since the current watermark
>> code for i830M is clearly completely busted?
>
>
> Yes. And it broke things completely. I send a report on Friday night. Now
> neither the display on the internal nor on the external screen are stable. I
> also mailed the register value of the watermark register, and the correct
> value.

Well that never arrived, and there's also nothing in the moderation
queue. Or I've missed it since it wasn't a direct reply to my patch.

>> Otherwise the spec is fairly clear that lower values means to fetch
>> earlier. One issue though is that atm we hardcode the burst-lenght to
>> 0x3, which means 8*32 bytes. If the watermark is lower than 8*32 then
>> the spec says that hilarity will ensue. I'll wip up a quick patch to
>> add some #defines to i915_reg.h and use them in the code for better
>> documentation.
>
>
> Daniel, don't mind about the specs for the moment. I can only tell you what
> I see. The modified patch sets the watermark values to 1, the old code set
> them to five and left the watermark value for pipe B unmodified, which was
> the only reason why the internal display kept working with the old code.
> According to your understanding, this should be "better" (earlier refetch)
> but it is actually worse, not to say unusable.
>
> With the new code, nothing works anymore. You need to set the watermark
> value to *at least* 6 on pipe A and pipe B to get a stable image, and the
> *lower* I set it, the less stable the display becomes. And not the *higher*.
> Thus, something is severely wrong here. Do you have the precise wording of
> the specs handy? This is probably a misinterpretation of what is stated
> there because it does not at all fit to the outcome of experiments.

Like I've said I guess there's a lower limit - we need to have a
watermark which is at least the burst rate, which atm is set to 8*16
bytes.

> If you like to, I can try to set the watermark to 0x3f (maximum) tomorrow
> and report back. According to the specs, this is the worst possible case,
> though my guess is that this will just work.

Spec says:

[DevALM, DevMGM]: Display Plane B FIFO Watermark:
Number in SW (32Bs) of space in the Display Plane B FIFO above which
the Display B Stream will
generate requests to Memory (Value should be as recommended in the
high priority bandwidth analysis
spreadsheet).
Programming Note:
•
 This value should always be set assuming a 32bpp display mode, even
though the display
mode may be 15 or 16 bpp.
‘00000’ = 1 SW
...

And for burst rate:

[DevALM, DevMGM]: Display Plane B Burst Length:
Size in SW (32Bs) of individual requests issued to Memory for Display Plane B.
‘000’ = 4
‘001’ = 8
...

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch