Re: [PATCH v2] drm/i915/vgpu: Disallow loading on old vGPU hosts

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

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

> Since commit fd8526e50902 ("drm/i915/execlists: Trust the CSB") we
> actually broke the force-mmio mode for our execlists implementation. No
> one noticed, so ergo no one is actually using an old vGPU host (where we
> required the older method) and so can simply remove the broken support.
>
> v2: csb_read can go as well (Mika)
>
> Reported-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Fixes: fd8526e50902 ("drm/i915/execlists: Trust the CSB")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.c         | 14 +++++++++++
>  drivers/gpu/drm/i915/intel_lrc.c        | 32 +++++++------------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 16 -------------
>  3 files changed, 22 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e39016713464..3e5e2efce670 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1384,6 +1384,20 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
>  		}
>  	}
>  
> +	if (HAS_EXECLISTS(dev_priv)) {
> +		/*
> +		 * Older GVT emulation depends upon intercepting CSB mmio,
> +		 * which we no longer use, preferring to use the HWSP cache
> +		 * instead.
> +		 */
> +		if (intel_vgpu_active(dev_priv) &&
> +		    !intel_vgpu_has_hwsp_emulation(dev_priv)) {
> +			i915_report_error(dev_priv,
> +					  "old vGPU host found, support for HWSP emulation required\n");
> +			return -ENXIO;
> +		}
> +	}
> +
>  	intel_sanitize_options(dev_priv);
>  
>  	i915_perf_init(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 0a690c557113..bb5abd4f7516 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -748,6 +748,8 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
>  
>  static void reset_csb_pointers(struct intel_engine_execlists *execlists)
>  {
> +	const unsigned int reset_value = GEN8_CSB_ENTRIES - 1;
> +
>  	/*
>  	 * After a reset, the HW starts writing into CSB entry [0]. We
>  	 * therefore have to set our HEAD pointer back one entry so that
> @@ -757,8 +759,8 @@ static void reset_csb_pointers(struct intel_engine_execlists *execlists)
>  	 * inline comparison of our cached head position against the last HW
>  	 * write works even before the first interrupt.
>  	 */
> -	execlists->csb_head = execlists->csb_write_reset;
> -	WRITE_ONCE(*execlists->csb_write, execlists->csb_write_reset);
> +	execlists->csb_head = reset_value;
> +	WRITE_ONCE(*execlists->csb_write, reset_value);
>  }
>  
>  static void nop_submission_tasklet(unsigned long data)
> @@ -2213,12 +2215,6 @@ logical_ring_setup(struct intel_engine_cs *engine)
>  	logical_ring_default_irqs(engine);
>  }
>  
> -static bool csb_force_mmio(struct drm_i915_private *i915)
> -{
> -	/* Older GVT emulation depends upon intercepting CSB mmio */
> -	return intel_vgpu_active(i915) && !intel_vgpu_has_hwsp_emulation(i915);
> -}
> -
>  static int logical_ring_init(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *i915 = engine->i915;
> @@ -2248,24 +2244,12 @@ static int logical_ring_init(struct intel_engine_cs *engine)
>  			upper_32_bits(ce->lrc_desc);
>  	}
>  
> -	execlists->csb_read =
> -		i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine));
> -	if (csb_force_mmio(i915)) {
> -		execlists->csb_status = (u32 __force *)
> -			(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_BUF_LO(engine, 0)));
> +	execlists->csb_status =
> +		&engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
>  
> -		execlists->csb_write = (u32 __force *)execlists->csb_read;
> -		execlists->csb_write_reset =
> -			_MASKED_FIELD(GEN8_CSB_WRITE_PTR_MASK,
> -				      GEN8_CSB_ENTRIES - 1);
> -	} else {
> -		execlists->csb_status =
> -			&engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
> +	execlists->csb_write =
> +		&engine->status_page.page_addr[intel_hws_csb_write_index(i915)];
>  
> -		execlists->csb_write =
> -			&engine->status_page.page_addr[intel_hws_csb_write_index(i915)];
> -		execlists->csb_write_reset = GEN8_CSB_ENTRIES - 1;
> -	}
>  	reset_csb_pointers(execlists);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 970fb5c05c36..096043b784f0 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -312,13 +312,6 @@ struct intel_engine_execlists {
>  	 */
>  	struct rb_root_cached queue;
>  
> -	/**
> -	 * @csb_read: control register for Context Switch buffer
> -	 *
> -	 * Note this register is always in mmio.
> -	 */
> -	u32 __iomem *csb_read;
> -
>  	/**
>  	 * @csb_write: control register for Context Switch buffer
>  	 *
> @@ -338,15 +331,6 @@ struct intel_engine_execlists {
>  	 */
>  	u32 preempt_complete_status;
>  
> -	/**
> -	 * @csb_write_reset: reset value for CSB write pointer
> -	 *
> -	 * As the CSB write pointer maybe either in HWSP or as a field
> -	 * inside an mmio register, we want to reprogram it slightly
> -	 * differently to avoid later confusion.
> -	 */
> -	u32 csb_write_reset;
> -
>  	/**
>  	 * @csb_head: context status buffer head
>  	 */
> -- 
> 2.20.0.rc1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx