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

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

[Jani Nikula]

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(-)

-- 
2.39.2

[Intel-gfx] [RFC 1/3] drm/i915: rough ideas for further separating display code from the rest

[Jani Nikula]

Long term goal: Separate display code from struct drm_i915_private and
i915_drv.h, and everything in them.

First step, draft some ideas how we could use struct intel_display as
the main device structure for display, while struct drm_device remains
in struct drm_i915_private (or, in the case of xe, in struct xe_device).

To get at struct drm_device * given a struct intel_display *, simply
store a backpointer.

To get at struct intel_display * given a struct drm_device *, require
storing a struct intel_display * right after struct drm_device in
memory. It's slightly hackish, but devm_drm_dev_alloc() facilitates
defining the enclosing struct as we wish.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 .../gpu/drm/i915/display/intel_display_core.h    | 16 ++++++++++++++++
 .../gpu/drm/i915/display/intel_display_device.c  | 13 +++++++++++++
 drivers/gpu/drm/i915/i915_drv.h                  | 11 ++++++++++-
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
index 53e5c33e08c3..a5463a9b5f54 100644
--- a/drivers/gpu/drm/i915/display/intel_display_core.h
+++ b/drivers/gpu/drm/i915/display/intel_display_core.h
@@ -14,6 +14,7 @@
 #include <linux/workqueue.h>
 
 #include <drm/drm_connector.h>
+#include <drm/drm_device.h>
 #include <drm/drm_modeset_lock.h>
 
 #include "intel_cdclk.h"
@@ -267,6 +268,9 @@ struct intel_wm {
 };
 
 struct intel_display {
+	/* drm device backpointer */
+	struct drm_device *drm;
+
 	/* Display functions */
 	struct {
 		/* Top level crtc-ish functions */
@@ -521,4 +525,16 @@ struct intel_display {
 	struct intel_wm wm;
 };
 
+/* FIXME: could be placed somewhere else to avoid drm/drm_device.h include */
+static inline struct intel_display *to_intel_display(const struct drm_device *drm)
+{
+	/*
+	 * Assume there's a pointer to struct intel_display in memory right
+	 * after struct drm_device.
+	 */
+	struct intel_display **p = (struct intel_display **)(drm + 1);
+
+	return *p;
+}
+
 #endif /* __INTEL_DISPLAY_CORE_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
index a6a18eae7ae8..b90da136ac65 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.c
+++ b/drivers/gpu/drm/i915/display/intel_display_device.c
@@ -921,6 +921,19 @@ void intel_display_device_probe(struct drm_i915_private *i915)
 	const struct intel_display_device_info *info;
 	u16 ver, rel, step;
 
+	/*
+	 * These are here for now to do them as early as possible. i915 has just
+	 * been allocated, drm isn't even initialized yet, but we have the
+	 * pointer.
+	 *
+	 * Later on, the display probe would allocate struct intel_display
+	 * itself, and return the pointer to the caller, for whom struct
+	 * intel_display would be an opaque type, a cookie to be passed on to
+	 * display functions.
+	 */
+	i915->__intel_display_private = &i915->display;
+	i915->display.drm = &i915->drm;
+
 	if (HAS_GMD_ID(i915))
 		info = probe_gmdid_display(i915, &ver, &rel, &step);
 	else
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 511eba3bbdba..10770fb5f429 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -193,7 +193,16 @@ struct i915_selftest_stash {
 };
 
 struct drm_i915_private {
-	struct drm_device drm;
+	struct {
+		struct drm_device drm;
+
+		/*
+		 * Display private data. Do *not* access directly. Must be
+		 * placed right after drm_device to facilitate getting to it
+		 * given a drm device pointer.
+		 */
+		struct intel_display *__intel_display_private;
+	} __packed;
 
 	struct intel_display display;
 
-- 
2.39.2

[Intel-gfx] [RFC 2/3] drm/i915/hdmi: drafting what struct intel_display usage would look like

[Jani Nikula]

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_hdmi.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index af4102e91769..f06d57b386c0 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -64,6 +64,11 @@ inline struct drm_i915_private *intel_hdmi_to_i915(struct intel_hdmi *intel_hdmi
 	return to_i915(hdmi_to_dig_port(intel_hdmi)->base.base.dev);
 }
 
+static inline struct intel_display *intel_hdmi_to_display(struct intel_hdmi *intel_hdmi)
+{
+	return to_intel_display(hdmi_to_dig_port(intel_hdmi)->base.base.dev);
+}
+
 static void
 assert_hdmi_port_disabled(struct intel_hdmi *intel_hdmi)
 {
@@ -1239,16 +1244,16 @@ static void hsw_set_infoframes(struct intel_encoder *encoder,
 
 void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable)
 {
-	struct drm_i915_private *dev_priv = intel_hdmi_to_i915(hdmi);
+	struct intel_display *display = intel_hdmi_to_display(hdmi);
 	struct i2c_adapter *ddc = hdmi->attached_connector->base.ddc;
 
 	if (hdmi->dp_dual_mode.type < DRM_DP_DUAL_MODE_TYPE2_DVI)
 		return;
 
-	drm_dbg_kms(&dev_priv->drm, "%s DP dual mode adaptor TMDS output\n",
+	drm_dbg_kms(display->drm, "%s DP dual mode adaptor TMDS output\n",
 		    enable ? "Enabling" : "Disabling");
 
-	drm_dp_dual_mode_set_tmds_output(&dev_priv->drm,
+	drm_dp_dual_mode_set_tmds_output(display->drm,
 					 hdmi->dp_dual_mode.type, ddc, enable);
 }
 
-- 
2.39.2

[Intel-gfx] [RFC 3/3] drm/i915: draft what feature test macros would look like

[Jani Nikula]

Obviously they'd be without underscore prefix.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_device.h | 4 ++++
 drivers/gpu/drm/i915/display/intel_hdmi.c           | 4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
index 44733c9d5812..4738d7c59703 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.h
+++ b/drivers/gpu/drm/i915/display/intel_display_device.h
@@ -32,6 +32,10 @@ struct drm_printer;
 	func(overlay_needs_physical); \
 	func(supports_tv);
 
+#define _DISPLAY_INFO(display)		((display)->info.__device_info)
+#define _DISPLAY_RUNTIME_INFO(display)	(&(display)->info.__runtime_info)
+#define _HAS_DDI(display)		(_DISPLAY_INFO(display)->has_ddi)
+
 #define HAS_4TILE(i915)			(IS_DG2(i915) || DISPLAY_VER(i915) >= 14)
 #define HAS_ASYNC_FLIPS(i915)		(DISPLAY_VER(i915) >= 5)
 #define HAS_CDCLK_CRAWL(i915)		(DISPLAY_INFO(i915)->has_cdclk_crawl)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index f06d57b386c0..efc398ad1b67 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2583,9 +2583,9 @@ static const struct drm_connector_funcs intel_hdmi_connector_funcs = {
 static int intel_hdmi_connector_atomic_check(struct drm_connector *connector,
 					     struct drm_atomic_state *state)
 {
-	struct drm_i915_private *i915 = to_i915(state->dev);
+	struct intel_display *display = to_intel_display(state->dev);
 
-	if (HAS_DDI(i915))
+	if (_HAS_DDI(display))
 		return intel_digital_connector_atomic_check(connector, state);
 	else
 		return g4x_hdmi_connector_atomic_check(connector, state);
-- 
2.39.2

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: split display from drm_i915_private and i915_drv.h

[Patchwork]

== Series Details ==

Series: drm/i915: split display from drm_i915_private and i915_drv.h
URL   : https://patchwork.freedesktop.org/series/124286/
State : warning

== Summary ==

Error: dim checkpatch failed
a84ff5b4caf4 drm/i915: rough ideas for further separating display code from the rest
3cba6ac324b0 drm/i915/hdmi: drafting what struct intel_display usage would look like
-:8: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

total: 0 errors, 1 warnings, 0 checks, 30 lines checked
0bc1adbe0bfd drm/i915: draft what feature test macros would look like

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: split display from drm_i915_private and i915_drv.h

[Patchwork]

== Series Details ==

Series: drm/i915: split display from drm_i915_private and i915_drv.h
URL   : https://patchwork.freedesktop.org/series/124286/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: split display from drm_i915_private and i915_drv.h

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 10297 bytes --]

== Series Details ==

Series: drm/i915: split display from drm_i915_private and i915_drv.h
URL   : https://patchwork.freedesktop.org/series/124286/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13682 -> Patchwork_124286v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_124286v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_124286v1, please notify your bug team (lgci.bug.filing@intel.com) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/index.html

Participating hosts (41 -> 40)
------------------------------

  Additional (1): fi-kbl-soraka 
  Missing    (2): bat-dg2-9 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_124286v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gt_mocs:
    - bat-adlp-9:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13682/bat-adlp-9/igt@i915_selftest@live@gt_mocs.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/bat-adlp-9/igt@i915_selftest@live@gt_mocs.html

  * igt@i915_selftest@live@workarounds:
    - fi-glk-j4005:       [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13682/fi-glk-j4005/igt@i915_selftest@live@workarounds.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/fi-glk-j4005/igt@i915_selftest@live@workarounds.html

  
Known issues
------------

  Here are the changes found in Patchwork_124286v1 that come from known issues:

### CI changes ###

#### Issues hit ####

  * boot:
    - fi-hsw-4770:        [PASS][5] -> [FAIL][6] ([i915#8293])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13682/fi-hsw-4770/boot.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/fi-hsw-4770/boot.html

  

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][7] ([fdo#109271] / [i915#2190])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][8] ([fdo#109271] / [i915#4613]) +3 other tests skip
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/fi-kbl-soraka/igt@gem_lmem_swapping@basic.html

  * igt@i915_module_load@load:
    - fi-kbl-soraka:      NOTRUN -> [DMESG-WARN][9] ([i915#1982])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/fi-kbl-soraka/igt@i915_module_load@load.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-kbl-soraka:      NOTRUN -> [DMESG-FAIL][10] ([i915#5334] / [i915#7872])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@gt_pm:
    - fi-kbl-soraka:      NOTRUN -> [DMESG-FAIL][11] ([i915#1886] / [i915#7913])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html

  * igt@kms_dsc@dsc-basic:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][12] ([fdo#109271]) +9 other tests skip
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/fi-kbl-soraka/igt@kms_dsc@dsc-basic.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
    - bat-adlp-9:         NOTRUN -> [SKIP][13] ([i915#3546]) +2 other tests skip
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/bat-adlp-9/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html

  * igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-c-dp-5:
    - bat-adlp-11:        [PASS][14] -> [ABORT][15] ([i915#8668])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13682/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-c-dp-5.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-c-dp-5.html

  
#### Possible fixes ####

  * igt@kms_chamelium_edid@hdmi-edid-read:
    - {bat-dg2-13}:       [DMESG-WARN][16] ([i915#7952]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13682/bat-dg2-13/igt@kms_chamelium_edid@hdmi-edid-read.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/bat-dg2-13/igt@kms_chamelium_edid@hdmi-edid-read.html

  * igt@kms_chamelium_frames@dp-crc-fast:
    - {bat-dg2-13}:       [DMESG-WARN][18] ([Intel XE#485]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13682/bat-dg2-13/igt@kms_chamelium_frames@dp-crc-fast.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/bat-dg2-13/igt@kms_chamelium_frames@dp-crc-fast.html

  * igt@kms_flip@basic-plain-flip@b-dp6:
    - bat-adlp-11:        [DMESG-WARN][20] ([i915#6868]) -> [PASS][21] +1 other test pass
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13682/bat-adlp-11/igt@kms_flip@basic-plain-flip@b-dp6.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/bat-adlp-11/igt@kms_flip@basic-plain-flip@b-dp6.html

  * igt@kms_flip@basic-plain-flip@c-dp6:
    - bat-adlp-11:        [FAIL][22] ([i915#6121]) -> [PASS][23] +8 other tests pass
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13682/bat-adlp-11/igt@kms_flip@basic-plain-flip@c-dp6.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/bat-adlp-11/igt@kms_flip@basic-plain-flip@c-dp6.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-bsw-nick:        [FAIL][24] ([i915#9276]) -> [PASS][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13682/fi-bsw-nick/igt@kms_frontbuffer_tracking@basic.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/fi-bsw-nick/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_hdmi_inject@inject-audio:
    - fi-kbl-guc:         [FAIL][26] ([IGT#3]) -> [PASS][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13682/fi-kbl-guc/igt@kms_hdmi_inject@inject-audio.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/fi-kbl-guc/igt@kms_hdmi_inject@inject-audio.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-c-dp-5:
    - bat-adlp-11:        [DMESG-FAIL][28] ([i915#6868]) -> [PASS][29]
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13682/bat-adlp-11/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-c-dp-5.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/bat-adlp-11/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-c-dp-5.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-d-dp-5:
    - bat-adlp-11:        [FAIL][30] ([i915#9047]) -> [PASS][31]
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13682/bat-adlp-11/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-d-dp-5.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/bat-adlp-11/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-d-dp-5.html

  * igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-b-dp-6:
    - bat-adlp-11:        [ABORT][32] ([i915#8668]) -> [PASS][33]
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13682/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-b-dp-6.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-b-dp-6.html

  
#### Warnings ####

  * igt@kms_force_connector_basic@force-edid:
    - bat-adlp-11:        [FAIL][34] ([i915#8803]) -> [SKIP][35] ([i915#4093])
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13682/bat-adlp-11/igt@kms_force_connector_basic@force-edid.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/bat-adlp-11/igt@kms_force_connector_basic@force-edid.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [IGT#3]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/3
  [Intel XE#485]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/485
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1886]: https://gitlab.freedesktop.org/drm/intel/issues/1886
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#4093]: https://gitlab.freedesktop.org/drm/intel/issues/4093
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6121]: https://gitlab.freedesktop.org/drm/intel/issues/6121
  [i915#6868]: https://gitlab.freedesktop.org/drm/intel/issues/6868
  [i915#7872]: https://gitlab.freedesktop.org/drm/intel/issues/7872
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7952]: https://gitlab.freedesktop.org/drm/intel/issues/7952
  [i915#8293]: https://gitlab.freedesktop.org/drm/intel/issues/8293
  [i915#8668]: https://gitlab.freedesktop.org/drm/intel/issues/8668
  [i915#8803]: https://gitlab.freedesktop.org/drm/intel/issues/8803
  [i915#9047]: https://gitlab.freedesktop.org/drm/intel/issues/9047
  [i915#9276]: https://gitlab.freedesktop.org/drm/intel/issues/9276


Build changes
-------------

  * Linux: CI_DRM_13682 -> Patchwork_124286v1

  CI-20190529: 20190529
  CI_DRM_13682: a42554bf0755b80fdfb8e91ca35ae6835bb3534d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7503: 7503
  Patchwork_124286v1: a42554bf0755b80fdfb8e91ca35ae6835bb3534d @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

f427353a8396 drm/i915: draft what feature test macros would look like
356446c9a5bb drm/i915/hdmi: drafting what struct intel_display usage would look like
7f9f7dbe61d4 drm/i915: rough ideas for further separating display code from the rest

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124286v1/index.html

[-- Attachment #2: Type: text/html, Size: 11728 bytes --]

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

[Matt Roper]

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
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation

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

[Jani Nikula]

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