[Intel-xe] [PATCH v2 0/5] drm/xe: uAPI cleanup

[Lucas De Marchi]

On Wed, May 31, 2023 at 07:02:51PM +0200, Francois Dugast wrote:
>On Wed, May 31, 2023 at 09:23:45AM -0700, Lucas De Marchi wrote:
>> +Oded
>>
>> On Wed, May 31, 2023 at 03:23:33PM +0000, Francois Dugast wrote:
>> > This series addresses some feedback on xe_drm.h provided here:
>> > https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
>> >
>> > It is based on xe-rev1-2023-05-02, not on drm-xe-next, so it will
>> > likely break CI.
>>
>> As I said on the previous reply, this is not going to fly. Now we have a
>> patch series against xe-rev1-2023-05-02, pushed as xe-rev2 branch with
>> *no recorded reviews*, doing things that already conflict with other
>> fixes that landed in drm-xe-next. Examples: the 32b compat for uapi,
>> the mmio refactor, the oob WAs, the probe refactor, etc etc.
>>
>> Also, there are other fixes queued as fixups, which is still the process
>> being used in drm-xe-next. It's also where CI is running. Due to display
>> being moved up in the branch it's basically the only sane thing to do.
>>
>> I think we need we are shooting ourselves on the foot and this review
>> process needs to be fixed / agreed upon before we create a mess out of
>> it.
>>
>> Lucas De Marchi
>
>Yes I understand the concern. The intention is to address Oded's feedback on the
>revision that was reviewed, so that the next review iteration is manageable and
>not based on a moving target, which could lead to and endless effort.

and then what are we going to do next? rebase the rest of drm-xe-next on top?
How do we guarantee the fixes on top of xe-rev is not breaking stuff?
What about the changes that are already there changing the commits that
are on xe-rev?

>
>Maybe I created confusion by sharing on the mailing list but at this stage the

no, that was the right part of it. Reviews on the patches happen on this
mailing list. The wrong part IMO was to push it as a branch before it
got the necessary review and before we know how we are going to
reconciliate things. Rebasing drm-xe-next on xe-rev doesn't
really work for the reasons mentioned above.

>idea is not to get this series merged, only to get Oded's validation that it
>fixes the issues that he reported. Yes this means more work will be required to
>rebase and create fixups later for drm-xe-next but it seems acceptable for the
>work that has been done so far.
>
>Do you see a better way that would still satisfy the "not moving target" goal?

0) Ignore the "not moving target" goal

1) The review should be on very a recent rev, not something old with a
    lot of commits on top. That doesn't scale and there will be another 100
    patches before the next review round.

	$ tag=xe-rev1-2023-05-02
	$ s=$(git log -1 --format=%s $tag)
	$ commit=$(git log --grep "$s" -n1 --format=%H)
	$ git rev-list $commit..HEAD | wc -l 
	208

    Then as outcome of the review we have more patches to land, rebase
    the rest, etc, etc.  This is a much worse "moving target" and IMO
    it's worse for both reviewers and committers.

2) The xe-revX should have the same baseline. That makes for a really
    simple way to check "what changed since I last looked". This can be
    checked on the overall diff with `git diff` or on the amended fixes
    with git range-diff.

    Currently drm-xe-next is on drm-tip due to the display integration.
    Being on drm-next is important for merging the branch upstream, but
    not so much for these kind of reviews

3) Not merge things in the xe-revX branch that is not already
    in drm-xe-next.  Ideally if there are things that really need to
    be changed back in the xe-rev branch, it should be a fixup on
    drm-xe-next that is then brought back to the xe-rev branch.


Lucas De Marchi

>
>Thanks,
>Francois
>
>>
>> >
>> > v2: fix ordering of defines and fields in uAPI (Lucas De Marchi)
>> >
>> > Francois Dugast (5):
>> >  drm/xe: Use SPDX-License-Identifier instead of license text
>> >  drm/xe: Group engine related structs
>> >  drm/xe: Move defines before relevant fields
>> >  drm/xe: Document usage of struct drm_xe_device_query
>> >  drm/xe: Fix some formatting issues in uAPI
>> >
>> > include/uapi/drm/xe_drm.h | 171 ++++++++++++++++++++------------------
>> > 1 file changed, 90 insertions(+), 81 deletions(-)
>> >
>> > --
>> > 2.34.1
>> >