Re: [PATCH v7 08/10] accel/rocket: add RK3576 NPU (RKNN) support

[Igor Paunovic <royalnet026@gmail.com>]

Hi Jiaxing,

Thank you for withdrawing 40 and 72 before I built anything on them, and for
the field table. I checked the table against registers.xml and it is right in
all eighteen cells, including RESERVED_0 = 34, which is bits 12 and 16 and is
not obvious. I also confirmed the part of your reasoning that matters most:
your two words XOR to 0x100, and 0x124 against 0x80011111 and 0x024 against
0x80011011 both XOR to 0x80011035. The five-field offset really is common mode
across the two arms and cancels in an A against B.

One thing I cannot resolve on my own before I write code.

You wrote: "Use your own constant with bit 8 cleared instead, 0x124 where the
predicate is true and 0x024 where it is false." The merge request line you told
me to take instead of your branch reads

  R_DPU(0x4050, (DIV_ROUND_UP(output_channels, FEATURE_ATOMIC_SIZE) & 1)
                   ? 0x80011011 : 0x80011111);

which puts the SIZE_E_2 = 0 word on the true arm. Read with that predicate your
sentence gives me 0x024 at oc = 64, where your merge request emits 0x80011111
and current upstream emits 0x124, both SIZE_E_2 = 1. Read with "the predicate"
meaning output_channels % 32 == 0, your sentence is exactly your modulo form at
every count. Both readings agree at every count with oc mod 32 in 17..31, so
your five discriminating counts are unaffected either way. They disagree only at
oc mod 32 == 0 and 1..16 - and oc = 64 is the count you said you most want.

So, concretely, so that I do not guess: at oc = 64, do you want SIZE_E_2 = 1 or
SIZE_E_2 = 0? And if you meant your own form rather than the merge request's,
send me the expression.

I would rather we name the arms by value and by count than by the word
predicate, since it now refers to two different things in this thread.

Now the part I think is worth more than the A against B.

I went looking for what SIZE_E means rather than which value works, and I think
it falls out of the padding. In upstream rkt_task.c the output channel count is
padded to a multiple of 32 for the normal case (line 87,
align(MAX2(oc, 32), 32)) and to 64 for depthwise (lines 88-91), and
FEATURE_ATOMIC_SIZE is 16 (rkt_ml.h:18). So the padded bank is 2 atoms of 16 in
the normal case and 4 in the depthwise case. Upstream writes SIZE_E = 1 and
SIZE_E = 3 respectively. That is 2 - 1 and 4 - 1.

  SIZE_E_n + 1 = the number of 16-channel atoms in the final output channel bank

If that reading is right, your parity predicate is not a fit at all, it is this
same statement: I checked over oc = 1..512 with no exceptions that
DIV_ROUND_UP(oc, 16) being odd is exactly the condition that the final
32-channel bank holds one atom rather than two. Odd atom count means a
half-populated last bank means SIZE_E_2 should be 0. That is the reason you said
on 16 August you wanted for the field, and it is arithmetically identical to
what you already wrote.

It also explains why my eight counts said nothing, which had been bothering me.
Upstream hands the padded count to the write path, not the real one -
rkt_regcmd.c lines 201, 226 and 247 all take task->output_channels, and only
ORIG_CHANNEL at line 225 takes output_channels_real. So on RK3588 the final bank
is always declared full, the atom count is always 2, and SIZE_E_2 = 1 is simply
correct at every remainder. My five new counts could not have discriminated
anything; they were confirming that upstream never under-declares the last bank.

And it predicts your losses arithmetically rather than describing them. An
under-declared last bank keeps floor(oc/32) * 32 channels, which is 32, 64 and
96 at oc = 56, 88 and 120 - your three numbers, including the lost ranges 32-55,
64-87 and 96-119.

Please treat that as a hypothesis. I derived it from the encoder's arithmetic,
not from a TRM or a vendor document, and it assumes the three SIZE_E fields are
independent, which is the same assumption you flagged. It also does not touch
the timeout, and I agree with you that the timeout is unexplained.

What I will run, once the polarity above is settled:

  - oc = 64 both ways with the constant forced, no predicate in the build at
    all, so the ambiguity cannot reach it. I will patch only the non-depthwise
    branch at rkt_regcmd.c:239-241; clearing bit 8 in the depthwise arm would be
    0x36c -> 0x26c, SIZE_E 3 -> 2, a different move. I will verify the emitted
    word from the submitted register stream rather than the source, because at
    oc = 64 real and padded are equal and a build that read the wrong one would
    be invisible.
  - oc = 20 and oc = 60 on stock upstream as the reference arm. Those need no
    patch and no answer from you, so I can send them whenever. They also add
    residues 20 and 28 to the table, which so far only has 1, 4, 8, 16 and 24.

Generator and scorer are on the way in a separate mail.

Two small things. Your sentence "modulo true implies parity true and never the
reverse" is the wrong way round - parity true is residues 1..16, which is a
subset of not-a-multiple-of-32, so the implication runs the other way. Nothing
downstream of it changes. And your five are a sample rather than the set: below
128 there are sixty counts with oc mod 32 in 17..31, and odd ones are fine since
33 already ran, so 17, 25, 51, 83 and 117 would do just as well as 20 and 50.

Regards,
Igor