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

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

[Zhanjun Dong]

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, &gt->ggtt->vm, NULL);
 	if (IS_ERR(vma))
 		i915_gem_object_put(obj);
-- 
2.34.1

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

[Nirmoy Das]

Hi Zhanjun,


please restart the CI, https://patchwork.freedesktop.org/series/119485/

It seems to be stuck.



On 6/17/2023 12:11 AM, 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>
Reviewed-by: Nirmoy Das <nirmoy.das@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, &gt->ggtt->vm, NULL);
>   	if (IS_ERR(vma))
>   		i915_gem_object_put(obj);

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

[Zhanjun Dong]

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, &gt->ggtt->vm, NULL);
 	if (IS_ERR(vma))
 		i915_gem_object_put(obj);
-- 
2.34.1

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

[Dong, Zhanjun]

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, &gt->ggtt->vm, NULL);
>   	if (IS_ERR(vma))
>   		i915_gem_object_put(obj);

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

[Yang, Fei]

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

> 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, &gt->ggtt->vm, NULL);
>          if (IS_ERR(vma))
>                  i915_gem_object_put(obj);
> --
> 2.34.1


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

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

[Dong, Zhanjun]

[-- 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 --]