Re: [Intel-gfx] [PATCH] drm/i915/gt: Remove incorrect hard coded cache coherrency setting

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2240 bytes --]

Hi Fei,

Thanks for review. I put my answers inline below.

Regards,

Zhanjun

On 2023-06-22 6:20 p.m., Yang, Fei wrote:
> > The previouse i915_gem_object_create_internal already set it with 
> proper
> > value before function return. This hard coded setting is incorrect for
> > platforms like MTL, thus need to be removed.
> >
> > Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_timeline.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.c 
> b/drivers/gpu/drm/i915/gt/intel_timeline.c
> > index b9640212d659..693d18e14b00 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_timeline.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_timeline.c
> > @@ -26,8 +26,6 @@ static struct i915_vma *hwsp_alloc(struct intel_gt 
> *gt)
> >          if (IS_ERR(obj))
> >                  return ERR_CAST(obj);
> >
> > - i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
> > -
>
> Does this change really fix the coherency issue?

Testing in progress. Issue reported by E2E team, now is their public 
holiday. Meanwhile, I have trouble to run the test case on my setup. 
Need to sync with them later.


> I consulted with Chris and he said that the hwsp is purposely set to be
> cacheable. The mapping on CPU side also indicates it's cacheable,

For single end access area that setting works well. Here the problem is 
the head/tail memory area requires different cache setting.

As  the previous i915_gem_object_create_internal already set the cache 
setting for current platform properly, why we overwrite it here?


>
> intel_timeline_pin_map(struct intel_timeline *timeline)
>         {
>                 struct drm_i915_gem_object *obj = 
> timeline->hwsp_ggtt->obj;
>                 u32 ofs = offset_in_page(timeline->hwsp_offset);
>                 void *vaddr;
>
>                 vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
>                 ...
>         }
Maybe we should also set it to match platform as well?
>
> >          vma = i915_vma_instance(obj, &gt->ggtt->vm, NULL);
> >          if (IS_ERR(vma))
> >  i915_gem_object_put(obj);
> > --
> > 2.34.1
>

[-- Attachment #2: Type: text/html, Size: 43160 bytes --]