agp/intel: Flush all chipset writes after updating the GGTT

Submitted by Chris Wilson on Dec. 8, 2017, 9:46 p.m.

Details

Message ID 20171208214616.30147-1-chris@chris-wilson.co.uk
State Accepted
Commit 8516673a996870ea0ceb337ee4f83c33c5ec3111
Headers show
Series "agp/intel: Flush all chipset writes after updating the GGTT" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Chris Wilson Dec. 8, 2017, 9:46 p.m.
Before accessing the GGTT we must flush the PTE writes and make them
visible to the chipset, or else the indirect access may end up in the
wrong page. In commit 3497971a71d8 ("agp/intel: Flush chipset writes
after updating a single PTE"), we noticed corruption of the uploads for
pwrite and for capturing GPU error states, but it was presumed that the
explicit calls to intel_gtt_chipset_flush() were sufficient for the
execbuffer path. However, we have not been flushing the chipset between
the PTE writes and access via the GTT itself.

For simplicity, do the flush after any PTE update rather than try and
batch the flushes on a just-in-time basis.

References: 3497971a71d8 ("agp/intel: Flush chipset writes after updating a single PTE")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: drm-intel-fixes@lists.freedesktop.org
---
 drivers/char/agp/intel-gtt.c | 2 ++
 1 file changed, 2 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index 9b6b6023193b..dde7caac7f9f 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -872,6 +872,8 @@  void intel_gtt_insert_sg_entries(struct sg_table *st,
 		}
 	}
 	wmb();
+	if (intel_private.driver->chipset_flush)
+		intel_private.driver->chipset_flush();
 }
 EXPORT_SYMBOL(intel_gtt_insert_sg_entries);
 

Comments

On Fri, 2017-12-08 at 21:46 +0000, Chris Wilson wrote:
> Before accessing the GGTT we must flush the PTE writes and make them
> visible to the chipset, or else the indirect access may end up in the
> wrong page. In commit 3497971a71d8 ("agp/intel: Flush chipset writes
> after updating a single PTE"), we noticed corruption of the uploads for
> pwrite and for capturing GPU error states, but it was presumed that the
> explicit calls to intel_gtt_chipset_flush() were sufficient for the
> execbuffer path. However, we have not been flushing the chipset between
> the PTE writes and access via the GTT itself.
> 
> For simplicity, do the flush after any PTE update rather than try and
> batch the flushes on a just-in-time basis.
> 
> References: 3497971a71d8 ("agp/intel: Flush chipset writes after updating a single PTE")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: drm-intel-fixes@lists.freedesktop.org

I don't think this is being used so much anymore? (+ Jani for this)

Why not Cc: stable? My DIM says # v4.9+

> +++ b/drivers/char/agp/intel-gtt.c
> @@ -872,6 +872,8 @@ void intel_gtt_insert_sg_entries(struct sg_table *st,
>  		}
>  	}
>  	wmb();
> +	if (intel_private.driver->chipset_flush)
> +		intel_private.driver->chipset_flush();

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
Quoting Joonas Lahtinen (2017-12-11 10:55:40)
> On Fri, 2017-12-08 at 21:46 +0000, Chris Wilson wrote:
> > Before accessing the GGTT we must flush the PTE writes and make them
> > visible to the chipset, or else the indirect access may end up in the
> > wrong page. In commit 3497971a71d8 ("agp/intel: Flush chipset writes
> > after updating a single PTE"), we noticed corruption of the uploads for
> > pwrite and for capturing GPU error states, but it was presumed that the
> > explicit calls to intel_gtt_chipset_flush() were sufficient for the
> > execbuffer path. However, we have not been flushing the chipset between
> > the PTE writes and access via the GTT itself.
> > 
> > For simplicity, do the flush after any PTE update rather than try and
> > batch the flushes on a just-in-time basis.
> > 
> > References: 3497971a71d8 ("agp/intel: Flush chipset writes after updating a single PTE")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: drm-intel-fixes@lists.freedesktop.org
> 
> I don't think this is being used so much anymore? (+ Jani for this)
> 
> Why not Cc: stable? My DIM says # v4.9+

I don't use stable@ anymore since Greg doesn't like our patches and
would much prefer to pick randomly instead. /slightly-s

So I leave that management to you guys.
-Chris