Re: [PATCH 3/3] drm/amdkfd: gfx12 context save/restore trap handler fixes

[Jay Cornwall <jay.cornwall@amd.com>]

On 5/23/2024 13:37, Lancelot SIX wrote:

>> @@ -622,8 +638,15 @@ L_SAVE_HWREG:
>>   #if ASIC_FAMILY >= CHIP_GFX12
>>       // Ensure no further changes to barrier or LDS state.
>> +    // STATE_PRIV.BARRIER_COMPLETE may change up to this point.
>>       s_barrier_signal    -2
>>       s_barrier_wait    -2
>> +
>> +    // Re-read final state of BARRIER_COMPLETE field for save.
>> +    s_getreg_b32    s_save_tmp, hwreg(S_STATUS_HWREG)
>> +    s_and_b32    s_save_tmp, s_save_tmp, 
>> SQ_WAVE_STATE_PRIV_BARRIER_COMPLETE_MASK
>> +    s_andn2_b32    s_save_status, s_save_status, 
>> SQ_WAVE_STATE_PRIV_BARRIER_COMPLETE_MASK
> 
> Even if BARRIER_COMPLETE can be asserted while we are in the trap 
> hadler, I do not think it can be cleared.  That being said, it might be 
> easier to just replace the bit, making it clearer.

Yes, I chose to structure it this way to make the intent clearer. We 
don't gain much from dropping the s_andn2. Most of the time spent in the 
save handler is stalled on memory instructions.

>> @@ -1351,7 +1369,17 @@ L_SKIP_BARRIER_RESTORE:
>>       s_setreg_b32    hwreg(HW_REG_SHADER_XNACK_MASK), 
>> s_restore_xnack_mask
>>   #endif
>> +#if ASIC_FAMILY < CHIP_GFX12
>>       s_setreg_b32    hwreg(S_TRAPSTS_HWREG), s_restore_trapsts
> 
> Wouldn't other gfx1x architectures have a similar issue when writing 
> TRAPSTS here?  That is if TRAPSTS.SAVECTX is set while we are restoring, 
> wouldn't we loose it?
> 
> And for gfx11, there is TRAPSTS.HOST_TRAP that could have the same issue 
> to some degree (not sure if we would loose the host trap completly, or 
> re-enter with trap ID + HT bit set in ttmp1).

Prior to gfx12 context save and host trap exceptions are not delivered 
to a wave until STATUS.PRIV=0, i.e. it leaves the trap handler.

The changes needed for gfx12 are due to a design change in this area. 
Exceptions are now flagged immediately and cause re-entry to the trap if 
any are non-zero.