Re: [Intel-xe] [PATCH 00/18] xe&i915 display integration: add uncore and pcode compat layers

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, May 03, 2023 at 10:02:28AM -0400, Rodrigo Vivi wrote:
> On Wed, May 03, 2023 at 04:09:47PM +0300, Jani Nikula wrote:
> > Add intel_uncore.h and intel_pcode.h compat layers to direct the calls
> > in i915 display code properly, without needing to change a lot of call
> > sites in i915 display.
> > 
> > The main trick or hack here is adding a fake uncore member to struct
> > xe_device, which lets all the call sites use &i915->uncore as before,
> > which we can use in the glue layers to get at the struct xe_device
> > pointer.
> > 
> > The fake uncore is intentionally struct fake_uncore in xe, so you can't
> > really start passing around uncore pointers in display code, only do
> > &i915->uncore, and it'll be the correct, but different type for each
> > driver.
> > 
> > IMO the trick and the compat layers are cleaner than what we've had
> > before, and doesn't force using intel_de_* accessors for non-de
> > registers in i915 display code.
> 
> Nice clean-up.
> 
> Couple comments:
> 
> 1. I believe the fake uncore you created is better than convert entire xe
> to match i915 uncore.
> 
> 2. Does the fixup! as revert works well on autosquash? if so this is really
> neat... I was manually removing the revert and reverted patches on the
> clean-up rebases...

No, this doesn't play nicely with autosquash.
it is better to just use regular revert, then manually removing both patches
on the rebase clean-up...

result of some local experiments here:

You asked to amend the most recent commit, but doing so would make
it empty. You can repeat your command with --allow-empty, or you can
remove the commit entirely with "git reset HEAD^".
interactive rebase in progress; onto 78054149ebf8
Last commands done (14 commands done):
   pick 4358c97316a9 drm/i915/display: Add more macros to remove all direct calls to uncore
   fixup 98a5b4a593bc fixup! drm/i915/display: Add more macros to remove all direct calls to uncore
  (see more in file .git/rebase-merge/done)
Next commands to do (348 remaining commands):
   pick 5388ce02065b drm/i915/display: Remove all uncore mmio accesses in favor of intel_de
   fixup 6d8f5663eaaf fixup! drm/i915/display: Remove all uncore mmio accesses in favor of intel_de
  (use "git rebase --edit-todo" to view and edit)
You are currently rebasing branch 'drm-xe-next' on '78054149ebf8'.
  (all conflicts fixed: run "git rebase --continue")

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	drivers/gpu/drm/xe/

No changes
Could not apply 98a5b4a593bc... fixup! drm/i915/display: Add more macros to remove all direct calls to uncore

---

Then if I allow empty I have some bogus commits to get removed on a next round.
if I --skip then the offending code doesn't get removed.

> 
> > 
> > BR,
> > Jani.
> > 
> > 
> > Jani Nikula (18):
> >   fixup! drm/xe/display: Implement display support
> >   fixup! drm/xe/display: Implement display support
> >   fixup! drm/xe/display: Implement display support
> >   fixup! drm/xe/display: Rename intel_de.h to xe_de.h
> >   fixup! drm/xe/display: Implement display support
> >   fixup! drm/i915/display: Remaining changes to make xe compile
> >   fixup! drm/xe/display: Implement display support
> >   fixup! drm/i915/display: Add more macros to remove all direct calls to
> >     uncore
> >   fixup! drm/i915/display: Remove all uncore mmio accesses in favor of
> >     intel_de
> >   fixup! drm/i915/display: Remaining changes to make xe compile
> >   fixup! drm/xe: Introduce a new DRM driver for Intel GPUs
> >   fixup! drm/xe/display: Implement display support
> >   fixup! drm/i915/display: Remaining changes to make xe compile
> >   fixup! drm/i915/display: Remaining changes to make xe compile
> >   fixup! drm/xe/display: Implement display support
> >   fixup! drm/xe/display: Implement display support
> >   fixup! drm/xe/display: Implement display support
> >   fixup! drm/xe/display: Implement display support
> > 
> >  drivers/gpu/drm/i915/display/hsw_ips.c        |   7 +-
> >  drivers/gpu/drm/i915/display/intel_bios.c     |  19 +-
> >  drivers/gpu/drm/i915/display/intel_bw.c       |  34 ++--
> >  drivers/gpu/drm/i915/display/intel_cdclk.c    |  45 ++---
> >  drivers/gpu/drm/i915/display/intel_de.h       |  54 ------
> >  drivers/gpu/drm/i915/display/intel_display.c  |   1 +
> >  .../drm/i915/display/intel_display_power.c    |   3 +-
> >  .../i915/display/intel_display_power_well.c   |   7 +-
> >  drivers/gpu/drm/i915/display/intel_dpio_phy.c |   9 +-
> >  drivers/gpu/drm/i915/display/intel_hdcp.c     |   9 +-
> >  drivers/gpu/drm/i915/display/skl_watermark.c  |  23 +--
> >  .../gpu/drm/xe/compat-i915-headers/i915_drv.h |   1 +
> >  .../drm/xe/compat-i915-headers/intel_pcode.h  |  42 +++++
> >  .../drm/xe/compat-i915-headers/intel_uncore.h |  99 +++++++++++
> >  drivers/gpu/drm/xe/display/ext/i915_irq.c     | 134 +++++++-------
> >  .../drm/xe/display/ext/intel_clock_gating.c   |   1 +
> >  .../drm/xe/display/ext/intel_device_info.c    |   1 +
> >  drivers/gpu/drm/xe/display/ext/intel_dram.c   |   3 +-
> >  drivers/gpu/drm/xe/display/xe_de.h            | 163 ------------------
> >  drivers/gpu/drm/xe/xe_device_types.h          |   4 +
> >  drivers/gpu/drm/xe/xe_display.c               |   3 +
> >  drivers/gpu/drm/xe/xe_mmio.h                  |   8 +
> >  22 files changed, 307 insertions(+), 363 deletions(-)
> >  create mode 100644 drivers/gpu/drm/xe/compat-i915-headers/intel_pcode.h
> >  create mode 100644 drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
> >  delete mode 100644 drivers/gpu/drm/xe/display/xe_de.h
> > 
> > -- 
> > 2.39.2
> >