| Message ID | 1436472288-595-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 7/10/2015 1:34 AM, Daniel Vetter wrote: > Instead of trying to deal with this complexity we'll simply require > that the dmc firmware is available for runtime pm support. We do that > by not releasing the rpm reference we acquire when starting the > firmware loader work. Note that since we hold a rpm reference (and rpm > get/put is synchronized with its own locking already) there's no need > for any additional synchronization between the dmc loader and the rpm > entry/exit code. > > Hence we can remove all dmc_load_status_get calls, they don't do > anything any more. > > Cc: Animesh Manna <animesh.manna@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> For BXT, without DMC firmware display engine can goto DC9, so if firmware failed to load then as we are holding rpm reference - it will block dc9 entry. Do we really want to block dc9 if firmware is not present or failed to load? Regards, Animesh > --- > drivers/gpu/drm/i915/intel_csr.c | 9 +++++---- > drivers/gpu/drm/i915/intel_runtime_pm.c | 17 +++-------------- > 2 files changed, 8 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index 16cd9dae1c1b..03d83892cdb0 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -393,8 +393,11 @@ static void finish_csr_load(const struct firmware *fw, void *context) > out: > if (fw_loaded) > intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); > - else > - intel_csr_load_status_set(dev_priv, FW_FAILED); > + > + /* > + * We require the dmc firmware for runtime pm on gen9+ - leak the rpm > + * reference in case this failed to disable rpm on. > + */ > > release_firmware(fw); > } > @@ -462,8 +465,6 @@ void intel_csr_ucode_fini(struct drm_device *dev) > > void assert_csr_loaded(struct drm_i915_private *dev_priv) > { > - WARN(intel_csr_load_status_get(dev_priv) != FW_LOADED, > - "CSR is not loaded.\n"); > WARN(!I915_READ(CSR_PROGRAM_BASE), > "CSR program storage start is NULL\n"); > WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n"); > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 2628b21ff2c0..ed8c0cee738f 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -644,21 +644,10 @@ 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) { > - enum csr_state state; > - /* TODO: wait for a completion event or > - * similar here instead of busy > - * waiting using wait_for function. > - */ > - wait_for((state = intel_csr_load_status_get(dev_priv)) != > - FW_UNINITIALIZED, 1000); > - if (state != FW_LOADED) > - DRM_ERROR("CSR firmware not ready (%d)\n", > - state); > + if (SKL_ENABLE_DC6(dev)) > + skl_enable_dc6(dev_priv); > else > - if (SKL_ENABLE_DC6(dev)) > - skl_enable_dc6(dev_priv); > - else > - gen9_enable_dc5(dev_priv); > + gen9_enable_dc5(dev_priv); > } > } > }
On Fri, Jul 10, 2015 at 01:50:23PM +0530, Animesh Manna wrote: > > > On 7/10/2015 1:34 AM, Daniel Vetter wrote: > >Instead of trying to deal with this complexity we'll simply require > >that the dmc firmware is available for runtime pm support. We do that > >by not releasing the rpm reference we acquire when starting the > >firmware loader work. Note that since we hold a rpm reference (and rpm > >get/put is synchronized with its own locking already) there's no need > >for any additional synchronization between the dmc loader and the rpm > >entry/exit code. > > > >Hence we can remove all dmc_load_status_get calls, they don't do > >anything any more. > > > >Cc: Animesh Manna <animesh.manna@intel.com> > >Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > For BXT, without DMC firmware display engine can goto DC9, so if > firmware failed to load then as we are holding rpm reference - it will > block dc9 entry. Do we really want to block dc9 if firmware is not > present or failed to load? I guess for bxt we can lift the restriction and check dev_priv->csr.dmc_payload in the bxt power well code and drop the power well reference here by adjusting the condition. This really was just to showcase what I had in mind with grabbing the right power well reference: We use get/put to make sure that any of the platform power well code is guaranteed to never run, instead of perhaps adding another mutex or something similar (which usually results in more design problems than just a get/put pair). In short: This is just to demonstrate the flow, details can of course be adjusted. -Daniel > > Regards, > Animesh > > >--- > > drivers/gpu/drm/i915/intel_csr.c | 9 +++++---- > > drivers/gpu/drm/i915/intel_runtime_pm.c | 17 +++-------------- > > 2 files changed, 8 insertions(+), 18 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > >index 16cd9dae1c1b..03d83892cdb0 100644 > >--- a/drivers/gpu/drm/i915/intel_csr.c > >+++ b/drivers/gpu/drm/i915/intel_csr.c > >@@ -393,8 +393,11 @@ static void finish_csr_load(const struct firmware *fw, void *context) > > out: > > if (fw_loaded) > > intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); > >- else > >- intel_csr_load_status_set(dev_priv, FW_FAILED); > >+ > >+ /* > >+ * We require the dmc firmware for runtime pm on gen9+ - leak the rpm > >+ * reference in case this failed to disable rpm on. > >+ */ > > release_firmware(fw); > > } > >@@ -462,8 +465,6 @@ void intel_csr_ucode_fini(struct drm_device *dev) > > void assert_csr_loaded(struct drm_i915_private *dev_priv) > > { > >- WARN(intel_csr_load_status_get(dev_priv) != FW_LOADED, > >- "CSR is not loaded.\n"); > > WARN(!I915_READ(CSR_PROGRAM_BASE), > > "CSR program storage start is NULL\n"); > > WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n"); > >diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > >index 2628b21ff2c0..ed8c0cee738f 100644 > >--- a/drivers/gpu/drm/i915/intel_runtime_pm.c > >+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > >@@ -644,21 +644,10 @@ 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) { > >- enum csr_state state; > >- /* TODO: wait for a completion event or > >- * similar here instead of busy > >- * waiting using wait_for function. > >- */ > >- wait_for((state = intel_csr_load_status_get(dev_priv)) != > >- FW_UNINITIALIZED, 1000); > >- if (state != FW_LOADED) > >- DRM_ERROR("CSR firmware not ready (%d)\n", > >- state); > >+ if (SKL_ENABLE_DC6(dev)) > >+ skl_enable_dc6(dev_priv); > > else > >- if (SKL_ENABLE_DC6(dev)) > >- skl_enable_dc6(dev_priv); > >- else > >- gen9_enable_dc5(dev_priv); > >+ gen9_enable_dc5(dev_priv); > > } > > } > > } >
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index 16cd9dae1c1b..03d83892cdb0 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -393,8 +393,11 @@ static void finish_csr_load(const struct firmware *fw, void *context) out: if (fw_loaded) intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); - else - intel_csr_load_status_set(dev_priv, FW_FAILED); + + /* + * We require the dmc firmware for runtime pm on gen9+ - leak the rpm + * reference in case this failed to disable rpm on. + */ release_firmware(fw); } @@ -462,8 +465,6 @@ void intel_csr_ucode_fini(struct drm_device *dev) void assert_csr_loaded(struct drm_i915_private *dev_priv) { - WARN(intel_csr_load_status_get(dev_priv) != FW_LOADED, - "CSR is not loaded.\n"); WARN(!I915_READ(CSR_PROGRAM_BASE), "CSR program storage start is NULL\n"); WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n"); diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 2628b21ff2c0..ed8c0cee738f 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -644,21 +644,10 @@ 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) { - enum csr_state state; - /* TODO: wait for a completion event or - * similar here instead of busy - * waiting using wait_for function. - */ - wait_for((state = intel_csr_load_status_get(dev_priv)) != - FW_UNINITIALIZED, 1000); - if (state != FW_LOADED) - DRM_ERROR("CSR firmware not ready (%d)\n", - state); + if (SKL_ENABLE_DC6(dev)) + skl_enable_dc6(dev_priv); else - if (SKL_ENABLE_DC6(dev)) - skl_enable_dc6(dev_priv); - else - gen9_enable_dc5(dev_priv); + gen9_enable_dc5(dev_priv); } } }
Instead of trying to deal with this complexity we'll simply require that the dmc firmware is available for runtime pm support. We do that by not releasing the rpm reference we acquire when starting the firmware loader work. Note that since we hold a rpm reference (and rpm get/put is synchronized with its own locking already) there's no need for any additional synchronization between the dmc loader and the rpm entry/exit code. Hence we can remove all dmc_load_status_get calls, they don't do anything any more. Cc: Animesh Manna <animesh.manna@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/i915/intel_csr.c | 9 +++++---- drivers/gpu/drm/i915/intel_runtime_pm.c | 17 +++-------------- 2 files changed, 8 insertions(+), 18 deletions(-)