Quoting Luca Coelho (2024-11-08 06:57:11-03:00) >On Thu, 2024-11-07 at 17:22 -0300, Gustavo Sousa wrote: >> 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". > >Right, I didn't pay much attention and conjugated it with >"information". > > >> > > > 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 :-) > >Yeah, I got that. :) > > >> 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; > >I think this is the simplest solution. > > >> > wakelock. >> > >> > Ugh... I don't like the latter very much... But, with those provided, I >> > believe we should be safe. >> > >> > Thoughts? > >I don't think it's a huge problem to initialize the spinlock even for >HW that doesn't use it. Yeah, it's a bit of wasteful in terms of >resources, but I think it's worth it because of the reduced complexity >of the implementation. Okay. Let's go with this solution then. So, to unblock this series, I decided to send v4 without this and the other patches related to using HAS_DMC() in HAS_DMC_WAKELOCK(). I'll send those in a new series. -- Gustavo Sousa > > >> > > > 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 :-) > >Definitely clearer, but I still think it may not be worth it. The >spinlock solution is very simple and clean, IMHO. We already have the >spinlock for other stuff, so it aligns well with the existing code. > >-- >Cheers, >Luca.
