<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 2022-03-21 4:07 a.m., Thomas Hellström wrote: </div> <blockquote type="cite" cite="mid:b986f2a7-c0d3-b145-dd45-54d25e10a8be@linux.intel.com"> On 3/21/22 11:30, Tvrtko Ursulin wrote: <blockquote type="cite"> On 19/03/2022 19:42, Michael Cheng wrote: <blockquote type="cite">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. </blockquote> And now we know, or we know it is not a concern? <blockquote type="cite">To make i915 more architecture-neutral and be less paranoid, lets attempt to </blockquote> "Lets attempt" as we don't know if this will work and/or what can/will break? <blockquote type="cite">use drm_clflush_sg to flush the pages for when the GPU wants to read from main memory. Signed-off-by: Michael Cheng <a class="moz-txt-link-rfc2396E" href="mailto:michael.cheng@intel.com"><michael.cheng@intel.com></a> --- 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); </blockquote> And as noticed before, drm_clfush_sg still can call wbinvd_on_all_cpus so are you just punting the issue somewhere else? How will it be solved there? </blockquote> I think in this case, drm_clflush_sg() can't be immediately used, because pages may not contain actual page pointers; might be just the dma address. It needs to be preceded with a dmabuf vmap. </blockquote> Could you elaborate more with using a dmabuf vmap? Doing a quick grep on drm_clflush_sg, were you thinking about something similar to the following? <div style="font-family: 'Liberation Mono', monospace; white-space: pre"><div> if (obj->cache_dirty) { </div><div> WARN_ON_ONCE(IS_DGFX(i915)); </div><div> obj->write_domain = 0; </div><div> if (i915_gem_object_has_struct_page(obj)) </div><div> drm_clflush_sg(pages); </div><div> obj->cache_dirty = false; </div><div> } </div></div> Thanks, Michael Cheng <blockquote type="cite" cite="mid:b986f2a7-c0d3-b145-dd45-54d25e10a8be@linux.intel.com">But otherwise this change, I figure, falls into the "prefer range-aware apis" category; If the CPU supports it, flush the range only, otherwise fall back to wbinvd(). /Thomas </blockquote> <blockquote type="cite" cite="mid:b986f2a7-c0d3-b145-dd45-54d25e10a8be@linux.intel.com"> <blockquote type="cite"> Regards, Tvrtko <blockquote type="cite"> sg_page_sizes = i915_sg_dma_sizes(pages->sgl); __i915_gem_object_set_pages(obj, pages, sg_page_sizes); </blockquote> </blockquote> </blockquote> </body> </html>