[Intel-gfx] [PATCH 27/53] drm/i915/bdw: GEN-specific logical ring emit request

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Volkin, Bradley D
> Sent: Friday, June 20, 2014 10:18 PM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 27/53] drm/i915/bdw: GEN-specific logical
> ring emit request
> 
> On Fri, Jun 13, 2014 at 08:37:45AM -0700, oscar.mateo at intel.com wrote:
> > From: Oscar Mateo <oscar.mateo at intel.com>
> >
> > Very similar to the legacy add_request, only modified to account for
> > logical ringbuffer.
> >
> > Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h         |  1 +
> >  drivers/gpu/drm/i915/intel_lrc.c        | 61
> +++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_ringbuffer.h |  2 ++
> >  3 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 9c8692a..63ec3ea 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -267,6 +267,7 @@
> >  #define   MI_FORCE_RESTORE		(1<<1)
> >  #define   MI_RESTORE_INHIBIT		(1<<0)
> >  #define MI_STORE_DWORD_IMM	MI_INSTR(0x20, 1)
> > +#define MI_STORE_DWORD_IMM_GEN8	MI_INSTR(0x20, 2)
> >  #define   MI_MEM_VIRTUAL	(1 << 22) /* 965+ only */
> >  #define MI_STORE_DWORD_INDEX	MI_INSTR(0x21, 1)
> >  #define   MI_STORE_DWORD_INDEX_SHIFT 2
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > b/drivers/gpu/drm/i915/intel_lrc.c
> > index 89aed7a..3debe8b 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -359,6 +359,62 @@ static void gen8_submit_ctx(struct
> intel_engine_cs *ring,
> >  	DRM_ERROR("Execlists still not ready!\n");  }
> >
> > +static int gen8_emit_request(struct intel_engine_cs *ring,
> > +			     struct intel_context *ctx)
> > +{
> > +	struct intel_ringbuffer *ringbuf = logical_ringbuf_get(ring, ctx);
> > +	u32 cmd;
> > +	int ret;
> > +
> > +	ret = intel_logical_ring_begin(ring, ctx, 6);
> > +	if (ret)
> > +		return ret;
> > +
> > +	cmd = MI_FLUSH_DW + 1;
> > +	cmd |= MI_INVALIDATE_TLB;
> 
> Is the TLB invalidation truely required here? Otherwise it seems like we could
> use the same function for all rings, like on gen6+.

Hmmmmm... this is inherited from back when we only had the simulator, and its true meaning has been lost in the multiple rewrites:

drm/i915/bdw: Use MI_FLUSH_DW for requests
    
    The primary reason for doing this is MI_STORE_DWORD_IDX doesn't work in
    simulation. The simulator doesn't complain, it's just the seqno never
    gets pushed to memory. The theory is (and AFAICT this may be broken on
    existing platforms) we must issue an MI_FLUSH_DW after we emit the
    seqno, if we want to be able to read it back coherently.

I´ll rewrite it to use the same MI_STORE_DATA_IMM for every ring and then test it on read hardware.
Thanks for the heads up!

> > +	cmd |= MI_FLUSH_DW_OP_STOREDW;
> > +
> > +	intel_logical_ring_emit(ringbuf, cmd);
> > +	intel_logical_ring_emit(ringbuf,
> > +				(ring->status_page.gfx_addr +
> > +				(I915_GEM_HWS_INDEX <<
> MI_STORE_DWORD_INDEX_SHIFT)) |
> > +				MI_FLUSH_DW_USE_GTT);
> > +	intel_logical_ring_emit(ringbuf, 0);
> > +	intel_logical_ring_emit(ringbuf, ring->outstanding_lazy_seqno);
> > +	intel_logical_ring_emit(ringbuf, MI_USER_INTERRUPT);
> > +	intel_logical_ring_emit(ringbuf, MI_NOOP);
> > +	intel_logical_ring_advance_and_submit(ring, ctx);
> > +
> > +	return 0;
> > +}
> > +
> > +static int gen8_emit_request_render(struct intel_engine_cs *ring,
> > +				    struct intel_context *ctx)
> > +{
> > +	struct intel_ringbuffer *ringbuf = logical_ringbuf_get(ring, ctx);
> > +	u32 cmd;
> > +	int ret;
> > +
> > +	ret = intel_logical_ring_begin(ring, ctx, 6);
> > +	if (ret)
> > +		return ret;
> > +
> > +	cmd = MI_STORE_DWORD_IMM_GEN8;
> > +	cmd |= (1 << 22); /* use global GTT */
> 
> We could use MI_MEM_VIRTUAL or MI_GLOBAL_GTT instead.

Will do, thanks.

-- Oscar