Re: [PATCH 1/2] drm/xe: Add XE_BO_FLAG_NEEDS_WC_CPU and unify mapping for page tables.

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Fri, 20 Sep 2024 12:01:06 -0700, Maarten Lankhorst wrote:
>

Hey Maarten,

I will try to review as well as I can, but I am not very familiar with this
stuff, so ideally someone else should also review this. I also later
realized that this patch is not useful to the use case I had in mind.

Any case, going on.

> There are various places where we map buffers WC_CPU and uncached on the
> GPU. Unify all of those users to a single flag.

To me, this patch doesn't seem to "Unify all of those users to a single
flag" at all. The patch only adds XE_BO_FLAG_NEEDS_WC_CPU to
XE_BO_FLAG_PAGETABLE, not to XE_BO_FLAG_SCANOUT and
DRM_XE_GEM_CPU_CACHING_WC. So finally the only unifying thing seems to be
"ttm_write_combined".

The other issue maybe is that caching is specified through both
bo->cpu_caching as well as bo->flags. Also, 'bo->cpu_caching == 0' seems to
mean the same thing as '!(bo->flags & XE_BO_FLAG_USER)'. But maybe ok.

So maybe the commit message should just say we are exposing a new
XE_BO_FLAG_NEEDS_WC_CPU flag for internally created BO's to use which will
give "WC from CPU, UC from GPU" mapping. And using this new flag for
XE_BO_FLAG_PAGETABLE.

>
> In particular our usage of page table flags has been incoherent,
> and we should use uncached where applicable.

"uncached" meaning XE_BO_FLAG_NEEDS_WC_CPU?

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>

With the commit message change, the patch itself seems ok to me, except for
one question below.

> @@ -568,6 +577,7 @@ xe_pt_stage_bind_entry(struct xe_ptw *parent, pgoff_t offset,
>			xe_child->is_compact = true;
>		}
>
> +		pat_index = xe_pt_pat_index_from_bo(xe_child->bo);

This wasn't needed earlier? Didn't know what to make out of this new line
which appeared out of the blue.

Thanks.
--
Ashutosh