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@xxxxxxxxx>
> ---
Reviewed-by: Luca Coelho <luciano.coelho@xxxxxxxxx>
--
Cheers,
Luca.
