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@xxxxxxxxx> > > > --- > > > .../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.