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

[Nirmoy Das <nirmoy.das@linux.intel.com>]

On 5/21/2024 5:32 PM, Matthew Brost wrote:
> On Tue, May 21, 2024 at 03:52:45PM +0100, Matthew Auld wrote:
>> 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.
>>
> Ah, yes. The comment below explains this. The this function is called on
> the parent while operating on the child. So to write 4k page entries the
> level would be 1.
>
> This LGTM.
Thanks. Merged to drm-xe-next.
>
> Matt
>
>>> 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
>>>>