| Message ID | cover.1695747484.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/i915: split display from drm_i915_private and i915_drv.h | expand |
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.)? 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. 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 >
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 >>
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. 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(-)