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

Mon Nov 6 10:24:56 UTC 2017

+ 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.

> 
> Thanks
> Signed-off: Rodrigo.

I see what you did there ;)

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation