[PATCH v3 17/18] drm/i915/dmc_wl: Do nothing until initialized

[Gustavo Sousa]

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
  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