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@xxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxxxxxxx>
> > > > Cc: Jeff Mcgee <jeff.mcgee@xxxxxxxxx>
> > > > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > > > Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> > > > Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
> > > > Signed-off-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > > > ---
> > > > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
