[Intel-gfx] Patch tag ordering (Was: Re: [PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module)

[Jani Nikula]

On Mon, 06 Nov 2017, Joonas Lahtinen <joonas.lahtinen at linux.intel.com> wrote:
> + Jani (and Daniel as emeritus maintainer)
>
> On Fri, 2017-11-03 at 10:08 -0700, Rodrigo Vivi wrote:
>> On Fri, Nov 03, 2017 at 08:36:01AM +0000, Joonas Lahtinen wrote:
>> > On Fri, 2017-11-03 at 00:03 +0000, Chris Wilson wrote:
>> > > Quoting Rodrigo Vivi (2017-11-02 23:52:45)
>> > > > On Wed, Oct 4, 2017 at 6:07 AM, Joonas Lahtinen
>> > > > <joonas.lahtinen at linux.intel.com> wrote:
>> > > > > On Tue, 2017-10-03 at 15:56 -0700, Sujaritha Sundaresan wrote:
>> > > > > > We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission".
>> > > > > > Whenever we need i915_modparams.enable_guc_submission=1, we also need enable_guc_loading=1.
>> > > > > > We also need enable_guc_loading=1 when we want to verify the HuC,
>> > > > > > which is every time we have a HuC (but all platforms with HuC have a GuC and viceversa).
>> > > > > 
>> > > > > Long lines in commit message, please give a look at:
>> > > > > 
>> > > > > https://www.kernel.org/doc/html/v4.13/process/submitting-patches.html
>> > > > > 
>> > > > > Section "14) The canonical patch format".
>> > > > > 
>> > > > > Then, about the patch. I think the commit message should be more clear
>> > > > > about the fact that if we have HuC firmware to be loaded, we need to
>> > > > > have GuC to actually load it. So if an user wants to avoid the GuC from
>> > > > > getting loaded, they must not have a HuC firmware to be loaded, in
>> > > > > addition to not using GuC submission.
>> > > > > 
>> > > > > > 
>> > > > > > v2: Clarifying the commit message (Anusha)
>> > > > > > 
>> > > > > > v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
>> > > > > > 
>> > > > > > v4: Rebase
>> > > > > > 
>> > > > > > v5: Separating message unification into a separate patch
>> > > > > > 
>> > > > > > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> > > > > > Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
>> > > > > > Cc: Oscar Mateo <oscar.mateo at intel.com>
>> > > > > > Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> > > > > > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
>> > > > > 
>> > > > > Try to keep the tags in chronological order, so start with Suggested-
>> > > > > by: (if any), Signed-off-by:, Cc: and so on.
>> > > > 
>> > > > Could we agree on have
>> > > > Suggested-by:
>> > > > Cc:
>> > > > Signed-off-by:
>> > > > as the initial chronological order and then follow the chronological
>> > > 
>> > > But CCs come after a s-o-b, because they are added after the commit. (I
>> > > write some code, then think who might be interested; usually by looking
>> > > at who previously worked on the same code). Then you also add new CCs
>> > > later on based on review feedback; a comment on v1 gets a CC on v2.
>> > > Bugzilla/reported-by/suggested-by are before since they presumably
>> > > prompted the commit to be written in the first place (plus also they
>> > > deserve extra credit for their effort in alerting us to the issue).
>> > 
>> > Yeah, this is my reasoning too.
>> 
>> So it seems the chronological order differs from case to case
>> from person to person.
>> When I write a patch most of the times I have people in mind
>> that I will cc. Like when I'm writing an email.
>> cc: people that touch this code from last time
>> cc: people that can help on review
>> cc: people that introduced this error
>> cc: people that will be futurely impacted by this change
>
> I don't follow this logic. Most of the Cc:s are chosen based on what
> the code does, in get_maintainers.pl fashion. I think one is set to
> implement a feature/bugfix, and it is not necessarily certain in the
> beginning where the code will land, could be core kernel, DRM or i915
> even. And Cc:s will vary accordingly depending on where the code
> landed.
>
> I can see an argument for some Cc:s before Signed-off-by:, but I never
> claimed that wouldn't be the case. Just that chronological ordering
> makes sense (will be easy to automate, too).
>
> I would claim that all of the four points you listed, you'll be looking
> at git blame based on what the code you wrote changed. And you don't
> know the whole scope of code in advance except for really small fixes.
> Where you wrote the code in your mind already. And it's really the IP
> you're S-o-b:ing. But this gets pretty theoritical already.
>
>> and then I sign-off on the end of the patch as I sign off in the
>> end of a message.
>> 
>> > 
>> > Also, when you add the machine assistance from Patchwork to
>> > automatically spread tags from the cover letter (Acked-by, Reviewed-by
>> > etc. and it's in the works, I understand). I don't quite see why we
>> > would have only a portion of the tags in chronological order.
>> > 
>> > If I respin a patch, it might already have:
>> > 
>> > Bugzilla:
>> > Suggested-by:
>> > Signed-off-by:
>> > Cc:
>> > Cc:
>> > Acked-by:
>> > Reviewed-by:
>> 
>> I really would like to have something like:
>> 
>> Bugzilla:
>> Suggested-by:
>> Cc:
>> Cc:
>> Signed-off-by:
>> Acked-by:
>> Reviewed-by:
>> 
>> This seems to be the most used in kernel.
>> the most intuitive and the easier to read.
>> 
>> The worst case this approach is creating is
>> 
>> Signed-off:
>> Cc:
>> Cc:
>> Cc:
>> 
>> really ugly on the first patch imho.
>> 
>> So, I doubt we can reach to an agreement. So let's
>> agree at least in not enforce this chronological thing
>> as a rule and let people use what ever they feel better.
>> 
>> Specially because I don't see any other place where
>> this is trying to get enforced like this.
>
> Well, I don't think we should be afraid to be the first to set an
> example.
>
> I think jointly agreeing on some form would be a good idea when we
> don't have a single maintainer that'd go around consolidating all the
> tag orders as they see fith.

I think I've only ever cared about two things wrt tag order: Getting the
Signed-off-bys in order because that has relevance in tracking the
origin of the patch, and putting my Signed-off-by last because that was
where everyone and their dog put it when I started doing kernel work.

If you want to encourage some uniform style, go for it, but please let's
not start nitpicking and having people resend their patches just because
they didn't put the tags in the order.

BR,
Jani.

>
>> 
>> Thanks
>> Signed-off: Rodrigo.
>
> I see what you did there ;)
>
> Regards, Joonas

-- 
Jani Nikula, Intel Open Source Technology Center