Re: [PATCH 19/19] drm/i915: Mark the GuC log buffer flush interrupts handling WQ as freezable

[Imre Deak <imre.deak@intel.com>]

On pe, 2016-08-19 at 14:13 +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> The GuC log buffer flush work item has to do a register access to send the
> ack to GuC and this work item, if not synced before suspend, can potentially
> get executed after the GFX device is suspended. This work item function uses
> rpm get/put calls around the Hw access, which covers the rpm suspend case
> but for system suspend a sync would be required as kernel can potentially
> schedule the work items even after some devices, including GFX, have been
> put to suspend. But sync has to be done only for the system suspend case,
> as sync along with rpm get/put can cause a deadlock for rpm suspend path.
> To have the sync, but like a NOOP, for rpm suspend path also this work
> item could have been queued from the irq handler only when the device is
> runtime active & kept active while that work item is pending or getting
> executed but an interrupt can come even after the device is out of use and
> so can potentially lead to missing of this work item.
> 
> By marking the workqueue, dedicated for handling GuC log buffer flush
> interrupts, as freezable we don't have to bother about flushing of this
> work item from the suspend hooks, the pending work item if any will be
> either executed before the suspend or scheduled later on resume. This way
> the handling of log buffer flush work item can be kept same between system
> suspend & rpm suspend.
> 
> Suggested-by: Imre Deak <imre.deak@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Akash Goel <akash.goel@intel.com>

Thanks for taking the time, looks good to me:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index d4a1c84..bb25404 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1242,8 +1242,19 @@ static int guc_create_log_extras(struct intel_guc *guc)
>  
>  		/* Need a dedicated wq to process log buffer flush interrupts
>  		 * from GuC without much delay so as to avoid any loss of logs.
> +		 *
> +		 * GuC log buffer flush work item has to do register access to
> +		 * send the ack to GuC and this work item, if not synced before
> +		 * suspend, can potentially get executed after the GFX device is
> +		 * suspended.
> +		 * By marking the WQ as freezable, we don't have to bother about
> +		 * flushing of this work item from the suspend hooks, the pending
> +		 * work item if any will be either executed before the suspend
> +		 * or scheduled later on resume. This way the handling of work
> +		 * item can be kept same between system suspend & rpm suspend.
>  		 */
> -		guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log", 0);
> +		guc->log.flush_wq =
> +			alloc_ordered_workqueue("i915-guc_log", WQ_FREEZABLE);
>  		if (guc->log.flush_wq == NULL) {
>  			DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
>  			return -ENOMEM;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx