| Message ID | 20230622152644.169400-1-zhanjun.dong@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gt: Remove incorrect hard coded cache coherrency setting | expand |
Resend to restart the CI, https://patchwork.freedesktop.org/series/119485/ Was stuck. Regards, Zhanjun On 2023-06-22 11:26 a.m., Zhanjun Dong 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); > - > vma = i915_vma_instance(obj, >->ggtt->vm, NULL); > if (IS_ERR(vma)) > i915_gem_object_put(obj);
> 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? 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, 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); ... } > vma = i915_vma_instance(obj, >->ggtt->vm, NULL); > if (IS_ERR(vma)) > i915_gem_object_put(obj); > -- > 2.34.1
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, >->ggtt->vm, NULL); > > if (IS_ERR(vma)) > > i915_gem_object_put(obj); > > -- > > 2.34.1 >
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); - vma = i915_vma_instance(obj, >->ggtt->vm, NULL); if (IS_ERR(vma)) i915_gem_object_put(obj);
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(-)