[RFC,2/3] drm/i915/guc: Omit guc_init_doorbell_hw during driver load

Submitted by Michel Thierry on Nov. 15, 2017, 6:30 p.m.

Details

Message ID 20171115183029.2990-2-michel.thierry@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Michel Thierry Nov. 15, 2017, 6:30 p.m.
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(-)

Patch hide | download patch | download mbox

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);
 

Comments

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);
>