On 18/09/2026 15:58, Lazar, Lijo wrote:
On 18-Sep-26 8:01 PM, Lazar, Lijo wrote:
On 18-Sep-26 7:55 PM, Lazar, Lijo wrote:
On 18-Sep-26 7:37 PM, Tvrtko Ursulin wrote:
On 18/09/2026 14:34, Lazar, Lijo wrote:
On 18-Sep-26 6:59 PM, Tvrtko Ursulin wrote:
On 18/09/2026 14:23, Lazar, Lijo wrote:
On 18-Sep-26 6:43 PM, Tvrtko Ursulin wrote:
On 18/09/2026 14:08, Lazar, Lijo wrote:
On 18-Sep-26 5:35 PM, Tvrtko Ursulin wrote:
Avoid constant register reloads while emitting IBs by using a
local write
pointer and only updating the size at the end of each helper.
Signed-off-by: Tvrtko Ursulin <[email protected]>
Cc: Timur Kristóf <[email protected]>
Cc: Alex Deucher <[email protected]>
---
v2:
* Fix decode buffer overwrite
Overall for this series, below type of calculation -
ib->length_dw = ptr - ib->ptr;
is not anticipating a case like below.
r = amdgpu_vcn_enc_get_create_msg(ring, 1, &ib, NULL);
if (r)
goto error;
r = amdgpu_vcn_enc_get_destroy_msg(ring, 1, &ib, &fence);
if (r)
goto error;
So amdgpu_vcn_enc_ring_test_ib() - are you saying there is a bug
there? Help me see it please.
ib->length_dw update is enclosed in both of those helpers, same
as before the patch.
What I'm saying is -
It's the same ib pointer that is passed to back to back calls.
Legacy implementation fills like -
ib->ptr[ib->length_dw++]
So no assumption that it is starting from offset 0 always. After
the function is filled, length gets updated based on what is filled.
I don't see the same with an assignment like
ib->length_dw = ptr - ib->ptr;
Hm, but both of those helpers emit separate jobs with their own
respective IB.
Wouldn't this kind of logic help to be on the safer side -
ib->length_dw += ptr - &ib->ptr[start_size];
I am not sure at which call sites you would suggest that?
I used the pattern of whoever writes to the IB starts their block with:
u32 *ptr = &ib->ptr[ib->length_dw];
Then they emit their stuff, and finally close with:
ib->length_dw = ptr - ib->ptr;
By that design callers are safe since all are self-contained. I at
the moment do not see how making that more complicated would make it
safer.
I would say all places where the function starts like below without
keeping any assumptions.
ib->ptr[ib->length_dw++] = <value>;
I don't see the complication with a code like this
ib->length_dw += ptr - &ib->ptr[ib->length_dw];
It keeps the same behavior as before.
As such the patch logic conflicts itself.
For this to be true -
ib->length_dw = ptr - ib->ptr;
It should start like.
u32 *ptr = ib->ptr;
Now it starts like
u32 *ptr = &ib->ptr[ib->length_dw];
So if you feel the new way is self contained, I would expect a change
here as well. Otherwise, this could be read as a bug.
Sorry for the noise. Got confused for a while; now I see what you meant.
No worries it happens to everyone! :)
Regards,
Tvrtko
