Re: [PATCH 5/9] drm/i915/execlists: Unify CSB access pointers

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 27/06/2018 22:07, Chris Wilson wrote:
> Following the removal of the last workarounds, the only CSB mmio access
> is for the old vGPU interface. The mmio registers presented by vGPU do
> not require forcewake and can be treated as ordinary volatile memory,
> i.e. they behave just like the HWSP access just at a different location.
> We can reduce the CSB access to a set of read/write/buffer pointers and
> treat the various paths identically and not worry about forcewake.
> (Forcewake is nightmare for worstcase latency, and we want to process
> this all with irqsoff -- no latency allowed!)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/intel_engine_cs.c  |  12 ---
>   drivers/gpu/drm/i915/intel_lrc.c        | 116 ++++++++++--------------
>   drivers/gpu/drm/i915/intel_ringbuffer.h |  23 +++--
>   3 files changed, 65 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index d3264bd6e9dc..7209c22798e6 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -25,7 +25,6 @@
>   #include <drm/drm_print.h>
>   
>   #include "i915_drv.h"
> -#include "i915_vgpu.h"
>   #include "intel_ringbuffer.h"
>   #include "intel_lrc.h"
>   
> @@ -456,21 +455,10 @@ static void intel_engine_init_batch_pool(struct intel_engine_cs *engine)
>   	i915_gem_batch_pool_init(&engine->batch_pool, engine);
>   }
>   
> -static bool csb_force_mmio(struct drm_i915_private *i915)
> -{
> -	/* Older GVT emulation depends upon intercepting CSB mmio */
> -	if (intel_vgpu_active(i915) && !intel_vgpu_has_hwsp_emulation(i915))
> -		return true;
> -
> -	return false;
> -}
> -
>   static void intel_engine_init_execlist(struct intel_engine_cs *engine)
>   {
>   	struct intel_engine_execlists * const execlists = &engine->execlists;
>   
> -	execlists->csb_use_mmio = csb_force_mmio(engine->i915);
> -
>   	execlists->port_mask = 1;
>   	BUILD_BUG_ON_NOT_POWER_OF_2(execlists_num_ports(execlists));
>   	GEM_BUG_ON(execlists_num_ports(execlists) > EXECLIST_MAX_PORTS);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 91656eb2f2db..368a8c74d11d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -137,6 +137,7 @@
>   #include <drm/i915_drm.h>
>   #include "i915_drv.h"
>   #include "i915_gem_render_state.h"
> +#include "i915_vgpu.h"
>   #include "intel_lrc_reg.h"
>   #include "intel_mocs.h"
>   #include "intel_workarounds.h"
> @@ -953,44 +954,23 @@ static void process_csb(struct intel_engine_cs *engine)
>   {
>   	struct intel_engine_execlists * const execlists = &engine->execlists;
>   	struct execlist_port *port = execlists->port;
> -	struct drm_i915_private *i915 = engine->i915;
> -
> -	/* The HWSP contains a (cacheable) mirror of the CSB */
> -	const u32 *buf =
> -		&engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
> -	unsigned int head, tail;
> -	bool fw = false;
> +	const u32 * const buf = execlists->csb_status;
> +	u8 head, tail;
>   
>   	/* Clear before reading to catch new interrupts */
>   	clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
>   	smp_mb__after_atomic();
>   
> -	if (unlikely(execlists->csb_use_mmio)) {
> -		intel_uncore_forcewake_get(i915, execlists->fw_domains);
> -		fw = true;
> -
> -		buf = (u32 * __force)
> -			(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_BUF_LO(engine, 0)));
> +	/* Note that csb_write, csb_status may be either in HWSP or mmio */
> +	head = execlists->csb_head;
> +	tail = READ_ONCE(*execlists->csb_write);

Under GVT when this is emulated mmio I think you need to mask and shift 
it with GEN8_CSB_WRITE_PTR.

> +	GEM_TRACE("%s cs-irq head=%d, tail=%d\n", engine->name, head, tail);
> +	if (unlikely(head == tail))
> +		return;
>   
> -		head = readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)));
> -		tail = GEN8_CSB_WRITE_PTR(head);
> -		head = GEN8_CSB_READ_PTR(head);
> -		execlists->csb_head = head;
> -	} else {
> -		const int write_idx =
> -			intel_hws_csb_write_index(i915) -
> -			I915_HWS_CSB_BUF0_INDEX;
> +	rmb(); /* Hopefully paired with a wmb() in HW */
>   
> -		head = execlists->csb_head;
> -		tail = READ_ONCE(buf[write_idx]);
> -		rmb(); /* Hopefully paired with a wmb() in HW */
> -	}
> -	GEM_TRACE("%s cs-irq head=%d [%d%s], tail=%d [%d%s]\n",
> -		  engine->name,
> -		  head, GEN8_CSB_READ_PTR(readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))), fw ? "" : "?",
> -		  tail, GEN8_CSB_WRITE_PTR(readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))), fw ? "" : "?");
> -
> -	while (head != tail) {
> +	do {

Why convert while to do-while? Early unlikely return above handles the 
void process_csb calls? Would the same effect happen if you put unlikely 
in the while (head != tail) condition and would be simpler?

>   		struct i915_request *rq;
>   		unsigned int status;
>   		unsigned int count;
> @@ -1016,12 +996,12 @@ static void process_csb(struct intel_engine_cs *engine)
>   		 * status notifier.
>   		 */
>   
> -		status = READ_ONCE(buf[2 * head]); /* maybe mmio! */
>   		GEM_TRACE("%s csb[%d]: status=0x%08x:0x%08x, active=0x%x\n",
>   			  engine->name, head,
> -			  status, buf[2*head + 1],
> +			  buf[2 * head + 0], buf[2 * head + 1],
>   			  execlists->active);
>   
> +		status = buf[2 * head];

Why not leave before GEM_TRACE above, as is, and then diff is smaller?

>   		if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
>   			      GEN8_CTX_STATUS_PREEMPTED))
>   			execlists_set_active(execlists,
> @@ -1103,16 +1083,11 @@ static void process_csb(struct intel_engine_cs *engine)
>   		} else {
>   			port_set(port, port_pack(rq, count));
>   		}
> -	}
> +	} while (head != tail);
>   
> -	if (head != execlists->csb_head) {
> -		execlists->csb_head = head;
> -		writel(_MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, head << 8),
> -		       i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)));
> -	}
> -
> -	if (unlikely(fw))
> -		intel_uncore_forcewake_put(i915, execlists->fw_domains);
> +	writel(_MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, head << 8),
> +	       execlists->csb_read);
> +	execlists->csb_head = head;

I guess early return helps to avoid this, but since it is going away in 
a following patch maybe it is not worth it.

>   }
>   
>   /*
> @@ -2430,28 +2405,11 @@ logical_ring_default_irqs(struct intel_engine_cs *engine)
>   static void
>   logical_ring_setup(struct intel_engine_cs *engine)
>   {
> -	struct drm_i915_private *dev_priv = engine->i915;
> -	enum forcewake_domains fw_domains;
> -
>   	intel_engine_setup_common(engine);
>   
>   	/* Intentionally left blank. */
>   	engine->buffer = NULL;
>   
> -	fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
> -						    RING_ELSP(engine),
> -						    FW_REG_WRITE);
> -
> -	fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
> -						     RING_CONTEXT_STATUS_PTR(engine),
> -						     FW_REG_READ | FW_REG_WRITE);
> -
> -	fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
> -						     RING_CONTEXT_STATUS_BUF_BASE(engine),
> -						     FW_REG_READ);
> -
> -	engine->execlists.fw_domains = fw_domains;
> -
>   	tasklet_init(&engine->execlists.tasklet,
>   		     execlists_submission_tasklet, (unsigned long)engine);
>   
> @@ -2459,34 +2417,56 @@ 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;
> +	struct intel_engine_execlists * const execlists = &engine->execlists;
>   	int ret;
>   
>   	ret = intel_engine_init_common(engine);
>   	if (ret)
>   		goto error;
>   
> -	if (HAS_LOGICAL_RING_ELSQ(engine->i915)) {
> -		engine->execlists.submit_reg = engine->i915->regs +
> +	if (HAS_LOGICAL_RING_ELSQ(i915)) {
> +		execlists->submit_reg = i915->regs +
>   			i915_mmio_reg_offset(RING_EXECLIST_SQ_CONTENTS(engine));
> -		engine->execlists.ctrl_reg = engine->i915->regs +
> +		execlists->ctrl_reg = i915->regs +
>   			i915_mmio_reg_offset(RING_EXECLIST_CONTROL(engine));
>   	} else {
> -		engine->execlists.submit_reg = engine->i915->regs +
> +		execlists->submit_reg = i915->regs +
>   			i915_mmio_reg_offset(RING_ELSP(engine));
>   	}
>   
> -	engine->execlists.preempt_complete_status = ~0u;
> -	if (engine->i915->preempt_context) {
> +	execlists->preempt_complete_status = ~0u;
> +	if (i915->preempt_context) {
>   		struct intel_context *ce =
> -			to_intel_context(engine->i915->preempt_context, engine);
> +			to_intel_context(i915->preempt_context, engine);
>   
> -		engine->execlists.preempt_complete_status =
> +		execlists->preempt_complete_status =
>   			upper_32_bits(ce->lrc_desc);
>   	}
>   
> -	engine->execlists.csb_head = GEN8_CSB_ENTRIES - 1;
> +	execlists->csb_head = GEN8_CSB_ENTRIES - 1;
> +	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_write = (u32 __force *)execlists->csb_read;
> +	} 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)];
> +	}
>   
>   	return 0;
>   
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index a0bc7a8222b4..5b92c5f03e1d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -300,19 +300,30 @@ struct intel_engine_execlists {
>   	struct rb_node *first;
>   
>   	/**
> -	 * @fw_domains: forcewake domains for irq tasklet
> +	 * @csb_head: context status buffer head
>   	 */
> -	unsigned int fw_domains;
> +	unsigned int csb_head;
>   
>   	/**
> -	 * @csb_head: context status buffer head
> +	 * @csb_read: control register for Context Switch buffer
> +	 *
> +	 * Note this register is always in mmio.
>   	 */
> -	unsigned int csb_head;
> +	u32 __iomem *csb_read;
>   
>   	/**
> -	 * @csb_use_mmio: access csb through mmio, instead of hwsp
> +	 * @csb_write: control register for Context Switch buffer
> +	 *
> +	 * Note this register may be either mmio or HWSP shadow.
> +	 */
> +	u32 *csb_write;
> +
> +	/**
> +	 * @csb_status: status array for Context Switch buffer
> +	 *
> +	 * Note these register may be either mmio or HWSP shadow.
>   	 */
> -	bool csb_use_mmio;
> +	u32 *csb_status;
>   
>   	/**
>   	 * @preempt_complete_status: expected CSB upon completing preemption
> 

Looks otherwise ok.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx