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
