Re: [PATCH 1/3] drm/i915: Sanitycheck device iomem on probe

[Matthew Auld <matthew.auld@xxxxxxxxx>]

On 08/12/2021 10:20, Ramalingam C wrote:
> From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>
> As we setup the memory regions for the device, give each a quick test to
> verify that we can read and write to the full iomem range. This ensures
> that our physical addressing for the device's memory is correct, and
> some reassurance that the memory is functional.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> Cc: CQ Tang <cq.tang@xxxxxxxxx>
> Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>

For the series, assuming CI is happy now,
Reviewed-by: Matthew Auld <matthew.auld@xxxxxxxxx>

Also patch 3 should be moved to the start of the series.

> ---
>   drivers/gpu/drm/i915/intel_memory_region.c | 104 +++++++++++++++++++++
>   1 file changed, 104 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
> index b43121609e25..c53e07f1d0c0 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.c
> +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> @@ -3,6 +3,8 @@
>    * Copyright © 2019 Intel Corporation
>    */
>   
> +#include <linux/prandom.h>
> +
>   #include "intel_memory_region.h"
>   #include "i915_drv.h"
>   #include "i915_ttm_buddy_manager.h"
> @@ -29,6 +31,99 @@ static const struct {
>   	},
>   };
>   
> +static int __iopagetest(struct intel_memory_region *mem,
> +			u8 __iomem *va, int pagesize,
> +			u8 value, resource_size_t offset,
> +			const void *caller)
> +{
> +	int byte = prandom_u32_max(pagesize);
> +	u8 result[3];
> +
> +	memset_io(va, value, pagesize); /* or GPF! */
> +	wmb();
> +
> +	result[0] = ioread8(va);
> +	result[1] = ioread8(va + byte);
> +	result[2] = ioread8(va + pagesize - 1);
> +	if (memchr_inv(result, value, sizeof(result))) {
> +		dev_err(mem->i915->drm.dev,
> +			"Failed to read back from memory region:%pR at [%pa + %pa] for %ps; wrote %x, read (%x, %x, %x)\n",
> +			&mem->region, &mem->io_start, &offset, caller,
> +			value, result[0], result[1], result[2]);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int iopagetest(struct intel_memory_region *mem,
> +		      resource_size_t offset,
> +		      const void *caller)
> +{
> +	const u8 val[] = { 0x0, 0xa5, 0xc3, 0xf0 };
> +	void __iomem *va;
> +	int err;
> +	int i;
> +
> +	va = ioremap_wc(mem->io_start + offset, PAGE_SIZE);
> +	if (!va) {
> +		dev_err(mem->i915->drm.dev,
> +			"Failed to ioremap memory region [%pa + %px] for %ps\n",
> +			&mem->io_start, &offset, caller);
> +		return -EFAULT;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(val); i++) {
> +		err = __iopagetest(mem, va, PAGE_SIZE, val[i], offset, caller);
> +		if (err)
> +			break;
> +
> +		err = __iopagetest(mem, va, PAGE_SIZE, ~val[i], offset, caller);
> +		if (err)
> +			break;
> +	}
> +
> +	iounmap(va);
> +	return err;
> +}
> +
> +static resource_size_t random_page(resource_size_t last)
> +{
> +	/* Limited to low 44b (16TiB), but should suffice for a spot check */
> +	return prandom_u32_max(last >> PAGE_SHIFT) << PAGE_SHIFT;
> +}
> +
> +static int iomemtest(struct intel_memory_region *mem, const void *caller)
> +{
> +	resource_size_t last = resource_size(&mem->region) - PAGE_SIZE;
> +	int err;
> +
> +	/*
> +	 * Quick test to check read/write access to the iomap (backing store).
> +	 *
> +	 * Write a byte, read it back. If the iomapping fails, we expect
> +	 * a GPF preventing further execution. If the backing store does not
> +	 * exist, the read back will return garbage. We check a couple of pages,
> +	 * the first and last of the specified region to confirm the backing
> +	 * store + iomap does cover the entire memory region; and we check
> +	 * a random offset within as a quick spot check for bad memory.
> +	 */
> +
> +	err = iopagetest(mem, 0, caller);
> +	if (err)
> +		return err;
> +
> +	err = iopagetest(mem, last, caller);
> +	if (err)
> +		return err;
> +
> +	err = iopagetest(mem, random_page(last), caller);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
>   struct intel_memory_region *
>   intel_memory_region_lookup(struct drm_i915_private *i915,
>   			   u16 class, u16 instance)
> @@ -126,8 +221,17 @@ intel_memory_region_create(struct drm_i915_private *i915,
>   			goto err_free;
>   	}
>   
> +	if (io_start && IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) {
> +		err = iomemtest(mem, (void *)_RET_IP_);
> +		if (err)
> +			goto err_release;
> +	}
> +
>   	return mem;
>   
> +err_release:
> +	if (mem->ops->release)
> +		mem->ops->release(mem);
>   err_free:
>   	kfree(mem);
>   	return ERR_PTR(err);