Re: [PATCH v2 3/3] drm/{i915,xe}/display: move irq calls to parent interface

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 11 Nov 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> 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