| Message ID | 20181109145822.15446-3-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915/gen9_bc: Work around DMC bug zeroing power well requests | expand |
On Fri, Nov 09, 2018 at 04:58:22PM +0200, Imre Deak wrote: > Even though PW#1 and the MISC_IO power wells are managed by the > DMC firmware (toggled dynamically if conditions allow it) from the > driver's POV they are always on if the display core is initialized > (always restored by DMC to the enabled state after exiting from DC5/6 > for instance b/c of MMIO access). Accordingly we can just mark them as > always-on and remove the special casing for them during state > verification (thus enabling verification for these power wells too). > > Cc: Ramalingam C <ramalingam.c@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_runtime_pm.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 44a77de439f2..6b1576ae778f 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -2358,6 +2358,7 @@ static const struct i915_power_well_desc skl_power_wells[] = { > { > .name = "power well 1", > /* Handled by the DMC firmware */ > + .always_on = true, > .domains = 0, First I was wondering if this somehow changes the behaviour of __intel_display_power_is_enabled(), but it won't since all these wells have domains==0 and so would be skipped there anyway. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > .ops = &hsw_power_well_ops, > .id = SKL_DISP_PW_1, > @@ -2370,6 +2371,7 @@ static const struct i915_power_well_desc skl_power_wells[] = { > { > .name = "MISC IO power well", > /* Handled by the DMC firmware */ > + .always_on = true, > .domains = 0, > .ops = &hsw_power_well_ops, > .id = SKL_DISP_PW_MISC_IO, > @@ -2449,6 +2451,8 @@ static const struct i915_power_well_desc bxt_power_wells[] = { > }, > { > .name = "power well 1", > + /* Handled by the DMC firmware */ > + .always_on = true, > .domains = 0, > .ops = &hsw_power_well_ops, > .id = SKL_DISP_PW_1, > @@ -2508,6 +2512,7 @@ static const struct i915_power_well_desc glk_power_wells[] = { > { > .name = "power well 1", > /* Handled by the DMC firmware */ > + .always_on = true, > .domains = 0, > .ops = &hsw_power_well_ops, > .id = SKL_DISP_PW_1, > @@ -2636,6 +2641,7 @@ static const struct i915_power_well_desc cnl_power_wells[] = { > { > .name = "power well 1", > /* Handled by the DMC firmware */ > + .always_on = true, > .domains = 0, > .ops = &hsw_power_well_ops, > .id = SKL_DISP_PW_1, > @@ -2803,6 +2809,7 @@ static const struct i915_power_well_desc icl_power_wells[] = { > { > .name = "power well 1", > /* Handled by the DMC firmware */ > + .always_on = true, > .domains = 0, > .ops = &hsw_power_well_ops, > .id = SKL_DISP_PW_1, > @@ -3936,14 +3943,6 @@ static void intel_power_domains_verify_state(struct drm_i915_private *dev_priv) > int domains_count; > bool enabled; > > - /* > - * Power wells not belonging to any domain (like the MISC_IO > - * and PW1 power wells) are under FW control, so ignore them, > - * since their state can change asynchronously. > - */ > - if (!power_well->desc->domains) > - continue; > - > enabled = power_well->desc->ops->is_enabled(dev_priv, > power_well); > if ((power_well->count || power_well->desc->always_on) != > -- > 2.13.2
On Mon, Nov 12, 2018 at 07:19:44PM +0200, Ville Syrjälä wrote: > On Fri, Nov 09, 2018 at 04:58:22PM +0200, Imre Deak wrote: > > Even though PW#1 and the MISC_IO power wells are managed by the > > DMC firmware (toggled dynamically if conditions allow it) from the > > driver's POV they are always on if the display core is initialized > > (always restored by DMC to the enabled state after exiting from DC5/6 > > for instance b/c of MMIO access). Accordingly we can just mark them as > > always-on and remove the special casing for them during state > > verification (thus enabling verification for these power wells too). > > > > Cc: Ramalingam C <ramalingam.c@intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_runtime_pm.c | 15 +++++++-------- > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > > index 44a77de439f2..6b1576ae778f 100644 > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > @@ -2358,6 +2358,7 @@ static const struct i915_power_well_desc skl_power_wells[] = { > > { > > .name = "power well 1", > > /* Handled by the DMC firmware */ > > + .always_on = true, > > .domains = 0, > > First I was wondering if this somehow changes the behaviour of > __intel_display_power_is_enabled(), but it won't since all these > wells have domains==0 and so would be skipped there anyway. Yes, it should only affect the verification, otherwise doesn't change the behaviour. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > .ops = &hsw_power_well_ops, > > .id = SKL_DISP_PW_1, > > @@ -2370,6 +2371,7 @@ static const struct i915_power_well_desc skl_power_wells[] = { > > { > > .name = "MISC IO power well", > > /* Handled by the DMC firmware */ > > + .always_on = true, > > .domains = 0, > > .ops = &hsw_power_well_ops, > > .id = SKL_DISP_PW_MISC_IO, > > @@ -2449,6 +2451,8 @@ static const struct i915_power_well_desc bxt_power_wells[] = { > > }, > > { > > .name = "power well 1", > > + /* Handled by the DMC firmware */ > > + .always_on = true, > > .domains = 0, > > .ops = &hsw_power_well_ops, > > .id = SKL_DISP_PW_1, > > @@ -2508,6 +2512,7 @@ static const struct i915_power_well_desc glk_power_wells[] = { > > { > > .name = "power well 1", > > /* Handled by the DMC firmware */ > > + .always_on = true, > > .domains = 0, > > .ops = &hsw_power_well_ops, > > .id = SKL_DISP_PW_1, > > @@ -2636,6 +2641,7 @@ static const struct i915_power_well_desc cnl_power_wells[] = { > > { > > .name = "power well 1", > > /* Handled by the DMC firmware */ > > + .always_on = true, > > .domains = 0, > > .ops = &hsw_power_well_ops, > > .id = SKL_DISP_PW_1, > > @@ -2803,6 +2809,7 @@ static const struct i915_power_well_desc icl_power_wells[] = { > > { > > .name = "power well 1", > > /* Handled by the DMC firmware */ > > + .always_on = true, > > .domains = 0, > > .ops = &hsw_power_well_ops, > > .id = SKL_DISP_PW_1, > > @@ -3936,14 +3943,6 @@ static void intel_power_domains_verify_state(struct drm_i915_private *dev_priv) > > int domains_count; > > bool enabled; > > > > - /* > > - * Power wells not belonging to any domain (like the MISC_IO > > - * and PW1 power wells) are under FW control, so ignore them, > > - * since their state can change asynchronously. > > - */ > > - if (!power_well->desc->domains) > > - continue; > > - > > enabled = power_well->desc->ops->is_enabled(dev_priv, > > power_well); > > if ((power_well->count || power_well->desc->always_on) != > > -- > > 2.13.2 > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 44a77de439f2..6b1576ae778f 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -2358,6 +2358,7 @@ static const struct i915_power_well_desc skl_power_wells[] = { { .name = "power well 1", /* Handled by the DMC firmware */ + .always_on = true, .domains = 0, .ops = &hsw_power_well_ops, .id = SKL_DISP_PW_1, @@ -2370,6 +2371,7 @@ static const struct i915_power_well_desc skl_power_wells[] = { { .name = "MISC IO power well", /* Handled by the DMC firmware */ + .always_on = true, .domains = 0, .ops = &hsw_power_well_ops, .id = SKL_DISP_PW_MISC_IO, @@ -2449,6 +2451,8 @@ static const struct i915_power_well_desc bxt_power_wells[] = { }, { .name = "power well 1", + /* Handled by the DMC firmware */ + .always_on = true, .domains = 0, .ops = &hsw_power_well_ops, .id = SKL_DISP_PW_1, @@ -2508,6 +2512,7 @@ static const struct i915_power_well_desc glk_power_wells[] = { { .name = "power well 1", /* Handled by the DMC firmware */ + .always_on = true, .domains = 0, .ops = &hsw_power_well_ops, .id = SKL_DISP_PW_1, @@ -2636,6 +2641,7 @@ static const struct i915_power_well_desc cnl_power_wells[] = { { .name = "power well 1", /* Handled by the DMC firmware */ + .always_on = true, .domains = 0, .ops = &hsw_power_well_ops, .id = SKL_DISP_PW_1, @@ -2803,6 +2809,7 @@ static const struct i915_power_well_desc icl_power_wells[] = { { .name = "power well 1", /* Handled by the DMC firmware */ + .always_on = true, .domains = 0, .ops = &hsw_power_well_ops, .id = SKL_DISP_PW_1, @@ -3936,14 +3943,6 @@ static void intel_power_domains_verify_state(struct drm_i915_private *dev_priv) int domains_count; bool enabled; - /* - * Power wells not belonging to any domain (like the MISC_IO - * and PW1 power wells) are under FW control, so ignore them, - * since their state can change asynchronously. - */ - if (!power_well->desc->domains) - continue; - enabled = power_well->desc->ops->is_enabled(dev_priv, power_well); if ((power_well->count || power_well->desc->always_on) !=
Even though PW#1 and the MISC_IO power wells are managed by the DMC firmware (toggled dynamically if conditions allow it) from the driver's POV they are always on if the display core is initialized (always restored by DMC to the enabled state after exiting from DC5/6 for instance b/c of MMIO access). Accordingly we can just mark them as always-on and remove the special casing for them during state verification (thus enabling verification for these power wells too). Cc: Ramalingam C <ramalingam.c@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_runtime_pm.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)