Quoting Gustavo Sousa (2024-11-07 17:14:36-03:00) >Quoting Luca Coelho (2024-11-07 16:23:06-03:00) >>On Thu, 2024-11-07 at 15:27 -0300, Gustavo Sousa wrote: >>> There is a bit of a chicken and egg situation where we depend on runtime >>> info to know that DMC and wakelock are supported by the hardware, and >>> such information is grabbed via display MMIO functions, which in turns >>> call intel_dmc_wl_get() and intel_dmc_wl_put() as part of their regular >>> flow. >> >>s/which in turns call/which in turn calls/ > >Thanks! > >I'll do > > s/which in turns call/which in turn call/ > >as the subject for "call" is "display MMIO functions". > >> >> >>> Since we do not expect DC states (and consequently the wakelock >>> mechanism) to be enabled until DMC and DMC wakelock software structures >>> are initialized, a simple and safe solution to this is to turn >>> intel_dmc_wl_get() and intel_dmc_wl_put() into no-op until we have >>> properly initialized. >> >> >>About "safe" here... Can we be sure this will be race-free? > >The initialization is done only once, during driver load. The wakelock >will be enabled only at a later moment. So, we are good in that regard. > >However, now that you mentioned, yeah, we should also consider that that >we do concurrent work during initialization (e.g. loading the DMC). >Based on that, we will need to protect "initialized", which means: > >- initializing the lock early together with the other ones; >- always going for the lock, even for hardware that does not support the Oh, to be clear: I meant the spin lock here :-) Something along the lines of: diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c index 4257cc380475..e6d4f6328c33 100644 --- a/drivers/gpu/drm/i915/display/intel_display_driver.c +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c @@ -186,6 +186,7 @@ void intel_display_driver_early_probe(struct drm_i915_private *i915) return; spin_lock_init(&i915->display.fb_tracking.lock); + spin_lock_init(&i915->display.wl.lock); mutex_init(&i915->display.backlight.lock); mutex_init(&i915->display.audio.mutex); mutex_init(&i915->display.wm.wm_mutex); diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c index e43077453a99..bf8d3b04336d 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c @@ -307,11 +307,11 @@ void intel_dmc_wl_enable(struct intel_display *display, u32 dc_state) struct intel_dmc_wl *wl = &display->wl; unsigned long flags; - if (!__intel_dmc_wl_supported(display)) - return; - spin_lock_irqsave(&wl->lock, flags); + if (!__intel_dmc_wl_supported(display)) + goto out_unlock; + wl->dc_state = dc_state; if (drm_WARN_ON(display->drm, wl->enabled)) @@ -353,13 +353,13 @@ void intel_dmc_wl_disable(struct intel_display *display) struct intel_dmc_wl *wl = &display->wl; unsigned long flags; + spin_lock_irqsave(&wl->lock, flags); + if (!__intel_dmc_wl_supported(display)) - return; + goto out_unlock; flush_delayed_work(&wl->work); - spin_lock_irqsave(&wl->lock, flags); - if (drm_WARN_ON(display->drm, !wl->enabled)) goto out_unlock; @@ -389,13 +389,13 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg) struct intel_dmc_wl *wl = &display->wl; unsigned long flags; + spin_lock_irqsave(&wl->lock, flags); + if (!wl->initialized) - return; + goto out_unlock; if (!__intel_dmc_wl_supported(display)) - return; - - spin_lock_irqsave(&wl->lock, flags); + goto out_unlock; if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg, wl->dc_state)) goto out_unlock; @@ -424,13 +424,13 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg) struct intel_dmc_wl *wl = &display->wl; unsigned long flags; + spin_lock_irqsave(&wl->lock, flags); + if (!wl->initialized) - return; + goto out_unlock; if (!__intel_dmc_wl_supported(display)) - return; - - spin_lock_irqsave(&wl->lock, flags); + goto out_unlock; if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg, wl->dc_state)) goto out_unlock; -- Gustavo Sousa > wakelock. > >Ugh... I don't like the latter very much... But, with those provided, I >believe we should be safe. > >Thoughts? > >> >> >>> Let's implement that via a new field "initialized". Not that, since we >>> expect __intel_dmc_wl_supported() to be used for most non-static DMC >>> wakelock functions, let's add a drm_WARN_ONCE() there for when it is >>> called prior to initialization. >> >> >>s/not that/note that/ > >Thanks! > >> >> >>> The only exception of functions that can be called before initialization >>> are intel_dmc_wl_get() and intel_dmc_wl_put(), so we bail before >>> calling __intel_dmc_wl_supported() if not initialized. >>> >>> An alternative solution would be to revise MMIO-related stuff in the >>> whole driver initialization sequence, but that would possibly come with >>> the cost of some added ordering dependencies and complexity to the >>> source code. >> >>I think this can be kept out of the commit message. It's not very >>clear what you mean and it sounds much more complex than the solution >>you implemented. Unless race can really be an issue here, but then the >>whole commit message should be changed to an eventual more complex >>solution. > >I meant that we would need to revise the initialization code and find >the correct place to put the DMC Wakelock software initialization call. >That might also come with changes in some places where we do probe the >hardware for info: > > - We need our initialization to happen before > intel_display_device_info_runtime_init(), because we want to check > HAS_DMC(). > > - Currently, __intel_display_device_info_runtime_init() is using > intel_re_read(), which in turn uses > intel_dmc_wl_get()/intel_dmc_wl_put(). > > - The alternative solution to using the "initialized" flag would be to > make sure that function does not use the MMIO functions that take > the DMC wakelock path. > > - However, __intel_display_device_info_runtime_init() is not necessary > the only function that would need to be changed, but rather > basically everything that does MMIO before the initialization! > >I hope it is clearer now :-) > >-- >Gustavo Sousa