Re: [PATCH] drm/xe: Add warn when level can not be zero.

[Matthew Auld <matthew.auld@intel.com>]

On 21/05/2024 15:08, Matthew Brost wrote:
> On Tue, May 21, 2024 at 12:36:23PM +0200, Nirmoy Das wrote:
>> At xe_pt_zap_ptes_entry() and xe_pt_stage_unbind_entry, the level cannot
>> be 0. Therefore, add an independent check for the level. Since the level
>> cannot be zero at this point, there is no need to check for `is_compact`,
>> so remove that instead.
>>
> 
> This doesn't look right. Both xe_pt_zap_ptes_entry &
> xe_pt_stage_unbind_entry can be at level 0 is 4K page entries are used,
> right? CI looks good though so confused by that. I think maybe 2
> independent VMAs would have to mapped within a 2M range for these paths
> to decend to level 0. Maybe we don't have tests in place that do this.

Not too sure, but in both cases this is followed by doing a level-1 
which is then used as the index into some array AFAICT. So if level can 
indeed by zero here then that would be a serious bug. Improving the 
assert here to catch that looked reasonable to me.

> 
> Regardless please don't merge this until my concerns are addresesed.
> 
> Matt
> 
>> Cc: Matthew Auld <matthew.auld@intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_pt.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
>> index 11dd0988ffda..cd60c009b679 100644
>> --- a/drivers/gpu/drm/xe/xe_pt.c
>> +++ b/drivers/gpu/drm/xe/xe_pt.c
>> @@ -763,7 +763,7 @@ static int xe_pt_zap_ptes_entry(struct xe_ptw *parent, pgoff_t offset,
>>   	pgoff_t end_offset;
>>   
>>   	XE_WARN_ON(!*child);
>> -	XE_WARN_ON(!level && xe_child->is_compact);
>> +	XE_WARN_ON(!level);
>>   
>>   	/*
>>   	 * Note that we're called from an entry callback, and we're dealing
>> @@ -1445,7 +1445,7 @@ static int xe_pt_stage_unbind_entry(struct xe_ptw *parent, pgoff_t offset,
>>   	struct xe_pt *xe_child = container_of(*child, typeof(*xe_child), base);
>>   
>>   	XE_WARN_ON(!*child);
>> -	XE_WARN_ON(!level && xe_child->is_compact);
>> +	XE_WARN_ON(!level);
>>   
>>   	xe_pt_check_kill(addr, next, level - 1, xe_child, action, walk);
>>   
>> -- 
>> 2.42.0
>>