On Tue, 11 Nov 2025, Ville Syrjälä <[email protected]> wrote: > On Tue, Nov 11, 2025 at 09:34:04AM +0200, Jani Nikula wrote: >> Add an irq parent driver interface for the .enabled and .synchronize >> calls. This lets us drop the dependency on i915_drv.h and i915_irq.h in >> multiple places, and subsequently remove the compat i915_irq.h and >> i915_irq.c files along with the display/ext directory from xe >> altogether. >> >> Use intel_irqs_enabled() and intel_synchronize_irq() static wrappers for >> parent interface calls in intel_display_irq.c while chasing the function >> pointers everywhere else. It's still a bit uncertain if we should >> universally have wrappers for the parent interface calls or not. > > Why would we want those ugly pointer chains anywhere (except perhaps > for single use cases)?
I guess my main point is, I'm not yet sure how those wrappers should be organized, and I'm simply postponing the decision. But let's discuss. I'm leaning towards putting them in files that are separate from the implementation, i.e. intel_irqs_enabled() and intel_synchronize_irq() would *not* be in intel_display_irq.[ch] but rather in something_parent_something.[ch]. Because it's not so much about the functionality, it's all about calling the parent interface. Maybe just one file grouping *all* parent interface access, similar to how include/drm/intel/display_parent_interface.h defines the interface. And then should the naming indicate it's calling the parent? I think there's value in knowing where the call ends up when reading the code. But then do the function names end up being unweildy? If you simply translate display->parent->irq->enabled to a function name, it would be display_parent_irq_enabled(). Or perhaps intel_parent_irq_enabled(). And you'd have the naming scheme right there, always 1:1 without further thinking. intel_display_rpm.[ch] has it all in a dedicated file now. But there's really nothing runtime pm specific there anymore, it's just mechanical calling of the interface and relaying parameters and return values. I think we have too much "intel_display_something_something.[ch]" so maybe intel_parent.[ch] and intel_parent_<SUBSTRUCT>_<FUNCTION>()? BR, Jani. -- Jani Nikula, Intel
