Re: [PATCH v2 05/12] drm/i915/pvc: Remove additional 3D flags from PIPE_CONTROL

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

No, that's what we're trying to avoid here.  Presence/absence of the 3D
pipeline is a characteristic of the platform, not of the engine.  A CCS
engine doesn't have access to the 3D pipeline on any platform (that's
what makes it different from an RCS), but we still wind up needing to
program the PIPE_CONTROL flags for a CCS engine differently on a
platform like PVC (which doesn't have a 3D pipeline at all) vs Xe_HP SDV
(which does have a vestigial 3D pipeline, even though it isn't directly
usable).

Basically:

        RCS:
             All flags used
        CCS on Xe_HP SDV or DG2:
             Strip out flags specific to RCS engines
        CCS on PVC:
             Strip out flags that only exist on platforms with an
             architectural 3D pipeline *and* strip out flags specific to
             RCS engines

> 
> And LACKS_3D_PIPELINE... may be better to invert the condition to follow
> the other macros: HAS_3D_PIPELINE.

I think the code was written that way originally, but we flipped it
because adding the flag to every single platform from gen2 onward
obfuscated the fact that the absence of a standard hardware block was a
notable characteristic our code needs to look for, rather than the
presence of something.  The conditions in the code will always be
written in the inverted form "!HAS_3D_PIPELINE" so reversing it and
making it a positive "LACKS" seemed more natural.  But I can switch it
back and we can see how that looks.

> 
> > +		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, absence of RCS does not imply absence of the 3D pipeline, just that
there's no engine that can use it.  Xe_HP SDV is still based on an
architecture with a 3D pipeline, even though it doesn't have an RCS
engine to access it, so its CCS engines have different PIPE_CONTROL
requirements than PVC's CCS engines.


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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux