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