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

"Dong, Zhanjun" <zhanjun.dong@xxxxxxxxx> · Fri, 23 Jun 2023 14:02:01 -0400

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@xxxxxxxxx>
        > ---
        >
           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