Re: [PATCH 1/7] drm/i915: Don't use MI_STORE_DWORD_IMM on Sandybridge/vcs

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> MI_STORE_DWORD_IMM just doesn't work on the video decode engine under
> Sandybridge, so refrain from using it. Then switch the selftests over to
> using the now common test prior to using MI_STORE_DWORD_IMM.
>
> Fixes: 7dd4f6729f92 ("drm/i915: Async GPU relocation processing")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.13-rc1+
> ---
>  drivers/gpu/drm/i915/i915_drv.h                     |  7 +++++++
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c          |  8 ++++----
>  drivers/gpu/drm/i915/i915_selftest.h                |  2 --
>  drivers/gpu/drm/i915/intel_ringbuffer.h             | 12 ++++++++++++
>  drivers/gpu/drm/i915/selftests/i915_gem_coherency.c |  2 +-
>  drivers/gpu/drm/i915/selftests/i915_gem_context.c   |  6 ++++--
>  drivers/gpu/drm/i915/selftests/intel_hangcheck.c    | 18 ++++++++++++------
>  7 files changed, 40 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6c25c8520c87..620e53bd705a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -4327,4 +4327,11 @@ int remap_io_mapping(struct vm_area_struct *vma,
>  		     unsigned long addr, unsigned long pfn, unsigned long size,
>  		     struct io_mapping *iomap);
>  
> +static inline bool
> +intel_engine_can_store_dword(struct intel_engine_cs *engine)
> +{
> +	return __intel_engine_can_store_dword(INTEL_GEN(engine->i915),
> +					      engine->class);
> +}
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 8e8bc7aefd9c..359d5dc6d8df 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1268,7 +1268,9 @@ relocate_entry(struct i915_vma *vma,
>  
>  	if (!eb->reloc_cache.vaddr &&
>  	    (DBG_FORCE_RELOC == FORCE_GPU_RELOC ||
> -	     !reservation_object_test_signaled_rcu(vma->resv, true))) {
> +	     !reservation_object_test_signaled_rcu(vma->resv, true)) &&
> +	    __intel_engine_can_store_dword(eb->reloc_cache.gen,
> +					   eb->engine->class)) {
>  		const unsigned int gen = eb->reloc_cache.gen;

If you lift this to upper scope, you can make the check little
shorter. But incase you are avoiding the assignment to the latest,
i am not insisting.

There is engine in the eb so could you elaborate that
do we get by not doig intel_engine_can_store_dword(eb->engine)?

-Mika

>  		unsigned int len;
>  		u32 *batch;
> @@ -1278,10 +1280,8 @@ relocate_entry(struct i915_vma *vma,
>  			len = offset & 7 ? 8 : 5;
>  		else if (gen >= 4)
>  			len = 4;
> -		else if (gen >= 3)
> +		else
>  			len = 3;
> -		else /* On gen2 MI_STORE_DWORD_IMM uses a physical address */
> -			goto repeat;
>  
>  		batch = reloc_gpu(eb, vma, len);
>  		if (IS_ERR(batch))
> diff --git a/drivers/gpu/drm/i915/i915_selftest.h b/drivers/gpu/drm/i915/i915_selftest.h
> index 9d7d86f1733d..78e1a1b168ff 100644
> --- a/drivers/gpu/drm/i915/i915_selftest.h
> +++ b/drivers/gpu/drm/i915/i915_selftest.h
> @@ -101,6 +101,4 @@ bool __igt_timeout(unsigned long timeout, const char *fmt, ...);
>  #define igt_timeout(t, fmt, ...) \
>  	__igt_timeout((t), KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
>  
> -#define igt_can_mi_store_dword_imm(D) (INTEL_GEN(D) > 2)
> -
>  #endif /* !__I915_SELFTEST_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index d33c93444c0d..02d8974bf9ab 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -735,4 +735,16 @@ bool intel_engines_are_idle(struct drm_i915_private *dev_priv);
>  void intel_engines_mark_idle(struct drm_i915_private *i915);
>  void intel_engines_reset_default_submission(struct drm_i915_private *i915);
>  
> +static inline bool
> +__intel_engine_can_store_dword(unsigned int gen, unsigned int class)
> +{
> +	if (gen <= 2)
> +		return false; /* uses physical not virtual addresses */
> +
> +	if (gen == 6 && class == VIDEO_DECODE_CLASS)
> +		return false; /* b0rked */
> +
> +	return true;
> +}
> +
>  #endif /* _INTEL_RINGBUFFER_H_ */
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c b/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c
> index 95d4aebc0181..35d778d70626 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_coherency.c
> @@ -241,7 +241,7 @@ static bool always_valid(struct drm_i915_private *i915)
>  
>  static bool needs_mi_store_dword(struct drm_i915_private *i915)
>  {
> -	return igt_can_mi_store_dword_imm(i915);
> +	return intel_engine_can_store_dword(i915->engine[RCS]);
>  }
>  
>  static const struct igt_coherency_mode {
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> index 12b85b3278cd..fb0a58fc8348 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> @@ -38,8 +38,6 @@ gpu_fill_dw(struct i915_vma *vma, u64 offset, unsigned long count, u32 value)
>  	u32 *cmd;
>  	int err;
>  
> -	GEM_BUG_ON(!igt_can_mi_store_dword_imm(vma->vm->i915));
> -
>  	size = (4 * count + 1) * sizeof(u32);
>  	size = round_up(size, PAGE_SIZE);
>  	obj = i915_gem_object_create_internal(vma->vm->i915, size);
> @@ -123,6 +121,7 @@ static int gpu_fill(struct drm_i915_gem_object *obj,
>  	int err;
>  
>  	GEM_BUG_ON(obj->base.size > vm->total);
> +	GEM_BUG_ON(!intel_engine_can_store_dword(engine));
>  
>  	vma = i915_vma_instance(obj, vm, NULL);
>  	if (IS_ERR(vma))
> @@ -359,6 +358,9 @@ static int igt_ctx_exec(void *arg)
>  		}
>  
>  		for_each_engine(engine, i915, id) {
> +			if (!intel_engine_can_store_dword(engine))
> +				continue;
> +
>  			if (!obj) {
>  				obj = create_test_object(ctx, file, &objects);
>  				if (IS_ERR(obj)) {
> diff --git a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> index 208b34e864fb..02e52a146ed8 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> @@ -253,9 +253,6 @@ static int igt_hang_sanitycheck(void *arg)
>  
>  	/* Basic check that we can execute our hanging batch */
>  
> -	if (!igt_can_mi_store_dword_imm(i915))
> -		return 0;
> -
>  	mutex_lock(&i915->drm.struct_mutex);
>  	err = hang_init(&h, i915);
>  	if (err)
> @@ -264,6 +261,9 @@ static int igt_hang_sanitycheck(void *arg)
>  	for_each_engine(engine, i915, id) {
>  		long timeout;
>  
> +		if (!intel_engine_can_store_dword(engine))
> +			continue;
> +
>  		rq = hang_create_request(&h, engine, i915->kernel_context);
>  		if (IS_ERR(rq)) {
>  			err = PTR_ERR(rq);
> @@ -599,6 +599,9 @@ static int igt_wait_reset(void *arg)
>  	long timeout;
>  	int err;
>  
> +	if (!intel_engine_can_store_dword(i915->engine[RCS]))
> +		return 0;
> +
>  	/* Check that we detect a stuck waiter and issue a reset */
>  
>  	global_reset_lock(i915);
> @@ -664,9 +667,6 @@ static int igt_reset_queue(void *arg)
>  
>  	/* Check that we replay pending requests following a hang */
>  
> -	if (!igt_can_mi_store_dword_imm(i915))
> -		return 0;
> -
>  	global_reset_lock(i915);
>  
>  	mutex_lock(&i915->drm.struct_mutex);
> @@ -679,6 +679,9 @@ static int igt_reset_queue(void *arg)
>  		IGT_TIMEOUT(end_time);
>  		unsigned int count;
>  
> +		if (!intel_engine_can_store_dword(engine))
> +			continue;
> +
>  		prev = hang_create_request(&h, engine, i915->kernel_context);
>  		if (IS_ERR(prev)) {
>  			err = PTR_ERR(prev);
> @@ -784,6 +787,9 @@ static int igt_handle_error(void *arg)
>  	if (!intel_has_reset_engine(i915))
>  		return 0;
>  
> +	if (!intel_engine_can_store_dword(i915->engine[RCS]))
> +		return 0;
> +
>  	mutex_lock(&i915->drm.struct_mutex);
>  
>  	err = hang_init(&h, i915);
> -- 
> 2.13.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx