>> 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, 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
