pevik_ has quit [Remote host closed the connection]
pevik_ has joined #linux-msm
pevik_ has quit [Remote host closed the connection]
pevik_ has joined #linux-msm
pevik_ has quit [Remote host closed the connection]
pespin has joined #linux-msm
pevik_ has joined #linux-msm
pespin_ has joined #linux-msm
pespin has quit [Ping timeout: 480 seconds]
jhovold has quit [Ping timeout: 480 seconds]
<calebccff>
elder___: Just rebased on -rc2 with your new generalised register patches and got some splats in ipa_reg_encode(), I sent a patch which i believe fixes it (works for me on 845 at least). But I'm curiouts as to how it worked before
<elder___>
OK, thank you. I have not been able to test on 845 for a while, so I'm glad to hear about this
<elder___>
Can you send me what you found?
<calebccff>
This is the splat: https://p.calebs.dev/f5d4b5@raw, my annotation of ipa_resource.c:83 is wrong, it's actually line 80
<calebccff>
Previously it seems to have been using u32_encode_bits() which I guess only complains about values being outside the mask if they're known at compile time
<elder___>
That's correct, but they had to be known at compile time
<elder___>
calebccff: I'm in the middle of two things right now. I glanced at the code but I'm going to need to clear my head to look at this more closely
<elder___>
Let me get back to you shortly
<calebccff>
elder___: no hurry! thanks :)
<elder___>
Oh I see your patch
<elder___>
That's interesting. I'm using numbers there that come straight from downstream...
<elder___>
And I guess the new code actually checks things a little more tightly
<calebccff>
heh, yeah, u32_encode_bits uses __builtin_constant_p to check if the value is known, which would be false for the ipa_data, so the range check wouldn't happen
<calebccff>
hopefully those reserved bits don't do anything...
<elder___>
They shouldn't...
<elder___>
I have seen this frequently with the downstream code.
<elder___>
Some bits are set when they aren't valid, for example, and the hardware ignores them, but in my view it's incorrect code.
<elder___>
calebccff: if I understand the problem you've identified, the X_MIN_LIM (etc.) fields (defined in ipa_reg/ipa_reg-v3.5.1.c) are only 6 bits wide for IPA v3.5.1. But the values we try to store in those fields (defined in ipa_data/ipa_data-v3.5.1.c) is 255, which can't be represented in 6 bits. Correct?
<elder___>
Your patch updates ipa_data-v3.1.c as well. Does that mean you verified this in the code (the corresponding two source files for 3.1), or do you have a 3.1 device that you tested it on?
<elder___>
It would be nice to be sure that *none* of the upstream-defined IPA versions has this issue. Did you happen to do that?
<calebccff>
elder___: I just grep'd for ".max = 255" and noticed that only 3.1 and 3.5.1 matched, I'll reply to your email and send a new patch
<elder___>
Thank you. Maybe two patches
pevik has quit [Ping timeout: 480 seconds]
pespin_ has quit [Remote host closed the connection]
<calebccff>
elder___: hopefully the last one, Jami reported an additional splat on msm8998: https://termbin.com/wg7b, I've narrowed it down to the IDLE_INDICATION_CFG register which is not supported on v3.1 but doesn't have a version check before it
<elder___>
Well that's a great find
<elder___>
It's so nice to have someone else reading this code.
<elder___>
But I'm also glad that making things a little more restrictive is reporting problems like this
<calebccff>
it seems like it should make it a lot easier to catch regressions in the future, it's definitely good to have the extra safety
<elder___>
Yes. But for now we'll see these splats (hopefully not for long)