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.
