| Message ID | 1435677400-7630-4-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 6/30/2015 4:16 PM, Mika Kuoppala wrote: > Previously we have pointed the page where the individual ppgtt > scratch structures refer to, to be the instance which GGTT setup have > allocated. So it has been shared. > > To achive full isolation between ppgtts also in this regard, ^^^^^achieve > allocate per ppgtt scratch page. > Maybe also say that it moved scratch page/pt/pd operations together (genx_init/free_scratch functions). Daniel, since you requested this, should it get yours r-b? It looks ok to me. -Michel > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 94 +++++++++++++++++++++++++++++-------- > 1 file changed, 74 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 402d6d3..b1a8fc4 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -682,6 +682,42 @@ static void gen8_free_page_tables(struct drm_device *dev, > } > } > > +static int gen8_init_scratch(struct i915_address_space *vm) > +{ > + struct drm_device *dev = vm->dev; > + > + vm->scratch_page = alloc_scratch_page(dev); > + if (IS_ERR(vm->scratch_page)) > + return PTR_ERR(vm->scratch_page); > + > + vm->scratch_pt = alloc_pt(dev); > + if (IS_ERR(vm->scratch_pt)) { > + free_scratch_page(dev, vm->scratch_page); > + return PTR_ERR(vm->scratch_pt); > + } > + > + vm->scratch_pd = alloc_pd(dev); > + if (IS_ERR(vm->scratch_pd)) { > + free_pt(dev, vm->scratch_pt); > + free_scratch_page(dev, vm->scratch_page); > + return PTR_ERR(vm->scratch_pd); > + } > + > + gen8_initialize_pt(vm, vm->scratch_pt); > + gen8_initialize_pd(vm, vm->scratch_pd); > + > + return 0; > +} > + > +static void gen8_free_scratch(struct i915_address_space *vm) > +{ > + struct drm_device *dev = vm->dev; > + > + free_pd(dev, vm->scratch_pd); > + free_pt(dev, vm->scratch_pt); > + free_scratch_page(dev, vm->scratch_page); > +} > + > static void gen8_ppgtt_cleanup(struct i915_address_space *vm) > { > struct i915_hw_ppgtt *ppgtt = > @@ -697,8 +733,7 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm) > free_pd(ppgtt->base.dev, ppgtt->pdp.page_directory[i]); > } > > - free_pd(vm->dev, vm->scratch_pd); > - free_pt(vm->dev, vm->scratch_pt); > + gen8_free_scratch(vm); > } > > /** > @@ -985,16 +1020,11 @@ err_out: > */ > static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > { > - ppgtt->base.scratch_pt = alloc_pt(ppgtt->base.dev); > - if (IS_ERR(ppgtt->base.scratch_pt)) > - return PTR_ERR(ppgtt->base.scratch_pt); > - > - ppgtt->base.scratch_pd = alloc_pd(ppgtt->base.dev); > - if (IS_ERR(ppgtt->base.scratch_pd)) > - return PTR_ERR(ppgtt->base.scratch_pd); > + int ret; > > - gen8_initialize_pt(&ppgtt->base, ppgtt->base.scratch_pt); > - gen8_initialize_pd(&ppgtt->base, ppgtt->base.scratch_pd); > + ret = gen8_init_scratch(&ppgtt->base); > + if (ret) > + return ret; > > ppgtt->base.start = 0; > ppgtt->base.total = 1ULL << 32; > @@ -1410,6 +1440,33 @@ unwind_out: > return ret; > } > > +static int gen6_init_scratch(struct i915_address_space *vm) > +{ > + struct drm_device *dev = vm->dev; > + > + vm->scratch_page = alloc_scratch_page(dev); > + if (IS_ERR(vm->scratch_page)) > + return PTR_ERR(vm->scratch_page); > + > + vm->scratch_pt = alloc_pt(dev); > + if (IS_ERR(vm->scratch_pt)) { > + free_scratch_page(dev, vm->scratch_page); > + return PTR_ERR(vm->scratch_pt); > + } > + > + gen6_initialize_pt(vm, vm->scratch_pt); > + > + return 0; > +} > + > +static void gen6_free_scratch(struct i915_address_space *vm) > +{ > + struct drm_device *dev = vm->dev; > + > + free_pt(dev, vm->scratch_pt); > + free_scratch_page(dev, vm->scratch_page); > +} > + > static void gen6_ppgtt_cleanup(struct i915_address_space *vm) > { > struct i915_hw_ppgtt *ppgtt = > @@ -1424,11 +1481,12 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm) > free_pt(ppgtt->base.dev, pt); > } > > - free_pt(vm->dev, vm->scratch_pt); > + gen6_free_scratch(vm); > } > > static int gen6_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt) > { > + struct i915_address_space *vm = &ppgtt->base; > struct drm_device *dev = ppgtt->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > bool retried = false; > @@ -1439,11 +1497,10 @@ static int gen6_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt) > * size. We allocate at the top of the GTT to avoid fragmentation. > */ > BUG_ON(!drm_mm_initialized(&dev_priv->gtt.base.mm)); > - ppgtt->base.scratch_pt = alloc_pt(ppgtt->base.dev); > - if (IS_ERR(ppgtt->base.scratch_pt)) > - return PTR_ERR(ppgtt->base.scratch_pt); > > - gen6_initialize_pt(&ppgtt->base, ppgtt->base.scratch_pt); > + ret = gen6_init_scratch(vm); > + if (ret) > + return ret; > > alloc: > ret = drm_mm_insert_node_in_range_generic(&dev_priv->gtt.base.mm, > @@ -1474,7 +1531,7 @@ alloc: > return 0; > > err_out: > - free_pt(ppgtt->base.dev, ppgtt->base.scratch_pt); > + gen6_free_scratch(vm); > return ret; > } > > @@ -1548,10 +1605,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > > static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > - > ppgtt->base.dev = dev; > - ppgtt->base.scratch_page = dev_priv->gtt.base.scratch_page; > > if (INTEL_INFO(dev)->gen < 8) > return gen6_ppgtt_init(ppgtt); >
On 7/1/2015 3:26 PM, Daniel Vetter wrote: > On Wed, Jul 01, 2015 at 03:05:44PM +0100, Michel Thierry wrote: >> On 6/30/2015 4:16 PM, Mika Kuoppala wrote: >>> Previously we have pointed the page where the individual ppgtt >>> scratch structures refer to, to be the instance which GGTT setup have >>> allocated. So it has been shared. >>> >>> To achive full isolation between ppgtts also in this regard, >> ^^^^^achieve >> >>> allocate per ppgtt scratch page. >>> >> Maybe also say that it moved scratch page/pt/pd operations together >> (genx_init/free_scratch functions). >> >> Daniel, since you requested this, should it get yours r-b? >> It looks ok to me. > > Does that count as an r-b? Doing a detailed review is more work than just > acking the overall idea ;-) Yes, it'd be great if you fix the typo while merging. Reviewed-by: Michel Thierry <michel.thierry@intel.com> > -Daniel >
On Wed, Jul 01, 2015 at 03:05:44PM +0100, Michel Thierry wrote: > On 6/30/2015 4:16 PM, Mika Kuoppala wrote: > >Previously we have pointed the page where the individual ppgtt > >scratch structures refer to, to be the instance which GGTT setup have > >allocated. So it has been shared. > > > >To achive full isolation between ppgtts also in this regard, > ^^^^^achieve > > >allocate per ppgtt scratch page. > > > Maybe also say that it moved scratch page/pt/pd operations together > (genx_init/free_scratch functions). > > Daniel, since you requested this, should it get yours r-b? > It looks ok to me. Does that count as an r-b? Doing a detailed review is more work than just acking the overall idea ;-) -Daniel
On Wed, Jul 01, 2015 at 03:25:09PM +0100, Michel Thierry wrote: > On 7/1/2015 3:26 PM, Daniel Vetter wrote: > >On Wed, Jul 01, 2015 at 03:05:44PM +0100, Michel Thierry wrote: > >>On 6/30/2015 4:16 PM, Mika Kuoppala wrote: > >>>Previously we have pointed the page where the individual ppgtt > >>>scratch structures refer to, to be the instance which GGTT setup have > >>>allocated. So it has been shared. > >>> > >>>To achive full isolation between ppgtts also in this regard, > >> ^^^^^achieve > >> > >>>allocate per ppgtt scratch page. > >>> > >>Maybe also say that it moved scratch page/pt/pd operations together > >>(genx_init/free_scratch functions). > >> > >>Daniel, since you requested this, should it get yours r-b? > >>It looks ok to me. > > > >Does that count as an r-b? Doing a detailed review is more work than just > >acking the overall idea ;-) > > Yes, it'd be great if you fix the typo while merging. Done. > Reviewed-by: Michel Thierry <michel.thierry@intel.com> Queued for -next, thanks for the patch. -Daniel
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6688
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK 302/302 302/302
SNB 312/316 312/316
IVB 343/343 343/343
BYT -3 287/287 284/287
HSW 380/380 380/380
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*BYT igt@gem_partial_pwrite_pread@reads PASS(1) FAIL(1)
*BYT igt@gem_partial_pwrite_pread@reads-display PASS(1) FAIL(1)
*BYT igt@gem_partial_pwrite_pread@reads-uncached PASS(1) FAIL(1)
Note: You need to pay more attention to line start with '*'
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 402d6d3..b1a8fc4 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -682,6 +682,42 @@ static void gen8_free_page_tables(struct drm_device *dev, } } +static int gen8_init_scratch(struct i915_address_space *vm) +{ + struct drm_device *dev = vm->dev; + + vm->scratch_page = alloc_scratch_page(dev); + if (IS_ERR(vm->scratch_page)) + return PTR_ERR(vm->scratch_page); + + vm->scratch_pt = alloc_pt(dev); + if (IS_ERR(vm->scratch_pt)) { + free_scratch_page(dev, vm->scratch_page); + return PTR_ERR(vm->scratch_pt); + } + + vm->scratch_pd = alloc_pd(dev); + if (IS_ERR(vm->scratch_pd)) { + free_pt(dev, vm->scratch_pt); + free_scratch_page(dev, vm->scratch_page); + return PTR_ERR(vm->scratch_pd); + } + + gen8_initialize_pt(vm, vm->scratch_pt); + gen8_initialize_pd(vm, vm->scratch_pd); + + return 0; +} + +static void gen8_free_scratch(struct i915_address_space *vm) +{ + struct drm_device *dev = vm->dev; + + free_pd(dev, vm->scratch_pd); + free_pt(dev, vm->scratch_pt); + free_scratch_page(dev, vm->scratch_page); +} + static void gen8_ppgtt_cleanup(struct i915_address_space *vm) { struct i915_hw_ppgtt *ppgtt = @@ -697,8 +733,7 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm) free_pd(ppgtt->base.dev, ppgtt->pdp.page_directory[i]); } - free_pd(vm->dev, vm->scratch_pd); - free_pt(vm->dev, vm->scratch_pt); + gen8_free_scratch(vm); } /** @@ -985,16 +1020,11 @@ err_out: */ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt) { - ppgtt->base.scratch_pt = alloc_pt(ppgtt->base.dev); - if (IS_ERR(ppgtt->base.scratch_pt)) - return PTR_ERR(ppgtt->base.scratch_pt); - - ppgtt->base.scratch_pd = alloc_pd(ppgtt->base.dev); - if (IS_ERR(ppgtt->base.scratch_pd)) - return PTR_ERR(ppgtt->base.scratch_pd); + int ret; - gen8_initialize_pt(&ppgtt->base, ppgtt->base.scratch_pt); - gen8_initialize_pd(&ppgtt->base, ppgtt->base.scratch_pd); + ret = gen8_init_scratch(&ppgtt->base); + if (ret) + return ret; ppgtt->base.start = 0; ppgtt->base.total = 1ULL << 32; @@ -1410,6 +1440,33 @@ unwind_out: return ret; } +static int gen6_init_scratch(struct i915_address_space *vm) +{ + struct drm_device *dev = vm->dev; + + vm->scratch_page = alloc_scratch_page(dev); + if (IS_ERR(vm->scratch_page)) + return PTR_ERR(vm->scratch_page); + + vm->scratch_pt = alloc_pt(dev); + if (IS_ERR(vm->scratch_pt)) { + free_scratch_page(dev, vm->scratch_page); + return PTR_ERR(vm->scratch_pt); + } + + gen6_initialize_pt(vm, vm->scratch_pt); + + return 0; +} + +static void gen6_free_scratch(struct i915_address_space *vm) +{ + struct drm_device *dev = vm->dev; + + free_pt(dev, vm->scratch_pt); + free_scratch_page(dev, vm->scratch_page); +} + static void gen6_ppgtt_cleanup(struct i915_address_space *vm) { struct i915_hw_ppgtt *ppgtt = @@ -1424,11 +1481,12 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm) free_pt(ppgtt->base.dev, pt); } - free_pt(vm->dev, vm->scratch_pt); + gen6_free_scratch(vm); } static int gen6_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt) { + struct i915_address_space *vm = &ppgtt->base; struct drm_device *dev = ppgtt->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; bool retried = false; @@ -1439,11 +1497,10 @@ static int gen6_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt) * size. We allocate at the top of the GTT to avoid fragmentation. */ BUG_ON(!drm_mm_initialized(&dev_priv->gtt.base.mm)); - ppgtt->base.scratch_pt = alloc_pt(ppgtt->base.dev); - if (IS_ERR(ppgtt->base.scratch_pt)) - return PTR_ERR(ppgtt->base.scratch_pt); - gen6_initialize_pt(&ppgtt->base, ppgtt->base.scratch_pt); + ret = gen6_init_scratch(vm); + if (ret) + return ret; alloc: ret = drm_mm_insert_node_in_range_generic(&dev_priv->gtt.base.mm, @@ -1474,7 +1531,7 @@ alloc: return 0; err_out: - free_pt(ppgtt->base.dev, ppgtt->base.scratch_pt); + gen6_free_scratch(vm); return ret; } @@ -1548,10 +1605,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) { - struct drm_i915_private *dev_priv = dev->dev_private; - ppgtt->base.dev = dev; - ppgtt->base.scratch_page = dev_priv->gtt.base.scratch_page; if (INTEL_INFO(dev)->gen < 8) return gen6_ppgtt_init(ppgtt);
Previously we have pointed the page where the individual ppgtt scratch structures refer to, to be the instance which GGTT setup have allocated. So it has been shared. To achive full isolation between ppgtts also in this regard, allocate per ppgtt scratch page. Cc: Michel Thierry <michel.thierry@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/i915_gem_gtt.c | 94 +++++++++++++++++++++++++++++-------- 1 file changed, 74 insertions(+), 20 deletions(-)