[PATCH v2 0/1] Drop wbinvd_on_all_cpus usage

[PATCH v2 0/1] Drop wbinvd_on_all_cpus usage

[Michael Cheng]

To align with the discussion in [1][2], this patch series drops all usage of    
wbvind_on_all_cpus within i915 by either replacing the call with certain        
drm clflush helpers, or reverting to a previous logic.                          
                                                                                 
[1]. https://lists.freedesktop.org/archives/dri-devel/2021-November/330928.html 
[2]. https://patchwork.freedesktop.org/patch/475752/?series=99991&rev=5 

v2 (Michael Cheng) : Per feedback from Thomas and Tvrtko, this series should be
split into two parts; First part is to unblock i915 from build errors and 
second part will focus on implementing range flushes on the propose reverts.


Michael Cheng (1):
  i915/gem: drop wbinvd_on_all_cpus usage

 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH v2 1/1] i915/gem: drop wbinvd_on_all_cpus usage

[Michael Cheng]

Previous concern with using drm_clflush_sg was that we don't know what the
sg_table is pointing to, thus the usage of wbinvd_on_all_cpus to flush
everything at once to avoid paranoia.

To make i915 more architecture-neutral and be less paranoid, lets attempt to
use drm_clflush_sg to flush the pages for when the GPU wants to read
from main memory.

Signed-off-by: Michael Cheng <michael.cheng@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index f5062d0c6333..b0a5baaebc43 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -8,6 +8,7 @@
 #include <linux/highmem.h>
 #include <linux/dma-resv.h>
 #include <linux/module.h>
+#include <drm/drm_cache.h>
 
 #include <asm/smp.h>
 
@@ -250,16 +251,10 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
 	 * DG1 is special here since it still snoops transactions even with
 	 * CACHE_NONE. This is not the case with other HAS_SNOOP platforms. We
 	 * might need to revisit this as we add new discrete platforms.
-	 *
-	 * XXX: Consider doing a vmap flush or something, where possible.
-	 * Currently we just do a heavy handed wbinvd_on_all_cpus() here since
-	 * the underlying sg_table might not even point to struct pages, so we
-	 * can't just call drm_clflush_sg or similar, like we do elsewhere in
-	 * the driver.
 	 */
 	if (i915_gem_object_can_bypass_llc(obj) ||
 	    (!HAS_LLC(i915) && !IS_DG1(i915)))
-		wbinvd_on_all_cpus();
+		drm_clflush_sg(pages);
 
 	sg_page_sizes = i915_sg_dma_sizes(pages->sgl);
 	__i915_gem_object_set_pages(obj, pages, sg_page_sizes);
-- 
2.25.1

Re: [Intel-gfx] [PATCH v2 1/1] i915/gem: drop wbinvd_on_all_cpus usage

[Lucas De Marchi]

On Thu, Apr 14, 2022 at 11:19:23AM -0700, Michael Cheng wrote:
>Previous concern with using drm_clflush_sg was that we don't know what the
>sg_table is pointing to, thus the usage of wbinvd_on_all_cpus to flush
>everything at once to avoid paranoia.

humn... and now we know it is backed by struct pages? I'm not sure I
follow what we didn't know before and now we do.

Thomas / Matthew, could you take another look herer if it seems correct
to you.


thanks
Lucas De Marchi


>To make i915 more architecture-neutral and be less paranoid, lets attempt to
>use drm_clflush_sg to flush the pages for when the GPU wants to read
>from main memory.
>
>Signed-off-by: Michael Cheng <michael.cheng@intel.com>
>---
> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>index f5062d0c6333..b0a5baaebc43 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>@@ -8,6 +8,7 @@
> #include <linux/highmem.h>
> #include <linux/dma-resv.h>
> #include <linux/module.h>
>+#include <drm/drm_cache.h>
>
> #include <asm/smp.h>
>
>@@ -250,16 +251,10 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
> 	 * DG1 is special here since it still snoops transactions even with
> 	 * CACHE_NONE. This is not the case with other HAS_SNOOP platforms. We
> 	 * might need to revisit this as we add new discrete platforms.
>-	 *
>-	 * XXX: Consider doing a vmap flush or something, where possible.
>-	 * Currently we just do a heavy handed wbinvd_on_all_cpus() here since
>-	 * the underlying sg_table might not even point to struct pages, so we
>-	 * can't just call drm_clflush_sg or similar, like we do elsewhere in
>-	 * the driver.
> 	 */
> 	if (i915_gem_object_can_bypass_llc(obj) ||
> 	    (!HAS_LLC(i915) && !IS_DG1(i915)))
>-		wbinvd_on_all_cpus();
>+		drm_clflush_sg(pages);
>
> 	sg_page_sizes = i915_sg_dma_sizes(pages->sgl);
> 	__i915_gem_object_set_pages(obj, pages, sg_page_sizes);
>-- 
>2.25.1
>

Re: [Intel-gfx] [PATCH v2 1/1] i915/gem: drop wbinvd_on_all_cpus usage

[Thomas Hellström]

Hi,

On Fri, 2022-06-17 at 16:30 -0700, Lucas De Marchi wrote:
> On Thu, Apr 14, 2022 at 11:19:23AM -0700, Michael Cheng wrote:
> > Previous concern with using drm_clflush_sg was that we don't know
> > what the
> > sg_table is pointing to, thus the usage of wbinvd_on_all_cpus to
> > flush
> > everything at once to avoid paranoia.
> 
> humn... and now we know it is backed by struct pages? I'm not sure I
> follow what we didn't know before and now we do.
> 
> Thomas / Matthew, could you take another look herer if it seems
> correct
> to you.
> 
> 
No, this isn't correct. A dma-buf sg-table may not be backed by struct
pages, and AFAIK, there is no way for the importer to tell, whether
that's the case or not.

If we need to get rid of the wbinvd_on_all_cpus here, we need to use
the dma_buf_vmap() function to obtain a virtual address and then use
drm_clflush_virt_range() on that address. After that clflush, we can
call dma_buf_vunmap(). This should work since x86 uses PIPT caches and
a flush to a virtual range should flush all other virtual ranges
pointing to the same pages as well.

/Thomas


> thanks
> Lucas De Marchi
> 
> 
> > To make i915 more architecture-neutral and be less paranoid, lets
> > attempt to
> > use drm_clflush_sg to flush the pages for when the GPU wants to
> > read
> > from main memory.
> > 
> > Signed-off-by: Michael Cheng <michael.cheng@intel.com>
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++-------
> > 1 file changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> > index f5062d0c6333..b0a5baaebc43 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> > @@ -8,6 +8,7 @@
> > #include <linux/highmem.h>
> > #include <linux/dma-resv.h>
> > #include <linux/module.h>
> > +#include <drm/drm_cache.h>
> > 
> > #include <asm/smp.h>
> > 
> > @@ -250,16 +251,10 @@ static int
> > i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
> >          * DG1 is special here since it still snoops transactions
> > even with
> >          * CACHE_NONE. This is not the case with other HAS_SNOOP
> > platforms. We
> >          * might need to revisit this as we add new discrete
> > platforms.
> > -        *
> > -        * XXX: Consider doing a vmap flush or something, where
> > possible.
> > -        * Currently we just do a heavy handed wbinvd_on_all_cpus()
> > here since
> > -        * the underlying sg_table might not even point to struct
> > pages, so we
> > -        * can't just call drm_clflush_sg or similar, like we do
> > elsewhere in
> > -        * the driver.
> >          */
> >         if (i915_gem_object_can_bypass_llc(obj) ||
> >             (!HAS_LLC(i915) && !IS_DG1(i915)))
> > -               wbinvd_on_all_cpus();
> > +               drm_clflush_sg(pages);
> > 
> >         sg_page_sizes = i915_sg_dma_sizes(pages->sgl);
> >         __i915_gem_object_set_pages(obj, pages, sg_page_sizes);
> > -- 
> > 2.25.1
> >