[Intel-gfx] [PATCH 04/10] drm/i915/guc: Add a second client, to be used for preemption

[Michał Winiarski]

On Thu, Oct 05, 2017 at 04:35:54PM +0000, Daniele Ceraolo Spurio wrote:
> 
> 
> On 05/10/17 09:00, Michał Winiarski wrote:
> > On Thu, Oct 05, 2017 at 03:22:59PM +0000, Daniele Ceraolo Spurio wrote:
> > > 
> > > 
> > > On 05/10/17 02:13, Michał Winiarski wrote:
> > > > From: Dave Gordon <david.s.gordon at intel.com>
> > > > 
> > > > This second client is created with priority KMD_HIGH, and marked
> > > > as preemptive. This will allow us to request preemption using GuC actions.
> > > > 
> > > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > > Cc: Jeff Mcgee <jeff.mcgee at intel.com>
> > > > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > > > Cc: Oscar Mateo <oscar.mateo at intel.com>
> > > > Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> > > > Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
> > > > ---
> > > >    drivers/gpu/drm/i915/i915_debugfs.c        |  4 ++++
> > > >    drivers/gpu/drm/i915/i915_guc_submission.c | 32 ++++++++++++++++++++++++++----
> > > >    drivers/gpu/drm/i915/intel_uc.h            |  1 +
> > > >    3 files changed, 33 insertions(+), 4 deletions(-)
> > 
> > [SNIP]
> > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> > > > index 10e8f0ed02e4..c6c6f8513bbf 100644
> > > > --- a/drivers/gpu/drm/i915/intel_uc.h
> > > > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > > > @@ -107,6 +107,7 @@ struct intel_guc {
> > > >    	struct ida stage_ids;
> > > >    	struct i915_guc_client *execbuf_client;
> > > > +	struct i915_guc_client *preempt_client;
> > > >    	DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
> > > >    	uint32_t db_cacheline;		/* Cyclic counter mod pagesize	*/
> > > > 
> > > 
> > > 
> > > I think you also need to update guc_init_doorbell_hw() to handle the new
> > > client. The comment in there says:
> > > 
> > > /* Now for every client (and not only execbuf_client) make sure their
> > >   * doorbells are known by the GuC */
> > 
> > But I don't want the doorbell for preempt_client...
> > I'm not submitting anything 'directly' using this client (just indirectly -
> > through preempt action).
> > 
> > Are you sure we need the doorbell?
> > Last time I tried to refactor GuC submission to use doorbell per engine, I got
> > the info that I should *avoid* allocating multiple doorbells.
> > 
> > -Michał
> > 
> 
> Don't you still get a doorbell from guc_client_alloc()? or am I missing
> something?

Right, but if we could get away without allocating one...

> One of the issues that guc_init_doorbell_hw() tries to solve is that after a
> GuC reset the doorbell HW is not cleaned up, so you can potentially have an
> active doorbell that GuC doesn't know about, which could lead to a failure
> if we try to release that doorbell later on. By doing the H2G we are sure
> that HW and GuC FW are in sync and I think this should apply to the doorbell
> in the preempt client as well.

Agreed.

> The problem with having many doorbells is that they add a slight delay when
> waking the power well (not sure about the exact details) and AFAIK this
> happens even if you don't ring them. For this reason it is true that we want
> to keep the number of doorbells limited, but having 2 in total shouldn't be
> an issue. However, if we really don't need the doorbell then we should
> probably modify guc_client_alloc() to skip the doorbell allocation for the
> preempt client, but that could lead to a bigger rework to remove the
> assumption that each client has a doorbell.

I guess I'll take a look at how much would need to be changed.
If it ends up being larger that I'm comfortable with for this series, I'll
fallback to modifying init_doorbell_hw and leave that for some other time.

Thanks!

-Michał

> 
> Daniele
> 
> > > 
> > > Daniele