[Intel-gfx] [PATCH 14/18] drm/i915/gen9: Use flush_work to synchronize with dmc loader

[Daniel Vetter]

On Sun, Jul 26, 2015 at 12:30:35AM +0530, Animesh Manna wrote:
> As power well 1 is superset of power well 2 and always pw2
> will be disabled first and then pw1. On the other hand dmc
> is responsible to save & restore back pw1 when display
> engine goes and come back from low power state. Before
> disabling pw1 dmc must be loaded, so adding flush_work()
> while disabling pw2 which ensure that firmware will be
> available before disabling pw1 in suspend flow.
> 
> Cc: Daniel Vetter <daniel.vetter at intel.com>
> Cc: Damien Lespiau <damien.lespiau at intel.com>
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Sunil Kamath <sunil.kamath at intel.com>
> Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         | 2 --
>  drivers/gpu/drm/i915/intel_csr.c        | 2 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 1 +
>  3 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 98343eb..ddf8a25 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -995,8 +995,6 @@ static int i915_pm_resume(struct device *dev)
>  
>  static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>  {
> -	/* Enabling DC6 is not a hard requirement to enter runtime D3 */

Don't we need a flush_work here to make sure dmc is ready before going
into suspend?

> -
>  	skl_uninit_cdclk(dev_priv);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 36386324..1858f02 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -417,5 +417,7 @@ void intel_csr_ucode_fini(struct drm_i915_private *dev_priv)
>  	if (!HAS_CSR(dev_priv))
>  		return;
>  
> +	flush_work(&dev_priv->csr.work);
> +
>  	kfree(dev_priv->csr.dmc_payload);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 5f1ae23..a5059e8 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -652,6 +652,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  
>  			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>  				power_well->data == SKL_DISP_PW_2) {
> +				flush_work(&dev_priv->csr.work);

This flush_work here has two problems:
- It's an inversion of control since this is low-level code blocking on
  high-level. I think this is here is safe but in general it's very easy
  to create deadlocks and hence this pattern is discouraged.
- dmc loading might take a while (especially on cros/android where i915 is
  built-in but fw needs to wait for the full system before it can be
  loaded). We do it asynchronously so that i915 init can proceed and we
  can enable outputs really early, but the modeset code also does power
  well get/put calls and might end up in here, resulting in a stall.

If we instead prevent execution of this low-level power-well code by
grabbing a power_domain reference then both problems are solved:
- No flush_work needed since we'll only get into this code when the last
  power domain references is dropped, which necessarily means dmc loader
  has completed.
- No blocking of modesets while dmc loading is still in progress since the
  get/put power_domain calls will simply be no-ops too.

Long story short: If you apply the two patches I mentioned in my reply to
patch 4 there shouldn't be a need for this flush_work here in
skl_set_power_well.

Thanks, Daniel

>  				if (SKL_ENABLE_DC6(dev))
>  					skl_enable_dc6(dev_priv);
>  				else
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch