Re: [PATCH v4 16/30] drm/xe: Use ordered WQ for TLB invalidation fences

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Mar 25, 2024 at 04:30:51PM -0600, Zeng, Oak wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Matthew
> > Brost
> > Sent: Friday, March 8, 2024 12:08 AM
> > To: intel-xe@lists.freedesktop.org
> > Cc: Brost, Matthew <matthew.brost@intel.com>
> > Subject: [PATCH v4 16/30] drm/xe: Use ordered WQ for TLB invalidation fences
> > 
> > TLB invalidation fences need to be ordered within an exec queue and if
> > an unordered WQ is used TLB invalidation fences could be reordered. Use
> > an ordered WQ to fix this.
> 
> 
> The code change below makes sense to me. But I think we should improve the description. I think we need to explain what need to be in order and why. As I understand it, we need things to be in below order (just an example):
> 1)Gpu page table update (such as invalidate certain page table)
> 2)Invalidate tlb entries for above page table entries
> 3)Notify/call back to user fence to let user know above vm_unbind is complete.
> 
> Obviously 1) and 2) can't be re-ordered, because 2) should be triggered by 1). But 2) and 3) can be reordered if we use unordered WQ - why we want to use ordered wq here.
> 
> Simply saying TLB invalidation fences need to be ordered is too simple, at least it is hard for me to understand. Better to improve it.
> 

Let me improve this in the next rev.

Matt

> Thanks,
> Oak
> 
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_pt.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> > index 110d6917089b..a878e2217c7f 100644
> > --- a/drivers/gpu/drm/xe/xe_pt.c
> > +++ b/drivers/gpu/drm/xe/xe_pt.c
> > @@ -1107,7 +1107,7 @@ static void invalidation_fence_cb(struct dma_fence
> > *fence,
> > 
> >  	trace_xe_gt_tlb_invalidation_fence_cb(&ifence->base);
> >  	if (!ifence->fence->error) {
> > -		queue_work(system_wq, &ifence->work);
> > +		queue_work(ifence->gt->ordered_wq, &ifence->work);
> >  	} else {
> >  		ifence->base.base.error = ifence->fence->error;
> >  		dma_fence_signal(&ifence->base.base);
> > --
> > 2.34.1
>