On Wed, 12 Apr 2023, "Souza, Jose" <jose.souza@xxxxxxxxx> wrote: > On Tue, 2023-04-11 at 14:20 -0700, Lucas De Marchi wrote: >> On Tue, Apr 11, 2023 at 08:07:12PM +0000, Jose Souza wrote: >> > On Tue, 2023-04-11 at 12:59 -0700, Lucas De Marchi wrote: >> > > On Tue, Apr 11, 2023 at 10:51:04AM -0400, Rodrigo Vivi wrote: >> > > > On Tue, Apr 11, 2023 at 12:14:36PM +0300, Jani Nikula wrote: >> > > > > On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: >> > > > > > On Tue, Apr 11, 2023 at 11:51:33AM +0300, Jani Nikula wrote: >> > > > > > > On Tue, 11 Apr 2023, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: >> > > > > > > > On Mon, Apr 10, 2023 at 11:32:14AM -0700, José Roberto de Souza wrote: >> > > > > > > > > Start to move the initialization of some lock from >> > > > > > > > > i915_driver_early_probe(). >> > > > > > > > > No dkl function is called prior to intel_setup_outputs(), so this is >> > > > > > > > > a good place to initialize it. >> > > > > > > > >> > > > > > > > I disagree. We don't want to sprinke these all over the place. >> > > > > > > >> > > > > > > I'm thinking if only foo.c uses a lock, foo.c should initialize it, not >> > > > > > > someone else. >> > > > > > >> > > > > > Perhaps. But I think there should be some consistent place in the higher >> > > > > > level code where all such things get called instead of dropping each one >> > > > > > individually into some random spot in the overlall display init flow. >> > > > > >> > > > > Agreed. >> > > > >> > > > Ops, I just saw this now, right after I cc'ed you in the other thread. >> > > > >> > > > So, probably good to hold this and do the entire refactor together of all >> > > > those locks initialization so we find this common consistent place apparently... >> > > >> > > "internal" sw initialization of display-related stuff. It doesn't belong in >> > > i915_driver_early_probe(), it makes harder to follow the sequence if we sprinkle >> > > them around, like here in intel_setup_outputs. >> > > >> > > But I don't see why this couldn't be done in a higher level "sw >> > > initialization of display-related stuff". Should we add an equivalent >> > > of i915_driver_early_probe(), e.g. intel_display_early_probe()[1], and >> > > move the display-related things from i915_driver_early_probe()? >> > > >> > > In that case, from xe we would call this function rather than >> > > initializing these fields in xe_display_create() >> > >> > Sent another version, added intel_display_locks_init() that is called in the beginning of intel_modeset_init_noirq(). >> > https://patchwork.freedesktop.org/series/116326/ >> >> modeset? why? That is after we are already probing the hw.... > > That called during probe, called from i915_driver_probe(). > >> and what does that have to do with modeset? > > The name is misleading but intel_modeset_init_noirq() is the first generic display initialization function called. > There is other display functions called before it but they are very specific(intel_dram_detect(), intel_bw_init_hw() and > intel_device_info_runtime_init()). See [1]. BR, Jani. [1] https://lore.kernel.org/r/87ile1cjh8.fsf@xxxxxxxxx > >> >> Lucas De Marchi >> >> > >> > If this is accepted we can then move the other display locks from i915_driver_early_probe(). >> > >> > > >> > > Lucas De Marchi >> > > >> > > [1] I don't like the name, but it follows what is already there >> > > >> > > > >> > > > > >> > > > > -- >> > > > > Jani Nikula, Intel Open Source Graphics Center >> > > -- Jani Nikula, Intel Open Source Graphics Center
