Re: [Intel-gfx] [PATCH v2 3/3] drm/i915/gt: Only kick the signal worker if there's been an update

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Jul 08, 2022 at 10:40:24AM -0400, Rodrigo Vivi wrote:
> On Fri, Jul 08, 2022 at 04:20:13PM +0200, Karolina Drobnik wrote:
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > One impact of commit 047a1b877ed4 ("dma-buf & drm/amdgpu: remove
> > dma_resv workaround") is that it stores many, many more fences. Whereas
> > adding an exclusive fence used to remove the shared fence list, that
> > list is now preserved and the write fences included into the list. Not
> > just a single write fence, but now a write/read fence per context. That
> > causes us to have to track more fences than before (albeit half of those
> > are redundant), and we trigger more interrupts for multi-engine
> > workloads.
> > 
> > As part of reducing the impact from handling more signaling, we observe
> > we only need to kick the signal worker after adding a fence iff we have
> 
> s/iff/if
> 
> > good cause to believe that there is work to be done in processing the
> > fence i.e. we either need to enable the interrupt or the request is
> > already complete but we don't know if we saw the interrupt and so need
> > to check signaling.
> > 
> > References: 047a1b877ed4 ("dma-buf & drm/amdgpu: remove dma_resv workaround")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Karolina Drobnik <karolina.drobnik@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > index 9dc9dccf7b09..ecc990ec1b95 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > @@ -399,7 +399,8 @@ static void insert_breadcrumb(struct i915_request *rq)
> >  	 * the request as it may have completed and raised the interrupt as
> >  	 * we were attaching it into the lists.
> >  	 */
> > -	irq_work_queue(&b->irq_work);
> > +	if (!b->irq_armed || __i915_request_is_complete(rq))
> 
> would we need the READ_ONCE(irq_armed) ?
> would we need to use the irq_lock?

gentle ping on these questions here so maybe we can get this ready
for 5.20 still...

Thanks,
Rodrigo.

> 
> > +		irq_work_queue(&b->irq_work);
> >  }
> >  
> >  bool i915_request_enable_breadcrumb(struct i915_request *rq)
> > -- 
> > 2.25.1
> >