RE: [Intel-gfx] [PATCH 4/8] drm/i915/mtl: workaround coherency issue for Media

From: "Yang, Fei" <fei.yang@intel.com>
To: "Hajda, Andrzej" <andrzej.hajda@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>
Subject: RE: [Intel-gfx] [PATCH 4/8] drm/i915/mtl: workaround coherency issue for Media
Date: Wed, 19 Apr 2023 18:49:17 +0000	[thread overview]
Message-ID: <SN6PR11MB257482462F00C5B347E6CC339A629@SN6PR11MB2574.namprd11.prod.outlook.com> (raw)
In-Reply-To: <SN6PR11MB2574462BC46616D1954D40309A629@SN6PR11MB2574.namprd11.prod.outlook.com>

>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>>> index 1803a633ed64..98e682b7df07 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>>> @@ -415,12 +415,6 @@ static int ct_write(struct intel_guc_ct *ct,
>>>      }
>>>      GEM_BUG_ON(tail > size);
>>>
>>> -    /*
>>> -     * make sure H2G buffer update and LRC tail update (if this triggering a
>>> -     * submission) are visible before updating the descriptor tail
>>> -     */
>>> -    intel_guc_write_barrier(ct_to_guc(ct));
>>> -
>>>      /* update local copies */
>>>      ctb->tail = tail;
>>>      GEM_BUG_ON(atomic_read(&ctb->space) < len + GUC_CTB_HDR_LEN); @@
>>> -429,6 +423,12 @@ static int ct_write(struct intel_guc_ct *ct,
>>>      /* now update descriptor */
>>>      WRITE_ONCE(desc->tail, tail);
>>>
>>> +    /*
>>> +     * make sure H2G buffer update and LRC tail update (if this triggering a
>>> +     * submission) are visible before updating the descriptor tail
>>> +     */
>>> +    intel_guc_write_barrier(ct_to_guc(ct));
>>
>> The comment above needs update,

Never mind, I decided to revert this change because it's not necessary. There is a
MMIO write following the ct_write() call which would flush the write combining
buffer anyway, so the barrier is redundant here.

>
>Will update the comment.
>
>> if this is correct change. The question is why it is correct? If yes,
>> it implies that old barrier is incorrect, maybe it should be then separate fix?
>
> There is WRITE_ONCE(desc->tail, tail) right after the H2G buffer update which is also
> seen by the GuC firmware, the barrier is needed for both, thus moved it down a few
> lines to cover them all.
>
>> I am not an expert, but previous location of the barrier seems sane to
>> me - assure GuC will see proper buffer, before updating buffer's tail.
>
> That is correct, but the barrier is needed for both H2G buffer and descriptor, as they are all shared with GuC firmware.
>
> -Fei
>
>> And according to commit message this new barrier should flush WC
>> buffer, so for me it seems to be different thing.
>> Am I missing something?
>>
>>
>> Regards
>> Andrzej