| Message ID | 20171115183029.2990-2-michel.thierry@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 15/11/17 10:30, Michel Thierry wrote: > During driver load we create the GuC clients and allocate their > doorbells just before executing guc_init_doorbell_hw; but since we just > created these doorbells, how can they be out of sync? > This code has had more than enough refactoring (2 more still in progress) > so I would not be surprised if calling guc_init_doorbell_hw made sense at > some point, but not anymore. > I think the idea was to clean up the unallocated doorbells on takeover to be covered in case the previous occupant of the GPU didn't release them when leaving the HW. We do a full gpu reset during i915 load now in i915_gem_sanitize so the doorbell HW should be cleaned up by that, but there is still a possible issue when i915.reset=0. However, with reset=0 this wouldn't be the only thing not sanitized and the only bad consequence would be extra irqs to GuC (which would be ignored), so I don't think it is worth worrying about that case. Acked-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Daniele > The resume path is different, in this case the driver doesn't > recreate clients, and it is still reasonable to validate/reallocate the > doorbells in order to confirm that they still belong to the clients. > > And probably guc_init_doorbell_hw is no longer the right name, but I'll > leave that to someone else. > > Signed-off-by: Michel Thierry <michel.thierry@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 5d6576e01a91..d6762ca42cf1 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -1424,16 +1424,16 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) > } else { > guc_reset_wq(guc->execbuf_client); > guc_reset_wq(guc->preempt_client); > + > + err = guc_init_doorbell_hw(guc); > + if (err) > + goto err_free_clients; > } > > err = intel_guc_sample_forcewake(guc); > if (err) > goto err_free_clients; > > - err = guc_init_doorbell_hw(guc); > - if (err) > - goto err_free_clients; > - > /* Take over from manual control of ELSP (execlists) */ > guc_interrupts_capture(dev_priv); > >
On 11/16/2017 12:00 AM, Michel Thierry wrote: > During driver load we create the GuC clients and allocate their > doorbells just before executing guc_init_doorbell_hw; but since we just > created these doorbells, how can they be out of sync? > This code has had more than enough refactoring (2 more still in progress) > so I would not be surprised if calling guc_init_doorbell_hw made sense at > some point, but not anymore. > > The resume path is different, in this case the driver doesn't > recreate clients, and it is still reasonable to validate/reallocate the > doorbells in order to confirm that they still belong to the clients. Planning to change this in upcoming series (allocate doorbells on resume when not needing uc_init_hw) and then we can do away with this validation. Another problem I see is, this is time consuming and leads to increase in the resume time (we also sanitize on resume hence this is unnecessary for all unused doorbells) > And probably guc_init_doorbell_hw is no longer the right name, but I'll > leave that to someone else. > > Signed-off-by: Michel Thierry <michel.thierry@intel.com> Change looks good to me. Acked-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 5d6576e01a91..d6762ca42cf1 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -1424,16 +1424,16 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) > } else { > guc_reset_wq(guc->execbuf_client); > guc_reset_wq(guc->preempt_client); > + > + err = guc_init_doorbell_hw(guc); > + if (err) > + goto err_free_clients; > } > > err = intel_guc_sample_forcewake(guc); > if (err) > goto err_free_clients; > > - err = guc_init_doorbell_hw(guc); > - if (err) > - goto err_free_clients; > - > /* Take over from manual control of ELSP (execlists) */ > guc_interrupts_capture(dev_priv); >
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 5d6576e01a91..d6762ca42cf1 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -1424,16 +1424,16 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) } else { guc_reset_wq(guc->execbuf_client); guc_reset_wq(guc->preempt_client); + + err = guc_init_doorbell_hw(guc); + if (err) + goto err_free_clients; } err = intel_guc_sample_forcewake(guc); if (err) goto err_free_clients; - err = guc_init_doorbell_hw(guc); - if (err) - goto err_free_clients; - /* Take over from manual control of ELSP (execlists) */ guc_interrupts_capture(dev_priv);
During driver load we create the GuC clients and allocate their doorbells just before executing guc_init_doorbell_hw; but since we just created these doorbells, how can they be out of sync? This code has had more than enough refactoring (2 more still in progress) so I would not be surprised if calling guc_init_doorbell_hw made sense at some point, but not anymore. The resume path is different, in this case the driver doesn't recreate clients, and it is still reasonable to validate/reallocate the doorbells in order to confirm that they still belong to the clients. And probably guc_init_doorbell_hw is no longer the right name, but I'll leave that to someone else. Signed-off-by: Michel Thierry <michel.thierry@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Oscar Mateo <oscar.mateo@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_guc_submission.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)