Re: [PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module

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

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@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@intel.com>
> > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > > > > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > > > > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@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

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.

Thanks
Signed-off: Rodrigo.

> 
> By adding my Signed-off-by at the end and that's the only way to retain
> that history information correctly.
> 
> And it's an easy convention to follow for a developer. You only need to
> to write above the automatically generated S-o-b, if you reference a
> bug or attribute credit (because that's literally what happened first
> in chronological order, too). From then on, you just append at the end.
> 
> All the minutes spent thinking how to correctly order the tags can be
> recouped as moar patches.
> 
> Regards, Joonas
> -- 
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx