| Message ID | 20171005091349.9315-4-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Michał Winiarski (2017-10-05 10:13:43) > From: Dave Gordon <david.s.gordon@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@chris-wilson.co.uk> > Cc: Jeff Mcgee <jeff.mcgee@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > Signed-off-by: Michał Winiarski <michal.winiarski@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(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index b4a6ac60e7c6..1a963c13cab8 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2488,6 +2488,10 @@ static int i915_guc_info(struct seq_file *m, void *data) > > seq_printf(m, "\nGuC execbuf client @ %p:\n", guc->execbuf_client); > i915_guc_client_info(m, dev_priv, guc->execbuf_client); > + if (guc->preempt_client) { Hmm, when do we have guc->execbuf_client but not guc->preempt_client? Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Thu, 05 Oct 2017 11:13:43 +0200, Michał Winiarski <michal.winiarski@intel.com> wrote: > From: Dave Gordon <david.s.gordon@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@chris-wilson.co.uk> > Cc: Jeff Mcgee <jeff.mcgee@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > Signed-off-by: Michał Winiarski <michal.winiarski@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(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index b4a6ac60e7c6..1a963c13cab8 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2488,6 +2488,10 @@ static int i915_guc_info(struct seq_file *m, void > *data) > seq_printf(m, "\nGuC execbuf client @ %p:\n", guc->execbuf_client); > i915_guc_client_info(m, dev_priv, guc->execbuf_client); > + if (guc->preempt_client) { > + seq_printf(m, "\nGuC preempt client\n"); Maybe to match output from 'execbuf' there should be "@ %p:" > + i915_guc_client_info(m, dev_priv, guc->preempt_client); > + } > i915_guc_log_info(m, dev_priv); > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c > b/drivers/gpu/drm/i915/i915_guc_submission.c > index 374501a67274..e8052e86426d 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -331,6 +331,8 @@ static void guc_stage_desc_init(struct intel_guc > *guc, > memset(desc, 0, sizeof(*desc)); > desc->attribute = GUC_STAGE_DESC_ATTR_ACTIVE | > GUC_STAGE_DESC_ATTR_KERNEL; > + if (client->priority <= GUC_CLIENT_PRIORITY_HIGH) > + desc->attribute |= GUC_STAGE_DESC_ATTR_PREEMPT; > desc->stage_id = client->stage_id; > desc->priority = client->priority; > desc->db_id = client->doorbell_id; > @@ -1154,7 +1156,7 @@ int i915_guc_submission_enable(struct > drm_i915_private *dev_priv) > sizeof(struct guc_wq_item) * > I915_NUM_ENGINES > GUC_WQ_SIZE); > - if (!client) { Make sure to remove unnecessary earlier assignment: client = guc->execbuf_client; > + if (!guc->execbuf_client) { > client = guc_client_alloc(dev_priv, > INTEL_INFO(dev_priv)->ring_mask, > GUC_CLIENT_PRIORITY_KMD_NORMAL, > @@ -1167,15 +1169,29 @@ int i915_guc_submission_enable(struct > drm_i915_private *dev_priv) > guc->execbuf_client = client; > } > + if (!guc->preempt_client) { > + client = guc_client_alloc(dev_priv, > + INTEL_INFO(dev_priv)->ring_mask, > + GUC_CLIENT_PRIORITY_KMD_HIGH, > + dev_priv->preempt_context); > + if (IS_ERR(client)) { > + DRM_ERROR("Failed to create GuC client for preemption!\n"); > + err = PTR_ERR(client); > + goto err_free_clients; > + } > + > + guc->preempt_client = client; > + } > + If we allow clients to be already created here, then maybe we should move their creation into separate helper inline to clearly mark that. Then we will have simpler error handling sequence ;) > err = intel_guc_sample_forcewake(guc); > if (err) > - goto err_execbuf_client; > + goto err_free_clients; > guc_reset_wq(client); > err = guc_init_doorbell_hw(guc); > if (err) > - goto err_execbuf_client; > + goto err_free_clients; > /* Take over from manual control of ELSP (execlists) */ > guc_interrupts_capture(dev_priv); > @@ -1187,7 +1203,11 @@ int i915_guc_submission_enable(struct > drm_i915_private *dev_priv) > return 0; > -err_execbuf_client: > +err_free_clients: > + if (guc->preempt_client) { > + guc_client_free(guc->preempt_client); > + guc->preempt_client = NULL; > + } > guc_client_free(guc->execbuf_client); > guc->execbuf_client = NULL; > return err; > @@ -1202,6 +1222,10 @@ void i915_guc_submission_disable(struct > drm_i915_private *dev_priv) > /* Revert back to manual ELSP submission */ > intel_engines_reset_default_submission(dev_priv); > + if (guc->preempt_client) { > + guc_client_free(guc->preempt_client); > + guc->preempt_client = NULL; > + } > guc_client_free(guc->execbuf_client); > guc->execbuf_client = NULL; > } > 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 */
On Thu, Oct 05, 2017 at 11:13:43AM +0200, Michał Winiarski wrote: > From: Dave Gordon <david.s.gordon@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@chris-wilson.co.uk> > Cc: Jeff Mcgee <jeff.mcgee@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > Signed-off-by: Michał Winiarski <michal.winiarski@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(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index b4a6ac60e7c6..1a963c13cab8 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2488,6 +2488,10 @@ static int i915_guc_info(struct seq_file *m, void *data) > > seq_printf(m, "\nGuC execbuf client @ %p:\n", guc->execbuf_client); > i915_guc_client_info(m, dev_priv, guc->execbuf_client); > + if (guc->preempt_client) { > + seq_printf(m, "\nGuC preempt client\n"); > + i915_guc_client_info(m, dev_priv, guc->preempt_client); > + } > > i915_guc_log_info(m, dev_priv); > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 374501a67274..e8052e86426d 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -331,6 +331,8 @@ static void guc_stage_desc_init(struct intel_guc *guc, > memset(desc, 0, sizeof(*desc)); > > desc->attribute = GUC_STAGE_DESC_ATTR_ACTIVE | GUC_STAGE_DESC_ATTR_KERNEL; > + if (client->priority <= GUC_CLIENT_PRIORITY_HIGH) > + desc->attribute |= GUC_STAGE_DESC_ATTR_PREEMPT; > desc->stage_id = client->stage_id; > desc->priority = client->priority; > desc->db_id = client->doorbell_id; > @@ -1154,7 +1156,7 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) > sizeof(struct guc_wq_item) * > I915_NUM_ENGINES > GUC_WQ_SIZE); > I think that client is still being initialized above to guc->execbuf_client. If so, can it be removed? > - if (!client) { > + if (!guc->execbuf_client) { > client = guc_client_alloc(dev_priv, > INTEL_INFO(dev_priv)->ring_mask, > GUC_CLIENT_PRIORITY_KMD_NORMAL, > @@ -1167,15 +1169,29 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) > guc->execbuf_client = client; > } > > + if (!guc->preempt_client) { > + client = guc_client_alloc(dev_priv, > + INTEL_INFO(dev_priv)->ring_mask, > + GUC_CLIENT_PRIORITY_KMD_HIGH, > + dev_priv->preempt_context); > + if (IS_ERR(client)) { > + DRM_ERROR("Failed to create GuC client for preemption!\n"); > + err = PTR_ERR(client); > + goto err_free_clients; > + } > + > + guc->preempt_client = client; > + } > + > err = intel_guc_sample_forcewake(guc); > if (err) > - goto err_execbuf_client; > + goto err_free_clients; > > guc_reset_wq(client); > > err = guc_init_doorbell_hw(guc); > if (err) > - goto err_execbuf_client; > + goto err_free_clients; > > /* Take over from manual control of ELSP (execlists) */ > guc_interrupts_capture(dev_priv); > @@ -1187,7 +1203,11 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) > > return 0; > > -err_execbuf_client: > +err_free_clients: > + if (guc->preempt_client) { > + guc_client_free(guc->preempt_client); > + guc->preempt_client = NULL; > + } > guc_client_free(guc->execbuf_client); > guc->execbuf_client = NULL; > return err; > @@ -1202,6 +1222,10 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv) > /* Revert back to manual ELSP submission */ > intel_engines_reset_default_submission(dev_priv); > > + if (guc->preempt_client) { Do we need this check? It seems that execbuf_client and preempt_client are both required for GuC submission to be enabled. > + guc_client_free(guc->preempt_client); > + guc->preempt_client = NULL; > + } > guc_client_free(guc->execbuf_client); > guc->execbuf_client = NULL; > } > 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 */ > -- > 2.13.5 >
On 05/10/17 02:13, Michał Winiarski wrote: > From: Dave Gordon <david.s.gordon@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@chris-wilson.co.uk> > Cc: Jeff Mcgee <jeff.mcgee@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > Signed-off-by: Michał Winiarski <michal.winiarski@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(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index b4a6ac60e7c6..1a963c13cab8 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2488,6 +2488,10 @@ static int i915_guc_info(struct seq_file *m, void *data) > > seq_printf(m, "\nGuC execbuf client @ %p:\n", guc->execbuf_client); > i915_guc_client_info(m, dev_priv, guc->execbuf_client); > + if (guc->preempt_client) { > + seq_printf(m, "\nGuC preempt client\n"); > + i915_guc_client_info(m, dev_priv, guc->preempt_client); > + } > > i915_guc_log_info(m, dev_priv); > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 374501a67274..e8052e86426d 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -331,6 +331,8 @@ static void guc_stage_desc_init(struct intel_guc *guc, > memset(desc, 0, sizeof(*desc)); > > desc->attribute = GUC_STAGE_DESC_ATTR_ACTIVE | GUC_STAGE_DESC_ATTR_KERNEL; > + if (client->priority <= GUC_CLIENT_PRIORITY_HIGH) > + desc->attribute |= GUC_STAGE_DESC_ATTR_PREEMPT; > desc->stage_id = client->stage_id; > desc->priority = client->priority; > desc->db_id = client->doorbell_id; > @@ -1154,7 +1156,7 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) > sizeof(struct guc_wq_item) * > I915_NUM_ENGINES > GUC_WQ_SIZE); > > - if (!client) { > + if (!guc->execbuf_client) { > client = guc_client_alloc(dev_priv, > INTEL_INFO(dev_priv)->ring_mask, > GUC_CLIENT_PRIORITY_KMD_NORMAL, > @@ -1167,15 +1169,29 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) > guc->execbuf_client = client; > } > > + if (!guc->preempt_client) { > + client = guc_client_alloc(dev_priv, > + INTEL_INFO(dev_priv)->ring_mask, > + GUC_CLIENT_PRIORITY_KMD_HIGH, > + dev_priv->preempt_context); > + if (IS_ERR(client)) { > + DRM_ERROR("Failed to create GuC client for preemption!\n"); > + err = PTR_ERR(client); > + goto err_free_clients; > + } > + > + guc->preempt_client = client; > + } > + > err = intel_guc_sample_forcewake(guc); > if (err) > - goto err_execbuf_client; > + goto err_free_clients; > > guc_reset_wq(client); > > err = guc_init_doorbell_hw(guc); > if (err) > - goto err_execbuf_client; > + goto err_free_clients; > > /* Take over from manual control of ELSP (execlists) */ > guc_interrupts_capture(dev_priv); > @@ -1187,7 +1203,11 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) > > return 0; > > -err_execbuf_client: > +err_free_clients: > + if (guc->preempt_client) { > + guc_client_free(guc->preempt_client); > + guc->preempt_client = NULL; > + } > guc_client_free(guc->execbuf_client); > guc->execbuf_client = NULL; > return err; > @@ -1202,6 +1222,10 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv) > /* Revert back to manual ELSP submission */ > intel_engines_reset_default_submission(dev_priv); > > + if (guc->preempt_client) { > + guc_client_free(guc->preempt_client); > + guc->preempt_client = NULL; > + } > guc_client_free(guc->execbuf_client); > guc->execbuf_client = NULL; > } > 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 */ Daniele
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@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@chris-wilson.co.uk> > > Cc: Jeff Mcgee <jeff.mcgee@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Oscar Mateo <oscar.mateo@intel.com> > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > > Signed-off-by: Michał Winiarski <michal.winiarski@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ł > > Daniele
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@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@chris-wilson.co.uk> >>> Cc: Jeff Mcgee <jeff.mcgee@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Oscar Mateo <oscar.mateo@intel.com> >>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >>> Signed-off-by: Michał Winiarski <michal.winiarski@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? 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. 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. Daniele >> >> Daniele
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@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@chris-wilson.co.uk> > > > > Cc: Jeff Mcgee <jeff.mcgee@intel.com> > > > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > > > Cc: Oscar Mateo <oscar.mateo@intel.com> > > > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > > > > Signed-off-by: Michał Winiarski <michal.winiarski@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
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index b4a6ac60e7c6..1a963c13cab8 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2488,6 +2488,10 @@ static int i915_guc_info(struct seq_file *m, void *data) seq_printf(m, "\nGuC execbuf client @ %p:\n", guc->execbuf_client); i915_guc_client_info(m, dev_priv, guc->execbuf_client); + if (guc->preempt_client) { + seq_printf(m, "\nGuC preempt client\n"); + i915_guc_client_info(m, dev_priv, guc->preempt_client); + } i915_guc_log_info(m, dev_priv); diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 374501a67274..e8052e86426d 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -331,6 +331,8 @@ static void guc_stage_desc_init(struct intel_guc *guc, memset(desc, 0, sizeof(*desc)); desc->attribute = GUC_STAGE_DESC_ATTR_ACTIVE | GUC_STAGE_DESC_ATTR_KERNEL; + if (client->priority <= GUC_CLIENT_PRIORITY_HIGH) + desc->attribute |= GUC_STAGE_DESC_ATTR_PREEMPT; desc->stage_id = client->stage_id; desc->priority = client->priority; desc->db_id = client->doorbell_id; @@ -1154,7 +1156,7 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) sizeof(struct guc_wq_item) * I915_NUM_ENGINES > GUC_WQ_SIZE); - if (!client) { + if (!guc->execbuf_client) { client = guc_client_alloc(dev_priv, INTEL_INFO(dev_priv)->ring_mask, GUC_CLIENT_PRIORITY_KMD_NORMAL, @@ -1167,15 +1169,29 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) guc->execbuf_client = client; } + if (!guc->preempt_client) { + client = guc_client_alloc(dev_priv, + INTEL_INFO(dev_priv)->ring_mask, + GUC_CLIENT_PRIORITY_KMD_HIGH, + dev_priv->preempt_context); + if (IS_ERR(client)) { + DRM_ERROR("Failed to create GuC client for preemption!\n"); + err = PTR_ERR(client); + goto err_free_clients; + } + + guc->preempt_client = client; + } + err = intel_guc_sample_forcewake(guc); if (err) - goto err_execbuf_client; + goto err_free_clients; guc_reset_wq(client); err = guc_init_doorbell_hw(guc); if (err) - goto err_execbuf_client; + goto err_free_clients; /* Take over from manual control of ELSP (execlists) */ guc_interrupts_capture(dev_priv); @@ -1187,7 +1203,11 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) return 0; -err_execbuf_client: +err_free_clients: + if (guc->preempt_client) { + guc_client_free(guc->preempt_client); + guc->preempt_client = NULL; + } guc_client_free(guc->execbuf_client); guc->execbuf_client = NULL; return err; @@ -1202,6 +1222,10 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv) /* Revert back to manual ELSP submission */ intel_engines_reset_default_submission(dev_priv); + if (guc->preempt_client) { + guc_client_free(guc->preempt_client); + guc->preempt_client = NULL; + } guc_client_free(guc->execbuf_client); guc->execbuf_client = NULL; } 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 */