| Message ID | 1486364695-8727-1-git-send-email-zhi.a.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 06, 2017 at 03:04:55PM +0800, Zhi Wang wrote: > Aliasing PPGTT mode is broken due to recent changes. Mostly boot the > system with i915.enable_ppgtt=1 will lead a kernel crash. > > This patch fixes this problem by: > > - PPGTT page table will not be shrinkable if working under aliasing > PPGTT mode. Not that keen on adding yet another special case into the code. In the patches I sent, the idea was to keep the PD structure from the aliasing_ppgtt preallocation by keeping the used_pte count positive (and so keeping the entire tree marked as in-use). > - Load the root pointers of the PPGTT page table during the context > initialization, as currently the "LRI PDPs updating" magic only works > under full PPGTT mode and also GVT-g doesn't support LRI PDP updating. That is incorrect, they are loaded into the context image before submission. If you want to make sure they are unchanging so that lite-restore under g-GVT works, that is another matter. -Chris
Hi Chris:
Thanks for the reply! :P Have you also fixed here with your new ideas?
in intel_lrc:
static u64 execlists_update_context(struct drm_i915_gem_request *rq)
{
struct intel_context *ce = &rq->ctx->engine[rq->engine->id];
struct i915_hw_ppgtt *ppgtt = rq->ctx->ppgtt; // ----> check
if we should get ppgtt from dev_priv->mm.aliasing_ppgtt.
u32 *reg_state = ce->lrc_reg_state;
reg_state[CTX_RING_TAIL+1] = rq->tail;
On 02/06/17 16:10, Chris Wilson wrote:
>> - Load the root pointers of the PPGTT page table during the context
>> >initialization, as currently the "LRI PDPs updating" magic only works
>> >under full PPGTT mode and also GVT-g doesn't support LRI PDP updating.
> That is incorrect, they are loaded into the context image before
> submission. If you want to make sure they are unchanging so that
> lite-restore under g-GVT works, that is another matter.
> -Chris
On Mon, Feb 06, 2017 at 04:18:23PM +0800, Zhi Wang wrote: > Hi Chris: > Thanks for the reply! :P Have you also fixed here with your new ideas? > > in intel_lrc: > > static u64 execlists_update_context(struct drm_i915_gem_request *rq) > { > struct intel_context *ce = &rq->ctx->engine[rq->engine->id]; > struct i915_hw_ppgtt *ppgtt = rq->ctx->ppgtt; // ----> > check if we should get ppgtt from dev_priv->mm.aliasing_ppgtt. Oh dear. Yup, that's a bug. Can you send a ppgtt = rq->ctx->ppgtt ?: &rq->i915->mm.aliasing_ppgtt; One day we will bite the bullet and set ctx->ppgtt to be the aliasing_ppgtt (and maybe even fix it so that we can global_gtt as appropriate), and then just fix up the more unusual case in execbuf to bind using vm = global_gtt instead (if aliasing). -Chris
Yep. Wait a second. :P On 02/06/17 16:50, Chris Wilson wrote: > On Mon, Feb 06, 2017 at 04:18:23PM +0800, Zhi Wang wrote: >> Hi Chris: >> Thanks for the reply! :P Have you also fixed here with your new ideas? >> >> in intel_lrc: >> >> static u64 execlists_update_context(struct drm_i915_gem_request *rq) >> { >> struct intel_context *ce = &rq->ctx->engine[rq->engine->id]; >> struct i915_hw_ppgtt *ppgtt = rq->ctx->ppgtt; // ----> >> check if we should get ppgtt from dev_priv->mm.aliasing_ppgtt. > > Oh dear. Yup, that's a bug. > > Can you send a ppgtt = rq->ctx->ppgtt ?: &rq->i915->mm.aliasing_ppgtt; > > One day we will bite the bullet and set ctx->ppgtt to be the > aliasing_ppgtt (and maybe even fix it so that we can global_gtt as > appropriate), and then just fix up the more unusual case in execbuf to > bind using vm = global_gtt instead (if aliasing). > -Chris >
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 68ecfc1..4e06056 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -754,10 +754,19 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm, GEM_BUG_ON(pte_end > GEN8_PTES); - bitmap_clear(pt->used_ptes, pte, num_entries); + /* + * As there is only one PPGTT page table used to mirror the GGTT + * space in the system under aliasing PPGTT mode, we don't need + * to shrink it. Leave the PT pages "always used", so the upper + * level page table pages are safe during clear_range(). + * + */ + if (USES_FULL_PPGTT(vm->i915)) { + bitmap_clear(pt->used_ptes, pte, num_entries); - if (bitmap_empty(pt->used_ptes, GEN8_PTES)) - return true; + if (bitmap_empty(pt->used_ptes, GEN8_PTES)) + return true; + } pt_vaddr = kmap_px(pt); diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 44a92ea..9575562 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -2009,6 +2009,11 @@ static void execlists_init_reg_state(u32 *reg_state, * other PDP Descriptors are ignored. */ ASSIGN_CTX_PML4(ppgtt, reg_state); + } else { + ASSIGN_CTX_PDP(ppgtt, reg_state, 0); + ASSIGN_CTX_PDP(ppgtt, reg_state, 1); + ASSIGN_CTX_PDP(ppgtt, reg_state, 2); + ASSIGN_CTX_PDP(ppgtt, reg_state, 3); } if (engine->id == RCS) {
Aliasing PPGTT mode is broken due to recent changes. Mostly boot the system with i915.enable_ppgtt=1 will lead a kernel crash. This patch fixes this problem by: - PPGTT page table will not be shrinkable if working under aliasing PPGTT mode. - Load the root pointers of the PPGTT page table during the context initialization, as currently the "LRI PDPs updating" magic only works under full PPGTT mode and also GVT-g doesn't support LRI PDP updating. Tested on my SKL NUC box. Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Michal Winiarski <michal.winiarski@intel.com> Cc: Michel Thierry <michel.thierry@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Zhenyu Wang <zhenyuw@linux.intel.com> Cc: Zhiyuan Lv <zhiyuan.lv@intel.com> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com> --- drivers/gpu/drm/i915/i915_gem_gtt.c | 15 ++++++++++++--- drivers/gpu/drm/i915/intel_lrc.c | 5 +++++ 2 files changed, 17 insertions(+), 3 deletions(-)