On Wed, Mar 06, 2024 at 02:24:34PM +0200, Jani Nikula wrote: > This is v2 of [1]. Improve the abstractions for display code. > > The main goals are: > > 1) The display code does not access struct drm_i915_private or struct > xe_device. It only uses its own struct intel_display instead. > > 2) The i915 and xe driver cores do not access struct intel_display > directly. It becomes an opaque pointer to them, stored in struct > drm_i915_private and struct xe_device, and passed to display code. > > This will mean a lot of churn, unfortunately. But it will better > separate the display code from the xe and i915 driver cores, and pave > the way for a) removing -Ddrm_i915_private=xe_device from xe Makefile, > and b) stop building the display code twice. > > What's presented here goes a long way, and could get us started.
I believe this series is a great start towards the separation goal and I liked the approach. Reviewed-by: Rodrigo Vivi <rodrigo.v...@intel.com> for the series. > But > there are still opens, such as: > > 1) How to handle platform checks such as IS_TIGERLAKE(). perhaps we can use display_ver and make up random release version table. display.verion = 12 display.release = / 0->TIGERLAKE 1->ALDERLAKE, etc... or just some specific display macros IS_TIGERLAKE_DISPLAY(display) > > 2) How to handle access to non-display members of i915/xe, such as > i915->uncore. Perhaps some driver registration callbacks with the mmio functions? the worst part here will be drivers/gpu/drm/i915/display/intel_vblank.c: __acquires(i915->uncore.lock) drivers/gpu/drm/i915/display/intel_vblank.c: spin_lock(&i915->uncore.lock); and for that perhaps we need some different lock? or a lock pointer along with the mmio calback on a registration struct? > > There are other similar things, but I believe those are the most > prevalent, and are the biggest blockers for converting a lot of > functions over from i915 -> intel_display. > > > BR, > Jani. > > [1] https://lore.kernel.org/r/cover.1695747484.git.jani.nik...@intel.com > > > Jani Nikula (4): > drm/i915/display: ideas for further separating display code from the > rest > drm/i915/display: add generic to_intel_display() macro > drm/i915/display: accept either i915 or display for feature tests > drm/i915/display: test various to_intel_display() scenarios > > .../gpu/drm/i915/display/intel_display_core.h | 3 ++ > .../drm/i915/display/intel_display_device.c | 13 ++++++ > .../drm/i915/display/intel_display_device.h | 10 +++- > .../drm/i915/display/intel_display_types.h | 46 +++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_dp.c | 6 +-- > drivers/gpu/drm/i915/display/intel_hdmi.c | 13 +++--- > drivers/gpu/drm/i915/i915_drv.h | 11 ++++- > drivers/gpu/drm/xe/xe_device_types.h | 15 +++++- > 8 files changed, 103 insertions(+), 14 deletions(-) > > -- > 2.39.2 >
