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

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx