Re: [PATCH 1/2] drm: Ask whether drm_gem_get_pages() should clear the CPU cache
On Mon, Oct 12, 2020 at 11:51:30AM +0100, Chris Wilson wrote:
> On some processors (such as arch/x86), accessing a page via a WC PAT is
> bypassed if the page is physically tagged in the CPU cache, and the
> access is serviced by the cache instead -- which leads to incoherency
> should the physical page itself be accessed using DMA. In order to
> prevent the false cache sharing of the physical pages, we need to
> explicitly flush the cache lines of those pages.
>
> Signed-off-by: Chris Wilson
Hm I'd leave this out for now. dma-api/cache flushing, and especially on
x86 is kinda a mess. I'd just land v1 of your patch meanwhile for vgem.
-Daniel
> ---
> drivers/gpu/drm/drm_gem.c | 8 ++--
> drivers/gpu/drm/drm_gem_shmem_helper.c | 8 +++-
> drivers/gpu/drm/etnaviv/etnaviv_gem.c | 2 +-
> drivers/gpu/drm/gma500/gtt.c| 2 +-
> drivers/gpu/drm/msm/msm_gem.c | 2 +-
> drivers/gpu/drm/omapdrm/omap_gem.c | 2 +-
> drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 2 +-
> drivers/gpu/drm/tegra/gem.c | 2 +-
> drivers/gpu/drm/vgem/vgem_drv.c | 2 +-
> drivers/gpu/drm/vkms/vkms_gem.c | 2 +-
> drivers/gpu/drm/xen/xen_drm_front_gem.c | 2 +-
> include/drm/drm_gem.h | 2 +-
> 12 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 1da67d34e55d..1948855d69e6 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -40,6 +40,7 @@
> #include
>
> #include
> +#include
> #include
> #include
> #include
> @@ -525,6 +526,7 @@ static void drm_gem_check_release_pagevec(struct pagevec
> *pvec)
> * drm_gem_get_pages - helper to allocate backing pages for a GEM object
> * from shmem
> * @obj: obj in question
> + * @clflush: whether to clear any CPU caches associated with the backing
> store
> *
> * This reads the page-array of the shmem-backing storage of the given gem
> * object. An array of pages is returned. If a page is not allocated or
> @@ -546,14 +548,13 @@ static void drm_gem_check_release_pagevec(struct
> pagevec *pvec)
> * drm_gem_object_init(), but not for those initialized with
> * drm_gem_private_object_init() only.
> */
> -struct page **drm_gem_get_pages(struct drm_gem_object *obj)
> +struct page **drm_gem_get_pages(struct drm_gem_object *obj, bool clflush)
> {
> struct address_space *mapping;
> struct page *p, **pages;
> struct pagevec pvec;
> int i, npages;
>
> -
> if (WARN_ON(!obj->filp))
> return ERR_PTR(-EINVAL);
>
> @@ -589,6 +590,9 @@ struct page **drm_gem_get_pages(struct drm_gem_object
> *obj)
> (page_to_pfn(p) >= 0x0010UL));
> }
>
> + if (clflush)
> + drm_clflush_pages(pages, npages);
> +
> return pages;
>
> fail:
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index fb11df7aced5..78a2eb77802b 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -152,7 +152,13 @@ static int drm_gem_shmem_get_pages_locked(struct
> drm_gem_shmem_object *shmem)
> if (shmem->pages_use_count++ > 0)
> return 0;
>
> - pages = drm_gem_get_pages(obj);
> + /*
> + * On some processors (such as arch/x86), accessing a page via a WC PAT
> + * is bypassed if the page is physically tagged in the CPU cache, and
> + * the access is serviced by the cache instead -- which leads to
> + * incoherency should the physical page itself be accessed using DMA.
> + */
> + pages = drm_gem_get_pages(obj, !shmem->map_cached);
> if (IS_ERR(pages)) {
> DRM_DEBUG_KMS("Failed to get pages (%ld)\n", PTR_ERR(pages));
> shmem->pages_use_count = 0;
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index 67d9a2b9ea6a..d8279ea363b3 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -58,7 +58,7 @@ static void etnaviv_gem_scatterlist_unmap(struct
> etnaviv_gem_object *etnaviv_obj
> static int etnaviv_gem_shmem_get_pages(struct etnaviv_gem_object
> *etnaviv_obj)
> {
> struct drm_device *dev = etnaviv_obj->base.dev;
> - struct page **p = drm_gem_get_pages(_obj->base);
> + struct page **p = drm_gem_get_pages(_obj->base, false);
>
> if (IS_ERR(p)) {
> dev_dbg(dev->dev, "could not get pages: %ld\n", PTR_ERR(p));
> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
> index 9278bcfad1bf..ada56aec7e68 100644
> --- a/drivers/gpu/drm/gma500/gtt.c
> +++ b/drivers/gpu/drm/gma500/gtt.c
> @@ -197,7 +197,7 @@ static int psb_gtt_attach_pages(struct gtt_range *gt)
>
> WARN_ON(gt->pages);
>
> - pages = drm_gem_get_pages(>gem);
> +
[PATCH 1/2] drm: Ask whether drm_gem_get_pages() should clear the CPU cache
On some processors (such as arch/x86), accessing a page via a WC PAT is
bypassed if the page is physically tagged in the CPU cache, and the
access is serviced by the cache instead -- which leads to incoherency
should the physical page itself be accessed using DMA. In order to
prevent the false cache sharing of the physical pages, we need to
explicitly flush the cache lines of those pages.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/drm_gem.c | 8 ++--
drivers/gpu/drm/drm_gem_shmem_helper.c | 8 +++-
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 2 +-
drivers/gpu/drm/gma500/gtt.c| 2 +-
drivers/gpu/drm/msm/msm_gem.c | 2 +-
drivers/gpu/drm/omapdrm/omap_gem.c | 2 +-
drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 2 +-
drivers/gpu/drm/tegra/gem.c | 2 +-
drivers/gpu/drm/vgem/vgem_drv.c | 2 +-
drivers/gpu/drm/vkms/vkms_gem.c | 2 +-
drivers/gpu/drm/xen/xen_drm_front_gem.c | 2 +-
include/drm/drm_gem.h | 2 +-
12 files changed, 23 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 1da67d34e55d..1948855d69e6 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -40,6 +40,7 @@
#include
#include
+#include
#include
#include
#include
@@ -525,6 +526,7 @@ static void drm_gem_check_release_pagevec(struct pagevec
*pvec)
* drm_gem_get_pages - helper to allocate backing pages for a GEM object
* from shmem
* @obj: obj in question
+ * @clflush: whether to clear any CPU caches associated with the backing store
*
* This reads the page-array of the shmem-backing storage of the given gem
* object. An array of pages is returned. If a page is not allocated or
@@ -546,14 +548,13 @@ static void drm_gem_check_release_pagevec(struct pagevec
*pvec)
* drm_gem_object_init(), but not for those initialized with
* drm_gem_private_object_init() only.
*/
-struct page **drm_gem_get_pages(struct drm_gem_object *obj)
+struct page **drm_gem_get_pages(struct drm_gem_object *obj, bool clflush)
{
struct address_space *mapping;
struct page *p, **pages;
struct pagevec pvec;
int i, npages;
-
if (WARN_ON(!obj->filp))
return ERR_PTR(-EINVAL);
@@ -589,6 +590,9 @@ struct page **drm_gem_get_pages(struct drm_gem_object *obj)
(page_to_pfn(p) >= 0x0010UL));
}
+ if (clflush)
+ drm_clflush_pages(pages, npages);
+
return pages;
fail:
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index fb11df7aced5..78a2eb77802b 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -152,7 +152,13 @@ static int drm_gem_shmem_get_pages_locked(struct
drm_gem_shmem_object *shmem)
if (shmem->pages_use_count++ > 0)
return 0;
- pages = drm_gem_get_pages(obj);
+ /*
+* On some processors (such as arch/x86), accessing a page via a WC PAT
+* is bypassed if the page is physically tagged in the CPU cache, and
+* the access is serviced by the cache instead -- which leads to
+* incoherency should the physical page itself be accessed using DMA.
+*/
+ pages = drm_gem_get_pages(obj, !shmem->map_cached);
if (IS_ERR(pages)) {
DRM_DEBUG_KMS("Failed to get pages (%ld)\n", PTR_ERR(pages));
shmem->pages_use_count = 0;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 67d9a2b9ea6a..d8279ea363b3 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -58,7 +58,7 @@ static void etnaviv_gem_scatterlist_unmap(struct
etnaviv_gem_object *etnaviv_obj
static int etnaviv_gem_shmem_get_pages(struct etnaviv_gem_object *etnaviv_obj)
{
struct drm_device *dev = etnaviv_obj->base.dev;
- struct page **p = drm_gem_get_pages(_obj->base);
+ struct page **p = drm_gem_get_pages(_obj->base, false);
if (IS_ERR(p)) {
dev_dbg(dev->dev, "could not get pages: %ld\n", PTR_ERR(p));
diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index 9278bcfad1bf..ada56aec7e68 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -197,7 +197,7 @@ static int psb_gtt_attach_pages(struct gtt_range *gt)
WARN_ON(gt->pages);
- pages = drm_gem_get_pages(>gem);
+ pages = drm_gem_get_pages(>gem, false);
if (IS_ERR(pages))
return PTR_ERR(pages);
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index c79828d31822..a7a67ef4e27e 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -102,7 +102,7 @@ static struct page **get_pages(struct drm_gem_object *obj)
int
