| Message ID | 20241021222744.294371-5-gustavo.sousa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dmc_wl: Fixes and enablement for Xe3_LPD | expand |
On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote: > Bspec says that disabling dynamic DC states require taking the DMC > wakelock to cause an DC exit before writing to DC_STATE_EN. Implement > that. > > In fact, testing on PTL revealed we end up failing to exit DC5/6 without > this step. > > Bspec: 71583 > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > --- > .../drm/i915/display/intel_display_power_well.c | 10 +++++++--- > drivers/gpu/drm/i915/display/intel_dmc_wl.c | 14 ++++++++++++-- > drivers/gpu/drm/i915/display/intel_dmc_wl.h | 2 ++ > 3 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > index adaf7cf3a33b..e8946ce86aaa 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > @@ -987,10 +987,14 @@ void gen9_disable_dc_states(struct intel_display *display) > return; > } > > - gen9_set_dc_state(display, DC_STATE_DISABLE); > - > - if (!HAS_DISPLAY(display)) > + if (HAS_DISPLAY(display)) { > + intel_dmc_wl_get_noreg(display); > + gen9_set_dc_state(display, DC_STATE_DISABLE); > + intel_dmc_wl_put_noreg(display); > + } else { > + gen9_set_dc_state(display, DC_STATE_DISABLE); > return; > + } I think intel_dmc_get/put() already protect indirectly on HAS_DISPLAY(), doesn't it? If that's the case, then the if here is unnecessary. -- Cheers, Luca.
Quoting Luca Coelho (2024-11-01 09:24:08-03:00) >On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote: >> Bspec says that disabling dynamic DC states require taking the DMC >> wakelock to cause an DC exit before writing to DC_STATE_EN. Implement >> that. >> >> In fact, testing on PTL revealed we end up failing to exit DC5/6 without >> this step. >> >> Bspec: 71583 >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> >> --- >> .../drm/i915/display/intel_display_power_well.c | 10 +++++++--- >> drivers/gpu/drm/i915/display/intel_dmc_wl.c | 14 ++++++++++++-- >> drivers/gpu/drm/i915/display/intel_dmc_wl.h | 2 ++ >> 3 files changed, 21 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c >> index adaf7cf3a33b..e8946ce86aaa 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c >> @@ -987,10 +987,14 @@ void gen9_disable_dc_states(struct intel_display *display) >> return; >> } >> >> - gen9_set_dc_state(display, DC_STATE_DISABLE); >> - >> - if (!HAS_DISPLAY(display)) >> + if (HAS_DISPLAY(display)) { >> + intel_dmc_wl_get_noreg(display); >> + gen9_set_dc_state(display, DC_STATE_DISABLE); >> + intel_dmc_wl_put_noreg(display); >> + } else { >> + gen9_set_dc_state(display, DC_STATE_DISABLE); >> return; >> + } > >I think intel_dmc_get/put() already protect indirectly on >HAS_DISPLAY(), doesn't it? If that's the case, then the if here is >unnecessary. Actually, intel_dmc_wl_init() gets called only when HAS_DISPLAY() is true, so I think using intel_dmc_wl_{get,put}_noreg() for the !HAS_DISPLAY() case would not be right, at least not with the current state of the code. -- Gustavo Sousa
On Tue, 2024-11-05 at 09:44 -0300, Gustavo Sousa wrote: > Quoting Luca Coelho (2024-11-01 09:24:08-03:00) > > On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote: > > > Bspec says that disabling dynamic DC states require taking the DMC > > > wakelock to cause an DC exit before writing to DC_STATE_EN. Implement > > > that. > > > > > > In fact, testing on PTL revealed we end up failing to exit DC5/6 without > > > this step. > > > > > > Bspec: 71583 > > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > > > --- > > > .../drm/i915/display/intel_display_power_well.c | 10 +++++++--- > > > drivers/gpu/drm/i915/display/intel_dmc_wl.c | 14 ++++++++++++-- > > > drivers/gpu/drm/i915/display/intel_dmc_wl.h | 2 ++ > > > 3 files changed, 21 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > index adaf7cf3a33b..e8946ce86aaa 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > @@ -987,10 +987,14 @@ void gen9_disable_dc_states(struct intel_display *display) > > > return; > > > } > > > > > > - gen9_set_dc_state(display, DC_STATE_DISABLE); > > > - > > > - if (!HAS_DISPLAY(display)) > > > + if (HAS_DISPLAY(display)) { > > > + intel_dmc_wl_get_noreg(display); > > > + gen9_set_dc_state(display, DC_STATE_DISABLE); > > > + intel_dmc_wl_put_noreg(display); > > > + } else { > > > + gen9_set_dc_state(display, DC_STATE_DISABLE); > > > return; > > > + } > > > > I think intel_dmc_get/put() already protect indirectly on > > HAS_DISPLAY(), doesn't it? If that's the case, then the if here is > > unnecessary. > > Actually, intel_dmc_wl_init() gets called only when HAS_DISPLAY() is > true, so I think using intel_dmc_wl_{get,put}_noreg() for the > !HAS_DISPLAY() case would not be right, at least not with the current > state of the code. Okay, fair enough. -- Cheers, Luca.
diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c index adaf7cf3a33b..e8946ce86aaa 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c @@ -987,10 +987,14 @@ void gen9_disable_dc_states(struct intel_display *display) return; } - gen9_set_dc_state(display, DC_STATE_DISABLE); - - if (!HAS_DISPLAY(display)) + if (HAS_DISPLAY(display)) { + intel_dmc_wl_get_noreg(display); + gen9_set_dc_state(display, DC_STATE_DISABLE); + intel_dmc_wl_put_noreg(display); + } else { + gen9_set_dc_state(display, DC_STATE_DISABLE); return; + } intel_dmc_wl_disable(display); diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c index c298aef89449..5ed610c9be39 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c @@ -194,7 +194,7 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg) if (!__intel_dmc_wl_supported(display)) return; - if (!intel_dmc_wl_check_range(reg.reg)) + if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg)) return; spin_lock_irqsave(&wl->lock, flags); @@ -246,7 +246,7 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg) if (!__intel_dmc_wl_supported(display)) return; - if (!intel_dmc_wl_check_range(reg.reg)) + if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg)) return; spin_lock_irqsave(&wl->lock, flags); @@ -267,3 +267,13 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg) out_unlock: spin_unlock_irqrestore(&wl->lock, flags); } + +void intel_dmc_wl_get_noreg(struct intel_display *display) +{ + intel_dmc_wl_get(display, INVALID_MMIO_REG); +} + +void intel_dmc_wl_put_noreg(struct intel_display *display) +{ + intel_dmc_wl_put(display, INVALID_MMIO_REG); +} diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.h b/drivers/gpu/drm/i915/display/intel_dmc_wl.h index adab51208d0a..9aa72a4bf153 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.h +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.h @@ -27,5 +27,7 @@ void intel_dmc_wl_enable(struct intel_display *display); void intel_dmc_wl_disable(struct intel_display *display); void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg); void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg); +void intel_dmc_wl_get_noreg(struct intel_display *display); +void intel_dmc_wl_put_noreg(struct intel_display *display); #endif /* __INTEL_WAKELOCK_H__ */
Bspec says that disabling dynamic DC states require taking the DMC wakelock to cause an DC exit before writing to DC_STATE_EN. Implement that. In fact, testing on PTL revealed we end up failing to exit DC5/6 without this step. Bspec: 71583 Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> --- .../drm/i915/display/intel_display_power_well.c | 10 +++++++--- drivers/gpu/drm/i915/display/intel_dmc_wl.c | 14 ++++++++++++-- drivers/gpu/drm/i915/display/intel_dmc_wl.h | 2 ++ 3 files changed, 21 insertions(+), 5 deletions(-)