Re: [Intel-gfx] [RFC 0/3] drm/i915: split display from drm_i915_private and i915_drv.h

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

On Fri, 20 Oct 2023, Matt Roper <matthew.d.roper@intel.com> wrote:
> On Tue, Sep 26, 2023 at 08:15:40PM +0300, Jani Nikula wrote:
>> We've been gradually separating display code from the rest of the i915
>> driver code over the past few years. We can't get much further than this
>> without some more drastic changes.
>> 
>> One of them is separating struct drm_i915_private and struct
>> intel_display almost completely. The former would remain for core driver
>> code and gem, while the latter would be for display. Long term, i915
>> display code would not include i915_drv.h, and would not have access to
>> struct drm_i915_private definion.
>> 
>> For display code, struct drm_i915_private would be opaque, and for the
>> rest of the driver, struct intel_display would be opaque.
>> 
>> Naturally, such separation helps the upcoming xe driver integration with
>> i915 display code. It's basically a requirement if (and that's still an
>> if) we decide to use aux-bus to have a i915.ko/xe.ko <->
>> intel-display.ko split. Regardless of that, I think this is a big
>> maintainability plus on its own too. The everything-includes-everything
>> model is really a nightmare.
>> 
>> Here are some draft ideas how this could be started. It will be a lot of
>> churn especially in the display code, but I believe the end result will
>> be cleaner.
>
> I'm guessing the plan is also to pass some kind of 'ops' callback
> structure down to intel_display when initializing a new device that it
> can use when it needs general functionality from the base driver
> (runtime PM, lowest-level register access, various memory management
> stuff, etc.)?

The auxiliary bus framework would provide a way to define callbacks in
both directions.

> In general, I'm very much in favor of making intel_display
> self-contained with no direct access to drm_i915_private / xe_device,
> and no direct calls outside of the display code.  I've been hoping we'd
> find the time to start moving in that direction.

It is a *lot* of work and churn in the drivers. There's no question
about it. Although this thread has been dead quiet, I'm sure there are a
lot of strong opinions and arguments against and in favour.

I think the other extreme I've seen suggested is to copy all the display
code from i915 to xe, fork it, axe out the unnecessary stuff, and roll
with two separate display drivers, with the i915 counterpart gradually
falling out of maintenance along with old platforms.

What we currently have in drm-xe-next is the non-committal middle
ground. I think we can live with it for a while, but it doesn't feel
like a permanent solution.

Maybe I should start documenting the alternatives with pros and cons to
support the decision.


BR,
Jani.


>
>
> Matt
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> Jani Nikula (3):
>>   drm/i915: rough ideas for further separating display code from the
>>     rest
>>   drm/i915/hdmi: drafting what struct intel_display usage would look
>>     like
>>   drm/i915: draft what feature test macros would look like
>> 
>>  .../gpu/drm/i915/display/intel_display_core.h    | 16 ++++++++++++++++
>>  .../gpu/drm/i915/display/intel_display_device.c  | 13 +++++++++++++
>>  .../gpu/drm/i915/display/intel_display_device.h  |  4 ++++
>>  drivers/gpu/drm/i915/display/intel_hdmi.c        | 15 ++++++++++-----
>>  drivers/gpu/drm/i915/i915_drv.h                  | 11 ++++++++++-
>>  5 files changed, 53 insertions(+), 6 deletions(-)
>> 
>> -- 
>> 2.39.2
>> 

-- 
Jani Nikula, Intel