[Intel-gfx] [PATCH 02/20] drm/i915: Force PD restore on dirty ppGTTs

[Mika Kuoppala]

"Barbalho, Rafael" <rafael.barbalho at intel.com> writes:

>> -----Original Message-----
>> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>> Sent: Thursday, May 21, 2015 4:08 PM
>> To: Mika Kuoppala
>> Cc: intel-gfx at lists.freedesktop.org; miku at iki.fi; Barbalho, Rafael
>> Subject: Re: [Intel-gfx] [PATCH 02/20] drm/i915: Force PD restore on dirty
>> ppGTTs
>> 
>> On Thu, May 21, 2015 at 05:37:30PM +0300, Mika Kuoppala wrote:
>> > Force page directory reload when ppgtt va->pa
>> > mapping has changed. Extend dirty rings mechanism
>> > for gen > 7 and use it to force pd restore in execlist
>> > mode when vm has been changed.
>> >
>> > Some parts of execlist context update cleanup based on
>> > work by Chris Wilson.
>> >
>> > v2: Add comment about lite restore (Chris)
>> >
>> > Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_lrc.c | 65 ++++++++++++++++++++-------------
>> -------
>> >  1 file changed, 33 insertions(+), 32 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
>> b/drivers/gpu/drm/i915/intel_lrc.c
>> > index 0413b8f..5ee2a8c 100644
>> > --- a/drivers/gpu/drm/i915/intel_lrc.c
>> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> > @@ -264,9 +264,10 @@ u32 intel_execlists_ctx_id(struct
>> drm_i915_gem_object *ctx_obj)
>> >  }
>> >
>> >  static uint64_t execlists_ctx_descriptor(struct intel_engine_cs *ring,
>> > -					 struct drm_i915_gem_object
>> *ctx_obj)
>> > +					 struct intel_context *ctx)
>> >  {
>> >  	struct drm_device *dev = ring->dev;
>> > +	struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
>> >  	uint64_t desc;
>> >  	uint64_t lrca = i915_gem_obj_ggtt_offset(ctx_obj);
>> >
>> > @@ -284,6 +285,14 @@ static uint64_t execlists_ctx_descriptor(struct
>> intel_engine_cs *ring,
>> >  	 * signalling between Command Streamers */
>> >  	/* desc |= GEN8_CTX_FORCE_RESTORE; */
>> >
>> > +	/* When performing a LiteRestore but with updated PD we need
>> > +	 * to force the GPU to reload the PD
>> > +	 */
>> > +	if (intel_ring_flag(ring) & ctx->ppgtt->pd_dirty_rings) {
>> > +		desc |= GEN8_CTX_FORCE_PD_RESTORE;
>> 
>> Wasn't there a hardware issue which basically meant you are not
>> allowed to actually set this bit?
>> 
>> Rafael had some details on that as far as I recall so adding cc...
>
> Ville is correct, there is a hardware issue in CHV with this bit and it should
> not be set. On BDW I am not sure, although you can stop the pre-fetching
> & caching in BDW by using 64-bit PPGTT addressing.
>
> So it's no from me I'm afraid.

Thanks for pointing out the details on this.

I worked around this with preallocating our top level PDP structure (4 pages
with 32bit mode) so that hardware sees immutable top level structure
per context.

Ville also noticed that initializing scratch structures by copying
is not good as it introduces unnecessary read. Better to just fill with
pte/pde entries always. This triggered a rebase on rest of the
series so I will resend whole series.

-Mika

>
>> 
>> > +		ctx->ppgtt->pd_dirty_rings &= ~intel_ring_flag(ring);
>> > +	}
>> > +
>
> <Snip>
>
>> > --
>> > 1.9.1
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> --
>> Ville Syrjälä
>> Intel OTC