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

[Lucas De Marchi]

On Wed, May 03, 2023 at 03:41:58PM -0400, Rodrigo Vivi wrote:
>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.

I wish git rebase understood a "drop" verb and git commit had
--fixup=drop:, just like it knows about --fixup=amend: and --fixup=reword:

Anyway, I think it works the way it is if you follow the advice from the
mesage above:

	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^".

i.e. once you hit this conflict, you do:

	git reset --hard HEAD^
	git rebase --skip

from my tests the --allow-empty do not really work: they only play a
role if the "pick" action from git rebase leaves the commit empty, not
if the "fixup" action. If it worked, then the rebase could be 2 passes:

	git rebase -i --allow-empty --autosquash ...
	git rebase -i --empty=drop ...

Lucas De Marchi