diff mbox series

[v4,2/6] drm/i915/gt: Ensure memory quiesced before invalidation

Message ID 20230717173059.422892-3-andi.shyti@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Update AUX invalidation sequence | expand

Commit Message

Andi Shyti July 17, 2023, 5:30 p.m. UTC 
From: Jonathan Cavitt <jonathan.cavitt@intel.com>

All memory traffic must be quiesced before requesting
an aux invalidation on platforms that use Aux CCS.

Fixes: 972282c4cf24 ("drm/i915/gen12: Add aux table invalidate for all engines")
Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
Cc: <stable@vger.kernel.org> # v5.8+
---
 drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Matt Roper July 17, 2023, 5:54 p.m. UTC | #1 
On Mon, Jul 17, 2023 at 07:30:55PM +0200, Andi Shyti wrote:
> From: Jonathan Cavitt <jonathan.cavitt@intel.com>
> 
> All memory traffic must be quiesced before requesting
> an aux invalidation on platforms that use Aux CCS.
> 
> Fixes: 972282c4cf24 ("drm/i915/gen12: Add aux table invalidate for all engines")
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v5.8+
> ---
>  drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> index 563efee055602..bee3b7dc595cf 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> @@ -202,6 +202,13 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
>  {
>  	struct intel_engine_cs *engine = rq->engine;
>  
> +	/*
> +	 * Aux invalidations on Aux CCS platforms require
> +	 * memory traffic is quiesced prior.
> +	 */
> +	if ((mode & EMIT_INVALIDATE) && !HAS_FLAT_CCS(engine->i915))

It's a pre-existing mistake in drm-tip at the moment, but we shouldn't
assume !flatccs always implies auxccs.  PVC has neither, and there may
be other similar platforms in the future.  We should probably add a
helper function for AuxCCS, similar to what we added to the Xe driver
recently:

https://patchwork.freedesktop.org/patch/539304/?series=118334&rev=1


Matt


> +		mode |= EMIT_FLUSH;
> +
>  	if (mode & EMIT_FLUSH) {
>  		u32 flags = 0;
>  		int err;
> -- 
> 2.40.1
>
Matt Roper July 17, 2023, 8:31 p.m. UTC | #2 
On Mon, Jul 17, 2023 at 10:54:37AM -0700, Matt Roper wrote:
> On Mon, Jul 17, 2023 at 07:30:55PM +0200, Andi Shyti wrote:
> > From: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > 
> > All memory traffic must be quiesced before requesting
> > an aux invalidation on platforms that use Aux CCS.
> > 
> > Fixes: 972282c4cf24 ("drm/i915/gen12: Add aux table invalidate for all engines")
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v5.8+
> > ---
> >  drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > index 563efee055602..bee3b7dc595cf 100644
> > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > @@ -202,6 +202,13 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
> >  {
> >  	struct intel_engine_cs *engine = rq->engine;
> >  
> > +	/*
> > +	 * Aux invalidations on Aux CCS platforms require
> > +	 * memory traffic is quiesced prior.
> > +	 */
> > +	if ((mode & EMIT_INVALIDATE) && !HAS_FLAT_CCS(engine->i915))
> 
> It's a pre-existing mistake in drm-tip at the moment, but we shouldn't
> assume !flatccs always implies auxccs.  PVC has neither, and there may
> be other similar platforms in the future.  We should probably add a
> helper function for AuxCCS, similar to what we added to the Xe driver
> recently:
> 
> https://patchwork.freedesktop.org/patch/539304/?series=118334&rev=1
> 

BTW, since this patch didn't handle it I was expecting to see another
patch in the series that quiesces memory for the non-RCS/CCS engines,
but it looks like there isn't one yet.  So we should probably add the
necessary MI_FLUSH_DW logic for the other engines to this patch as well.


Matt

> 
> Matt
> 
> 
> > +		mode |= EMIT_FLUSH;
> > +
> >  	if (mode & EMIT_FLUSH) {
> >  		u32 flags = 0;
> >  		int err;
> > -- 
> > 2.40.1
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation
Andi Shyti July 17, 2023, 9:52 p.m. UTC | #3 
Hi Matt,

On Mon, Jul 17, 2023 at 01:31:03PM -0700, Matt Roper wrote:
> On Mon, Jul 17, 2023 at 10:54:37AM -0700, Matt Roper wrote:
> > On Mon, Jul 17, 2023 at 07:30:55PM +0200, Andi Shyti wrote:
> > > From: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > > 
> > > All memory traffic must be quiesced before requesting
> > > an aux invalidation on platforms that use Aux CCS.
> > > 
> > > Fixes: 972282c4cf24 ("drm/i915/gen12: Add aux table invalidate for all engines")
> > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> > > Cc: <stable@vger.kernel.org> # v5.8+
> > > ---
> > >  drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > > index 563efee055602..bee3b7dc595cf 100644
> > > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > > @@ -202,6 +202,13 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
> > >  {
> > >  	struct intel_engine_cs *engine = rq->engine;
> > >  
> > > +	/*
> > > +	 * Aux invalidations on Aux CCS platforms require
> > > +	 * memory traffic is quiesced prior.
> > > +	 */
> > > +	if ((mode & EMIT_INVALIDATE) && !HAS_FLAT_CCS(engine->i915))
> > 
> > It's a pre-existing mistake in drm-tip at the moment, but we shouldn't
> > assume !flatccs always implies auxccs.  PVC has neither, and there may
> > be other similar platforms in the future.  We should probably add a
> > helper function for AuxCCS, similar to what we added to the Xe driver
> > recently:
> > 
> > https://patchwork.freedesktop.org/patch/539304/?series=118334&rev=1

Currently that is done in patch 6...

> BTW, since this patch didn't handle it I was expecting to see another
> patch in the series that quiesces memory for the non-RCS/CCS engines,
> but it looks like there isn't one yet.  So we should probably add the
> necessary MI_FLUSH_DW logic for the other engines to this patch as well.

... where also other engines are handles as well. I left this
patch as it is in order to preserve the authorship and it's
original form.

Maybe in patch 6 I can add the extra check for PVC as you did for
XE.

Thanks,
Andi
Matt Roper July 17, 2023, 10 p.m. UTC | #4 
On Mon, Jul 17, 2023 at 11:52:25PM +0200, Andi Shyti wrote:
> Hi Matt,
> 
> On Mon, Jul 17, 2023 at 01:31:03PM -0700, Matt Roper wrote:
> > On Mon, Jul 17, 2023 at 10:54:37AM -0700, Matt Roper wrote:
> > > On Mon, Jul 17, 2023 at 07:30:55PM +0200, Andi Shyti wrote:
> > > > From: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > > > 
> > > > All memory traffic must be quiesced before requesting
> > > > an aux invalidation on platforms that use Aux CCS.
> > > > 
> > > > Fixes: 972282c4cf24 ("drm/i915/gen12: Add aux table invalidate for all engines")
> > > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > > > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> > > > Cc: <stable@vger.kernel.org> # v5.8+
> > > > ---
> > > >  drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > > > index 563efee055602..bee3b7dc595cf 100644
> > > > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > > > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > > > @@ -202,6 +202,13 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
> > > >  {
> > > >  	struct intel_engine_cs *engine = rq->engine;
> > > >  
> > > > +	/*
> > > > +	 * Aux invalidations on Aux CCS platforms require
> > > > +	 * memory traffic is quiesced prior.
> > > > +	 */
> > > > +	if ((mode & EMIT_INVALIDATE) && !HAS_FLAT_CCS(engine->i915))
> > > 
> > > It's a pre-existing mistake in drm-tip at the moment, but we shouldn't
> > > assume !flatccs always implies auxccs.  PVC has neither, and there may
> > > be other similar platforms in the future.  We should probably add a
> > > helper function for AuxCCS, similar to what we added to the Xe driver
> > > recently:
> > > 
> > > https://patchwork.freedesktop.org/patch/539304/?series=118334&rev=1
> 
> Currently that is done in patch 6...

Are you sure?  Patch #6 consolidates things a bit, but is still incorrectly
assuming flatccs = !auxccs:

       if (HAS_FLAT_CCS(engine->i915))                                                                                                            
               return _MMIO(0);                                                                                                                   

> 
> > BTW, since this patch didn't handle it I was expecting to see another
> > patch in the series that quiesces memory for the non-RCS/CCS engines,
> > but it looks like there isn't one yet.  So we should probably add the
> > necessary MI_FLUSH_DW logic for the other engines to this patch as well.
> 
> ... where also other engines are handles as well. I left this
> patch as it is in order to preserve the authorship and it's
> original form.

I don't see it being handled in patch #6.  That performs invalidation on
more engines than we were before, but it doesn't add the missing quiesce
logic as far as I can see.


Matt

> 
> Maybe in patch 6 I can add the extra check for PVC as you did for
> XE.
> 
> Thanks,
> Andi
Andi Shyti July 18, 2023, 12:28 a.m. UTC | #5 
Hi Matt,

> > > > > +	/*
> > > > > +	 * Aux invalidations on Aux CCS platforms require
> > > > > +	 * memory traffic is quiesced prior.
> > > > > +	 */
> > > > > +	if ((mode & EMIT_INVALIDATE) && !HAS_FLAT_CCS(engine->i915))
> > > > 
> > > > It's a pre-existing mistake in drm-tip at the moment, but we shouldn't
> > > > assume !flatccs always implies auxccs.  PVC has neither, and there may
> > > > be other similar platforms in the future.  We should probably add a
> > > > helper function for AuxCCS, similar to what we added to the Xe driver
> > > > recently:
> > > > 
> > > > https://patchwork.freedesktop.org/patch/539304/?series=118334&rev=1
> > 
> > Currently that is done in patch 6...
> 
> Are you sure?  Patch #6 consolidates things a bit, but is still incorrectly
> assuming flatccs = !auxccs:
> 
>        if (HAS_FLAT_CCS(engine->i915))                                                                                                            
>                return _MMIO(0);                                                                                                                   

But isn't it the same the patch you linked is doing?

	return !xe->info.has_flat_ccs;

And
Matt Roper July 18, 2023, 3:53 p.m. UTC | #6 
On Tue, Jul 18, 2023 at 02:28:26AM +0200, Andi Shyti wrote:
> Hi Matt,
> 
> > > > > > +	/*
> > > > > > +	 * Aux invalidations on Aux CCS platforms require
> > > > > > +	 * memory traffic is quiesced prior.
> > > > > > +	 */
> > > > > > +	if ((mode & EMIT_INVALIDATE) && !HAS_FLAT_CCS(engine->i915))
> > > > > 
> > > > > It's a pre-existing mistake in drm-tip at the moment, but we shouldn't
> > > > > assume !flatccs always implies auxccs.  PVC has neither, and there may
> > > > > be other similar platforms in the future.  We should probably add a
> > > > > helper function for AuxCCS, similar to what we added to the Xe driver
> > > > > recently:
> > > > > 
> > > > > https://patchwork.freedesktop.org/patch/539304/?series=118334&rev=1
> > > 
> > > Currently that is done in patch 6...
> > 
> > Are you sure?  Patch #6 consolidates things a bit, but is still incorrectly
> > assuming flatccs = !auxccs:
> > 
> >        if (HAS_FLAT_CCS(engine->i915))                                                                                                            
> >                return _MMIO(0);                                                                                                                   
> 
> But isn't it the same the patch you linked is doing?
> 
> 	return !xe->info.has_flat_ccs;

No, that's just the end of the function.  The important
platform-specific checks come before that point (at the moment we only
have PVC, but we expect more platforms to be added there very soon too).


Matt

> 
> And
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
index 563efee055602..bee3b7dc595cf 100644
--- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
@@ -202,6 +202,13 @@  int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
 {
 	struct intel_engine_cs *engine = rq->engine;
 
+	/*
+	 * Aux invalidations on Aux CCS platforms require
+	 * memory traffic is quiesced prior.
+	 */
+	if ((mode & EMIT_INVALIDATE) && !HAS_FLAT_CCS(engine->i915))
+		mode |= EMIT_FLUSH;
+
 	if (mode & EMIT_FLUSH) {
 		u32 flags = 0;
 		int err;