Re: [Intel-gfx] [PATCH v2 0/5] drm/i915: Allow user to set cache at BO creation

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 26/04/2023 16:41, Yang, Fei wrote:
>  > On 26/04/2023 07:24, fei.yang@intel.com wrote:
>  >> From: Fei Yang <fei.yang@intel.com>
>  >>
>  >> The first three patches in this series are taken from
>  >> https://patchwork.freedesktop.org/series/116868/
>  >> These patches are included here because the last patch
>  >> has dependency on the pat_index refactor.
>  >>
>  >> This series is focusing on uAPI changes,
>  >> 1. end support for set caching ioctl [PATCH 4/5]
>  >> 2. add set_pat extension for gem_create [PATCH 5/5]
>  >>
>  >> v2: drop one patch that was merged separately
>  >>      341ad0e8e254 drm/i915/mtl: Add PTE encode function
>  >
>  > Are the re-sends for stabilizing the series, or focusing on merge?
> 
> v2 was sent just to drop one of patches that has already been merged.
> 
>  > If the latter then opens are:
>  >
>  > 1) Link to Mesa MR reviewed and ready to merge.
>  >
>  > 2) IGT reviewed.
>  >
>  > 3) I raised an open that get/set_caching should not "lie" but return an
>  > error if set pat extension has been used. I don't see a good reason not
>  > to do that.
> 
> I don't think it's "lying" to the userspace. the comparison is only valid
> for objects created by KMD because only such object uses the pat_index
> from the cachelevel_to_pat table.

Lets double check my understanding is correct. Userspace sequence of 
operations:

1)
obj = gem_create()+set_pat(PAT_UC)

2a)
get_caching(obj)

What gets reported?

2b)
set_caching(obj, I915_CACHE_LLC)

What is the return code?

If answer to 2a is I915_CACHING_CACHED and to 2b) success, then please 
state a good reason why both shouldn't return an error.

> 
>  > + Joonas on this one.
>  >
>  > 4) Refactoring as done is not very pretty and I proposed an idea for a
>  > nicer approach. Feasible or not, open for discussion.
> 
> Still digesting your proposal. but not sure how would you define things
> like PAT_UC as that is platform dependent, not a constant.

"PAT_UC" in my outline was purely a driver specific value, similarly as 
I915_CACHE_... are. With the whole point that driver can ask if 
something is uncached, WT or whatever. Using the platform specific 
mapping table which converts platform specific obj->pat_index to driver 
representation of caching modes (PAT_UC etc).

Quite possible I missed some detail, but if I haven't then it could be 
a neat and lightweight solution.

Regards,

Tvrtko