Re: [PATCH 1/2] drm: Ask whether drm_gem_get_pages() should clear the CPU cache

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <chris@xxxxxxxxxxxxxxxxxx>

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 <linux/pagevec.h>
>  
>  #include <drm/drm.h>
> +#include <drm/drm_cache.h>
>  #include <drm/drm_device.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_file.h>
> @@ -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) >= 0x00100000UL));
>  	}
>  
> +	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(&etnaviv_obj->base);
> +	struct page **p = drm_gem_get_pages(&etnaviv_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(&gt->gem);
> +	pages = drm_gem_get_pages(&gt->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 npages = obj->size >> PAGE_SHIFT;
>  
>  		if (use_pages(obj))
> -			p = drm_gem_get_pages(obj);
> +			p = drm_gem_get_pages(obj, false);
>  		else
>  			p = get_pages_vram(obj, npages);
>  
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
> index d8e09792793a..1ff272c4be33 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -236,7 +236,7 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj)
>  	if (!(omap_obj->flags & OMAP_BO_MEM_SHMEM) || omap_obj->pages)
>  		return 0;
>  
> -	pages = drm_gem_get_pages(obj);
> +	pages = drm_gem_get_pages(obj, false);
>  	if (IS_ERR(pages)) {
>  		dev_err(obj->dev->dev, "could not get pages: %ld\n", PTR_ERR(pages));
>  		return PTR_ERR(pages);
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> index 7d5ebb10323b..003b76c3e623 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> @@ -80,7 +80,7 @@ static int rockchip_gem_get_pages(struct rockchip_gem_object *rk_obj)
>  	int ret, i;
>  	struct scatterlist *s;
>  
> -	rk_obj->pages = drm_gem_get_pages(&rk_obj->base);
> +	rk_obj->pages = drm_gem_get_pages(&rk_obj->base, false);
>  	if (IS_ERR(rk_obj->pages))
>  		return PTR_ERR(rk_obj->pages);
>  
> diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
> index 26af8daa9a16..f075f005ecbb 100644
> --- a/drivers/gpu/drm/tegra/gem.c
> +++ b/drivers/gpu/drm/tegra/gem.c
> @@ -289,7 +289,7 @@ static int tegra_bo_get_pages(struct drm_device *drm, struct tegra_bo *bo)
>  {
>  	int err;
>  
> -	bo->pages = drm_gem_get_pages(&bo->gem);
> +	bo->pages = drm_gem_get_pages(&bo->gem, false);
>  	if (IS_ERR(bo->pages))
>  		return PTR_ERR(bo->pages);
>  
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
> index fa54a6d1403d..f38dd590fa45 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.c
> +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> @@ -272,7 +272,7 @@ static struct page **vgem_pin_pages(struct drm_vgem_gem_object *bo)
>  	if (bo->pages_pin_count++ == 0) {
>  		struct page **pages;
>  
> -		pages = drm_gem_get_pages(&bo->base);
> +		pages = drm_gem_get_pages(&bo->base, false);
>  		if (IS_ERR(pages)) {
>  			bo->pages_pin_count--;
>  			mutex_unlock(&bo->pages_lock);
> diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
> index 19a0e260a4df..1d17b0ef56c7 100644
> --- a/drivers/gpu/drm/vkms/vkms_gem.c
> +++ b/drivers/gpu/drm/vkms/vkms_gem.c
> @@ -166,7 +166,7 @@ static struct page **_get_pages(struct vkms_gem_object *vkms_obj)
>  	struct drm_gem_object *gem_obj = &vkms_obj->gem;
>  
>  	if (!vkms_obj->pages) {
> -		struct page **pages = drm_gem_get_pages(gem_obj);
> +		struct page **pages = drm_gem_get_pages(gem_obj, false);
>  
>  		if (IS_ERR(pages))
>  			return pages;
> diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c b/drivers/gpu/drm/xen/xen_drm_front_gem.c
> index 4f34ef34ba60..520a15d75eb6 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
> @@ -132,7 +132,7 @@ static struct xen_gem_object *gem_create(struct drm_device *dev, size_t size)
>  	 * with the backend
>  	 */
>  	xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
> -	xen_obj->pages = drm_gem_get_pages(&xen_obj->base);
> +	xen_obj->pages = drm_gem_get_pages(&xen_obj->base, false);
>  	if (IS_ERR(xen_obj->pages)) {
>  		ret = PTR_ERR(xen_obj->pages);
>  		xen_obj->pages = NULL;
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index c38dd35da00b..118f13b1bb29 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -395,7 +395,7 @@ void drm_gem_free_mmap_offset(struct drm_gem_object *obj);
>  int drm_gem_create_mmap_offset(struct drm_gem_object *obj);
>  int drm_gem_create_mmap_offset_size(struct drm_gem_object *obj, size_t size);
>  
> -struct page **drm_gem_get_pages(struct drm_gem_object *obj);
> +struct page **drm_gem_get_pages(struct drm_gem_object *obj, bool clflush);
>  void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
>  		bool dirty, bool accessed);
>  
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux