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
