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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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.

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:

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