On Fri, May 06, 2022 at 10:23:41AM -0700, Lucas De Marchi wrote:
> On Thu, May 05, 2022 at 02:38:05PM -0700, Matt Roper wrote:
> > From: Stuart Summers <stuart.summers@xxxxxxxxx>
> >
> > Although we already strip 3D-specific flags from PIPE_CONTROL
> > instructions when submitting to a compute engine, there are some
> > additional flags that need to be removed when the platform as a whole
> > lacks a 3D pipeline. Add those restrictions here.
> >
> > Bspec: 47112
> > Signed-off-by: Stuart Summers <stuart.summers@xxxxxxxxx>
> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 18 ++++++++++++------
> > drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 12 ++++++++++--
> > drivers/gpu/drm/i915/i915_drv.h | 2 ++
> > drivers/gpu/drm/i915/i915_pci.c | 3 ++-
> > drivers/gpu/drm/i915/intel_device_info.h | 3 ++-
> > 5 files changed, 28 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > index 3e13960615bd..11c72792573d 100644
> > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > @@ -197,8 +197,10 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
> >
> > flags |= PIPE_CONTROL_CS_STALL;
> >
> > - if (engine->class == COMPUTE_CLASS)
> > - flags &= ~PIPE_CONTROL_3D_FLAGS;
> > + if (LACKS_3D_PIPELINE(engine->i915))
> > + flags &= ~PIPE_CONTROL_3D_ARCH_FLAGS;
> > + else if (engine->class == COMPUTE_CLASS)
> > + flags &= ~PIPE_CONTROL_3D_ENGINE_FLAGS;
> >
> > cs = intel_ring_begin(rq, 6);
> > if (IS_ERR(cs))
> > @@ -227,8 +229,10 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
> >
> > flags |= PIPE_CONTROL_CS_STALL;
> >
> > - if (engine->class == COMPUTE_CLASS)
> > - flags &= ~PIPE_CONTROL_3D_FLAGS;
> > + if (LACKS_3D_PIPELINE(engine->i915))
> > + flags &= ~PIPE_CONTROL_3D_ARCH_FLAGS;
> > + else if (engine->class == COMPUTE_CLASS)
> > + flags &= ~PIPE_CONTROL_3D_ENGINE_FLAGS;
> >
> > if (!HAS_FLAT_CCS(rq->engine->i915))
> > count = 8 + 4;
> > @@ -716,8 +720,10 @@ u32 *gen12_emit_fini_breadcrumb_rcs(struct i915_request *rq, u32 *cs)
> > /* Wa_1409600907 */
> > flags |= PIPE_CONTROL_DEPTH_STALL;
> >
> > - if (rq->engine->class == COMPUTE_CLASS)
> > - flags &= ~PIPE_CONTROL_3D_FLAGS;
> > + if (LACKS_3D_PIPELINE(rq->engine->i915))
> > + flags &= ~PIPE_CONTROL_3D_ARCH_FLAGS;
> > + else if (rq->engine->class == COMPUTE_CLASS)
> > + flags &= ~PIPE_CONTROL_3D_ENGINE_FLAGS;
> >
> > cs = gen12_emit_ggtt_write_rcs(cs,
> > rq->fence.seqno,
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> > index 556bca3be804..900755f4b787 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> > @@ -288,8 +288,8 @@
> > #define PIPE_CONTROL_DEPTH_CACHE_FLUSH (1<<0)
> > #define PIPE_CONTROL_GLOBAL_GTT (1<<2) /* in addr dword */
> >
> > -/* 3D-related flags can't be set on compute engine */
> > -#define PIPE_CONTROL_3D_FLAGS (\
> > +/* 3D-related flags that can't be set on _engines_ that lack a 3D pipeline */
> > +#define PIPE_CONTROL_3D_ENGINE_FLAGS (\
> > PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH | \
> > PIPE_CONTROL_DEPTH_CACHE_FLUSH | \
> > PIPE_CONTROL_TILE_CACHE_FLUSH | \
> > @@ -300,6 +300,14 @@
> > PIPE_CONTROL_VF_CACHE_INVALIDATE | \
> > PIPE_CONTROL_GLOBAL_SNAPSHOT_RESET)
> >
> > +/* 3D-related flags that can't be set on _platforms_ that lack a 3D pipeline */
> > +#define PIPE_CONTROL_3D_ARCH_FLAGS ( \
>
> names and comments here I think are confusing. In the code we do:
>
> if (LACKS_3D_PIPELINE(engine->i915))
> flags &= ~PIPE_CONTROL_3D_ARCH_FLAGS;
> else if (engine->class == COMPUTE_CLASS)
> flags &= ~PIPE_CONTROL_3D_ENGINE_FLAGS;
>
> coments give the _engines_ that lack a 3D pipeline doens't seem accurate
> as bcs, vcs, vecs also lack a 3d pipeline. Maybe be explicit about the
> flags being set on compute engine: PIPE_CONTROL_COMPUTE_ENGINE_FLAGS
>
> And LACKS_3D_PIPELINE... may be better to invert the condition to follow
> the other macros: HAS_3D_PIPELINE.
>
> > + PIPE_CONTROL_3D_ENGINE_FLAGS | \
> > + PIPE_CONTROL_INDIRECT_STATE_DISABLE | \
> > + PIPE_CONTROL_FLUSH_ENABLE | \
> > + PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE | \
> > + PIPE_CONTROL_DC_FLUSH_ENABLE)
> > +
> > #define MI_MATH(x) MI_INSTR(0x1a, (x) - 1)
> > #define MI_MATH_INSTR(opcode, op1, op2) ((opcode) << 20 | (op1) << 10 | (op2))
> > /* Opcodes for MI_MATH_INSTR */
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index b389674b5210..1e153cefc92e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1403,6 +1403,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> >
> > #define HAS_MBUS_JOINING(i915) (IS_ALDERLAKE_P(i915))
> >
> > +#define LACKS_3D_PIPELINE(i915) (INTEL_INFO(i915)->lacks_3d_pipeline)
> > +
> > /* i915_gem.c */
> > void i915_gem_init_early(struct drm_i915_private *dev_priv);
> > void i915_gem_cleanup_early(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> > index 07722cdf63ac..14e0e8225324 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -1077,7 +1077,8 @@ static const struct intel_device_info ats_m_info = {
> > #define XE_HPC_FEATURES \
> > XE_HP_FEATURES, \
> > .dma_mask_size = 52, \
> > - .has_l3_ccs_read = 1
> > + .has_l3_ccs_read = 1, \
> > + .lacks_3d_pipeline = 1
>
> isn't the absence of RCS sufficient so this could be done only in the
> macro rather than adding a flag here?
No, lacking an RCS is different than lacking a 3D pipeline. That's the
main point of this patch. Platforms like XEHPSDV don't have an RCS
engine (although it looks like we're still missing the patch that
removes it from the engine list...I need to send that) but
architecturally still have bits of a vestigial 3D pipeline somewhere
under the hood. Xe_HPC truly doesn't have the pipeline at all so the
rules are different, even for the CCS engines that wouldn't do 3D
anyway.
Matt
>
> Lucas De Marchi
>
> >
> > __maybe_unused
> > static const struct intel_device_info pvc_info = {
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> > index 09e33296157a..972084676984 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > @@ -165,7 +165,8 @@ enum intel_ppgtt_type {
> > func(has_snoop); \
> > func(has_coherent_ggtt); \
> > func(unfenced_needs_alignment); \
> > - func(hws_needs_physical);
> > + func(hws_needs_physical); \
> > + func(lacks_3d_pipeline);
> >
> > #define DEV_INFO_DISPLAY_FOR_EACH_FLAG(func) \
> > /* Keep in alphabetical order */ \
> > --
> > 2.35.1
> >
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
