[PATCH 09/13] drm/i915/dmc_wl: Deal with existing references when disabling

[Luca Coelho]

On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> It is possible that there are active wakelock references at the time we
> are disabling the DMC wakelock mechanism. We need to deal with that in
> two ways:
> 
> (A) Implement the missing step from Bspec:
> 
>     The Bspec instructs us to clear any existing wakelock request bit
>     after disabling the mechanism. That gives a clue that it is okay to
>     disable while there are locks held and we do not need to wait for
>     them. However, since the spec is not explicit about it, we need
>     still to get confirmation with the hardware team. Let's thus
>     implement the spec and add a TODO note.
> 
> (B) Ensure a consistent driver state:
> 
>     The enable/disable logic would be problematic if the following
>     sequence of events would happen:
> 
>     1. Function A calls intel_dmc_wl_get();
>     2. Some function calls intel_dmc_wl_disable();
>     3. Some function calls intel_dmc_wl_enable();
>     4. Function A is done and calls intel_dmc_wl_put().
> 
>     At (2), the refcount becomes zero and then (4) causes an invalid
>     decrement to the refcount. That would cause some issues:
> 
>         - At the time between (3) and (4), function A would think that
>           the hardware lock is held but it could not be really held
>           until intel_dmc_wl_get() is called by something else.
>         - The call made to (4) could cause the refcount to become zero
>           and consequently the hardware lock to be released while there
>           could be innocent paths trusting they still have the lock.
> 
>     To fix that, we need to keep the refcount correctly in sync with
>     intel_dmc_wl_{get,put}() calls and retake the hardware lock when
>     enabling the DMC wakelock with a non-zero refcount.
> 
>     One missing piece left to be handled here is the following scenario:
> 
>     1. Function A calls intel_dmc_wl_get();
>     2. Some function calls intel_dmc_wl_disable();
>     3. Some function calls intel_dmc_wl_enable();
>     4. Concurrently with (3), function A performs the MMIO in between
>        setting DMC_WAKELOCK_CFG_ENABLE and asserting the lock with
>        __intel_dmc_wl_take().
> 
>     I'm mostly sure this would cause issues future display IPs if DMC
>     trap implementation was completely removed. We need to check with
>     the hardware team whether it would be safe to assert the hardware
>     lock before setting DMC_WAKELOCK_CFG_ENABLE to avoid this scenario.
>     If not, then we would have to deal with that via software
>     synchronization.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
> ---

Reviewed-by: Luca Coelho <luciano.coelho at intel.com>

--
Cheers,
Luca.