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

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

On Wed, 12 Nov 2025, Jani Nikula <jani.nikula@intel.com> wrote:
> 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>()?

Typed it up, I like it, sent as v3.

BR,
Jani.


-- 
Jani Nikula, Intel