On Thu, 2024-11-07 at 17:47 -0300, Gustavo Sousa wrote: > Quoting Gustavo Sousa (2024-11-07 17:14:36-03:00) > > Quoting Luca Coelho (2024-11-07 16:23:06-03:00) > > > > > > 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. I'm not sure it's worth it, but if you feel this would be better, go ahead and show us the code. :) -- Cheers, Luca.
