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@xxxxxxxxx>
> Cc: Damien Lespiau <damien.lespiau@xxxxxxxxx>
> Cc: Imre Deak <imre.deak@xxxxxxxxx>
> Cc: Sunil Kamath <sunil.kamath@xxxxxxxxx>
> Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
