[Intel-xe] [PATCH 1/9] drm/xe: Normalize pte/pde encoding

Tue Sep 26 13:43:20 UTC 2023

On Mon, Sep 25, 2023 at 03:30:06PM -0700, Matt Roper wrote:
>On Mon, Sep 25, 2023 at 03:10:41PM -0700, Lucas De Marchi wrote:
>> Split functions that do only part of the pde/pte encoding and that can
>> be called by the different places. This normalizes how pde/pte are
>> encoded so they can be moved elsewhere in a subsequent change.
>>
>> xe_pte_encode() was calling __pte_encode() with a NULL vma, which is the
>> opposite of what xe_pt_stage_bind_entry() does. Stop passing a NULL vma
>> and just split another function that deals with a vma rather than a bo.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_pt.c | 119 +++++++++++++++++++++----------------
>>  1 file changed, 67 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
>> index aec469842471..afadd399684c 100644
>> --- a/drivers/gpu/drm/xe/xe_pt.c
>> +++ b/drivers/gpu/drm/xe/xe_pt.c
>> @@ -47,91 +47,106 @@ static struct xe_pt *xe_pt_entry(struct xe_pt_dir *pt_dir, unsigned int index)
>>  	return container_of(pt_dir->dir.entries[index], struct xe_pt, base);
>>  }
>>
>> -/**
>> - * xe_pde_encode() - Encode a page-table directory entry pointing to
>> - * another page-table.
>> - * @bo: The page-table bo of the page-table to point to.
>> - * @bo_offset: Offset in the page-table bo to point to.
>> - * @level: The cache level indicating the caching of @bo.
>> - *
>> - * TODO: Rename.
>> - *
>> - * Return: An encoded page directory entry. No errors.
>> - */
>> -u64 xe_pde_encode(struct xe_bo *bo, u64 bo_offset,
>> -		  const enum xe_cache_level level)
>> +static u64 pde_encode_cache(enum xe_cache_level cache)
>>  {
>> -	u64 pde;
>> -
>> -	pde = xe_bo_addr(bo, bo_offset, XE_PAGE_SIZE);
>> -	pde |= XE_PAGE_PRESENT | XE_PAGE_RW;
>> -
>>  	/* FIXME: I don't think the PPAT handling is correct for MTL */
>>
>> -	if (level != XE_CACHE_NONE)
>> -		pde |= PPAT_CACHED_PDE;
>> -	else
>> -		pde |= PPAT_UNCACHED;
>> +	if (cache != XE_CACHE_NONE)
>> +		return PPAT_CACHED_PDE;
>>
>> -	return pde;
>> +	return PPAT_UNCACHED;
>>  }
>>
>> -static u64 __pte_encode(u64 pte, enum xe_cache_level cache,
>> -			struct xe_vma *vma, u32 pt_level)
>> +static u64 pte_encode_cache(enum xe_cache_level cache)
>>  {
>> -	pte |= XE_PAGE_PRESENT | XE_PAGE_RW;
>> -
>> -	if (unlikely(vma && xe_vma_read_only(vma)))
>> -		pte &= ~XE_PAGE_RW;
>> -
>> -	if (unlikely(vma && xe_vma_is_null(vma)))
>> -		pte |= XE_PTE_NULL;
>> -
>>  	/* FIXME: I don't think the PPAT handling is correct for MTL */
>> -
>>  	switch (cache) {
>>  	case XE_CACHE_NONE:
>> -		pte |= PPAT_UNCACHED;
>> -		break;
>> +		return PPAT_UNCACHED;
>>  	case XE_CACHE_WT:
>> -		pte |= PPAT_DISPLAY_ELLC;
>> -		break;
>> +		return PPAT_DISPLAY_ELLC;
>>  	default:
>> -		pte |= PPAT_CACHED;
>> -		break;
>> +		return PPAT_CACHED;
>>  	}
>> +}
>> +
>> +static u64 pte_encode_ps(u32 pt_level)
>> +{
>> +	/* XXX: Does hw support 1 GiB pages? */
>> +	XE_WARN_ON(pt_level > 2);
>>
>>  	if (pt_level == 1)
>> -		pte |= XE_PDE_PS_2M;
>> +		return XE_PDE_PS_2M;
>>  	else if (pt_level == 2)
>> -		pte |= XE_PDPE_PS_1G;
>> +		return XE_PDPE_PS_1G;
>>
>> -	/* XXX: Does hw support 1 GiB pages? */
>> -	XE_WARN_ON(pt_level > 2);
>> +	return 0;
>> +}
>>
>> -	return pte;
>> +/**
>> + * xe_pde_encode() - Encode a page-table directory entry pointing to
>> + * another page-table.
>> + * @bo: The page-table bo of the page-table to point to.
>> + * @bo_offset: Offset in the page-table bo to point to.
>> + * @cache: The cache level indicating the caching of @bo.
>> + *
>> + * TODO: Rename.
>> + *
>> + * Return: An encoded page directory entry. No errors.
>> + */
>> +u64 xe_pde_encode(struct xe_bo *bo, u64 bo_offset,
>> +		  const enum xe_cache_level cache)
>> +{
>> +	u64 pde;
>> +
>> +	pde = xe_bo_addr(bo, bo_offset, XE_PAGE_SIZE);
>> +	pde |= XE_PAGE_PRESENT | XE_PAGE_RW;
>> +	pde |= pde_encode_cache(cache);
>> +
>> +	return pde;
>>  }
>>
>>  /**
>>   * xe_pte_encode() - Encode a page-table entry pointing to memory.
>>   * @bo: The BO representing the memory to point to.
>> - * @offset: The offset into @bo.
>> + * @bo_offset: The offset into @bo.
>>   * @cache: The cache level indicating
>>   * @pt_level: The page-table level of the page-table into which the entry
>>   * is to be inserted.
>>   *
>>   * Return: An encoded page-table entry. No errors.
>>   */
>> -u64 xe_pte_encode(struct xe_bo *bo, u64 offset, enum xe_cache_level cache,
>> +u64 xe_pte_encode(struct xe_bo *bo, u64 bo_offset, enum xe_cache_level cache,
>>  		  u32 pt_level)
>>  {
>>  	u64 pte;
>>
>> -	pte = xe_bo_addr(bo, offset, XE_PAGE_SIZE);
>> +	pte = xe_bo_addr(bo, bo_offset, XE_PAGE_SIZE);
>> +	pte |= XE_PAGE_PRESENT | XE_PAGE_RW;
>> +	pte |= pte_encode_cache(cache);
>> +	pte |= pte_encode_ps(pt_level);
>> +
>>  	if (xe_bo_is_vram(bo) || xe_bo_is_stolen_devmem(bo))
>>  		pte |= XE_PPGTT_PTE_DM;
>>
>> -	return __pte_encode(pte, cache, NULL, pt_level);
>> +	return pte;
>> +}
>> +
>> +/* Like xe_pte_encode(), but with a vma and a partially-encoded pte */
>> +static u64 __vma_pte_encode(u64 pte, struct xe_vma *vma,
>> +			    enum xe_cache_level cache, u32 pt_level)
>> +{
>> +	pte |= XE_PAGE_PRESENT | XE_PAGE_RW;
>> +	pte |= pte_encode_cache(cache);
>> +	pte |= pte_encode_ps(pt_level);
>> +
>> +	if (unlikely(vma && xe_vma_read_only(vma)))
>
>Do we need the non-NULL check part of this condition (ditto for the one
>below)?  With your refactoring, the vma parameter passed to this
>function should already be non-NULL, right?  In fact we've already
>dereferenced it before the only callsite of this function...

yeah... I think the goal of this patch was accomplished: to become
obvious we can't have a NULL vma at this point. I see you noticed it
later in patch 2 ;)

>
>Aside from that,
>
>Reviewed-by: Matt Roper <matthew.d.roper at intel.com>

thanks
Lucas De Marchi

>
>> +		pte &= ~XE_PAGE_RW;
>> +
>> +	if (unlikely(vma && xe_vma_is_null(vma)))
>> +		pte |= XE_PTE_NULL;
>> +
>> +	return pte;
>>  }
>>
>>  static u64 __xe_pt_empty_pte(struct xe_tile *tile, struct xe_vm *vm,
>> @@ -614,9 +629,9 @@ xe_pt_stage_bind_entry(struct xe_ptw *parent, pgoff_t offset,
>>
>>  		XE_WARN_ON(xe_walk->va_curs_start != addr);
>>
>> -		pte = __pte_encode(is_null ? 0 :
>> -				   xe_res_dma(curs) + xe_walk->dma_offset,
>> -				   xe_walk->cache, xe_walk->vma, level);
>> +		pte = __vma_pte_encode(is_null ? 0 :
>> +				       xe_res_dma(curs) + xe_walk->dma_offset,
>> +				       xe_walk->vma, xe_walk->cache, level);
>>  		pte |= xe_walk->default_pte;
>>
>>  		/*
>> --
>> 2.40.1
>>
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation