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

[Gustavo Sousa <gustavo.sousa@intel.com>]

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

Well, a hacky way to mitigate this is by checking the DISPLAY_VER() >=
20 before taking the spin lock, since that info is queried in
probe_gmdid_display(), which happens at the "no-mmio" phase of driver
initialization.

By the way, that makes me think: is it too bad to do the same kind of
early MMIO via pci_iomap_range() for ICL_DFSM_DMC_DISABLE? We could
avoid this whole thing, since we would already have the correct value
for HAS_DMC() when i915/xe MMIO functions are called.

--
Gustavo Sousa

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