After a more comprehensive offline discussion with Daniele and Rodrigo, design direction was made to go with Option2 where we elevate pxp to a global subsystem and within it it establish a pointer to the correct gt for pxp-controls. This also reflects the current HW architectures where the PXP feature (when viewed as a service for contexts and buffers) is global to all subsystems including any workload on any tile, despite its single control-knobs being only in the media tile (because PXP controls needs to be global to the GPU to avoid any races).
This would mean we move 'struct intel_pxp' to be under i915 so that all subsystems that need to call into PXP would now pass in i915 as its parameter. PXP internally would have a pointer to the correct GT (media-tile for MTL and gt0 for prior). It would also mean that certain code will still look a little kludgy or needs attention: - power-related operations like init/fini and suspend/resume would now need to called either before or after all-gt equivalents to ensure proper flow. - KCR IRQ will although being a gt level IRQ will now require passing i915 into the pxp subsystem. *NOTE: above list, even if i missed any, would still be nowhere near the amount of other external facing interfaces that would be called by global subsystems that would now look clean with i915->pxp as its param. ...alan On Tue, 2022-11-22 at 10:52 -0800, Alan Previn Teres Alexis wrote: > > On Tue, 2022-11-22 at 12:57 -0500, Vivi, Rodrigo wrote: > > > > > [Alan:snip] > > > As I had told I don't have a strong preference, as long as it keep clean > > and without these many helpers of something "on_gt"... > > > > If this stays inside the gt, just make sure that you call for all the gts, > > but then you inside the functions you can skip if pxp not enabled... without > > asking if enabled on_gt... > > > I think we have here conflicting requests. The "consumers" of pxp feature are > gem-execbuf, gem-context, gem-create (and > even display, for checking). Are we okay with making these callers be aware > of "if mtl, ensure i call intel_pxp_foo with > the media-tile's pxp, agnostic to the request, context or buffer i am dealing > with now". If you are okay with this, then > we can make this stay inside gt without "enabled on_gt" functions. But if > dont want to polute such low level backend > awareness into those upper layers, we need them to call something more global > like "intel_gpu_has_pxp_enabled" or > "intel_gpu_has_pxp_started" at the least with an i915 param. So that these > callers dont need to worry about it. Or > intel_pxp_enabled has to internally check with gt we are being passed with > and verify we are on the correct gt to - but > you said you dont want to have an "enabled on_gt" inside the pxp folder since > pxp is a subset of gt. The only thing i > can think of is just a rename - i.e. instead of "intel_pxp_enabled_on_gt", > have a "intel_gpu_has_pxp_enabled(i915)" - > but it would reside in the pxp folder. Would this work? > > > > > > > ...alan >
