Re: [Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

On Mon, 2022-11-21 at 11:39 +0000, Tvrtko Ursulin wrote:
> On 17/11/2022 22:34, Teres Alexis, Alan Previn wrote:
> > On Thu, 2022-11-17 at 11:02 -0500, Vivi, Rodrigo wrote:
> > > On Wed, Nov 16, 2022 at 04:30:13PM -0800, Alan Previn wrote:
> > > > In preparation for future MTL-PXP feature support, PXP control
> > > > context should only valid on the correct gt tile. Depending on the
> > > > device-info this depends on which tile owns the VEBOX and KCR.
> > > > PXP is still a global feature though (despite its control-context
> > > > located in the owning GT structure). Additionally, we find
> > > > that the HAS_PXP macro is only used within the pxp module,
> > > > 
> > > > That said, lets drop that HAS_PXP macro altogether and replace it
> > > > with a more fitting named intel_gtpxp_is_supported and helpers
> > > > so that PXP init/fini can use to verify if the referenced gt supports
> > > > PXP or teelink.
> > > 
> > > Yep, I understand you as I'm not fan of these macros, specially
> > > single usage. But we need to consider that we have multiple dependencies
> > > there and other cases like this in the driver... Well, but I'm not
> > > opposing, but probably better to first get rid of the macro,
> > > then change the behavior of the function on the next patch.
> > > 
> > > > 
> > > > Add TODO for Meteorlake that will come in future series.
> > > 
> > > This refactor patch should be standalone, without poputing it with
> > > the changes that didn't came yet to this point.
> > > 
> > Sure i can follow this rule, but it would then raise the question of "nothign is really changing anywhere for MTL, why
> > are we doing this" thats why i wanted to add that placeholder. I see "TODO"s are a common thing in the driver for larger
> > features that cant all be enabled at once. Respectfully and humbly, is there some documented rule? Can you show it to
> > me?
> 
> Separating refactoring from functional changes is one of the most basic 
> principles we follow (and not just us) and there should never be 
> pushback on the former due lack of functional changes.
> 
> On the basic level following this guideline makes it trivial to review 
> the refactoring patch, and in the vast majority of cases much easier to 
> review the functional change patch as well.
> 
> And easy to review means happy reviewers, faster turnaround time (easier 
> to carry a rebase), happier authors, easier reverts when things go bad 
> (only small functional patch needs to be reverted), sometimes even 
> easier backporting if code diverged a lot.
> 
> Oh, and it even has potential to decrease internal technical debt. Often 
> you can push refactoring upstream and keep a smaller delta behind the 
> closed doors, when that is required.
> 
> > > 
Okay got it - will remove that comment and ammend the commit msg to emphasis that this patch is for refactoring.