| Message ID | 1356755249-19810-3-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 28 Dec 2012 20:27:27 -0800, Ben Widawsky <ben@bwidawsk.net> wrote: > This refactor clearly identifies the GEN specific portion of the aliased > ppgtt code. Aside from some of the gen specific parts being extracted, > it also preps us for an upcoming patch that pulls out the size, which > has some nice benefits. Error handling looks a little wonky. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 51 ++++++++++++++++++++++--------------- > 1 file changed, 30 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 94d21aa..3b1bff1 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -108,29 +108,20 @@ static void i915_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt, > } > } > > -static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev) > +static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt) > { > + struct drm_device *dev = ppgtt->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - struct i915_hw_ppgtt *ppgtt; > unsigned first_pd_entry_in_global_pt; > - int i; > - int ret = -ENOMEM; > + int i, ret = -ENOMEM; > > /* ppgtt PDEs reside in the global gtt pagetable, which has 512*1024 > * entries. For aliasing ppgtt support we just steal them at the end for > * now. */ > first_pd_entry_in_global_pt = dev_priv->mm.gtt->gtt_total_entries - I915_PPGTT_PD_ENTRIES; > - > - ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL); > - if (!ppgtt) > - return ret; > - > - ppgtt->dev = dev; > ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES; > ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries, > GFP_KERNEL); > - if (!ppgtt->pt_pages) > - goto err_ppgtt; I think you still want this... > @@ -185,9 +171,32 @@ err_pt_alloc: > if (ppgtt->pt_pages[i]) > __free_page(ppgtt->pt_pages[i]); > } > - kfree(ppgtt->pt_pages); > -err_ppgtt: > - kfree(ppgtt); > + > + if (!ppgtt->pt_pages) > + kfree(ppgtt->pt_pages); And will want to reconsider this chunk. -Chris
On Sat, 29 Dec 2012 11:49:01 +0000 Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, 28 Dec 2012 20:27:27 -0800, Ben Widawsky <ben@bwidawsk.net> > wrote: > > This refactor clearly identifies the GEN specific portion of the > > aliased ppgtt code. Aside from some of the gen specific parts being > > extracted, it also preps us for an upcoming patch that pulls out > > the size, which has some nice benefits. > > Error handling looks a little wonky. I've not looked in much detail yet, but I'll assume you're right since I did this pretty quickly. So given that assumption, what do you think of the patches, useful, or waste of code complexity? > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > --- > > drivers/gpu/drm/i915/i915_gem_gtt.c | 51 > > ++++++++++++++++++++++--------------- 1 file changed, 30 > > insertions(+), 21 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > > b/drivers/gpu/drm/i915/i915_gem_gtt.c index 94d21aa..3b1bff1 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > @@ -108,29 +108,20 @@ static void i915_ppgtt_clear_range(struct > > i915_hw_ppgtt *ppgtt, } > > } > > > > -static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev) > > +static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt) > > { > > + struct drm_device *dev = ppgtt->dev; > > struct drm_i915_private *dev_priv = dev->dev_private; > > - struct i915_hw_ppgtt *ppgtt; > > unsigned first_pd_entry_in_global_pt; > > - int i; > > - int ret = -ENOMEM; > > + int i, ret = -ENOMEM; > > > > /* ppgtt PDEs reside in the global gtt pagetable, which > > has 512*1024 > > * entries. For aliasing ppgtt support we just steal them > > at the end for > > * now. */ > > first_pd_entry_in_global_pt = > > dev_priv->mm.gtt->gtt_total_entries - I915_PPGTT_PD_ENTRIES; - > > - ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL); > > - if (!ppgtt) > > - return ret; > > - > > - ppgtt->dev = dev; > > ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES; > > ppgtt->pt_pages = kzalloc(sizeof(struct page > > *)*ppgtt->num_pd_entries, GFP_KERNEL); > > - if (!ppgtt->pt_pages) > > - goto err_ppgtt; > > I think you still want this... > > > @@ -185,9 +171,32 @@ err_pt_alloc: > > if (ppgtt->pt_pages[i]) > > __free_page(ppgtt->pt_pages[i]); > > } > > - kfree(ppgtt->pt_pages); > > -err_ppgtt: > > - kfree(ppgtt); > > + > > + if (!ppgtt->pt_pages) > > + kfree(ppgtt->pt_pages); > > And will want to reconsider this chunk. > -Chris >
On Sat, 29 Dec 2012 11:45:09 -0800, Ben Widawsky <ben@bwidawsk.net> wrote: > On Sat, 29 Dec 2012 11:49:01 +0000 > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > On Fri, 28 Dec 2012 20:27:27 -0800, Ben Widawsky <ben@bwidawsk.net> > > wrote: > > > This refactor clearly identifies the GEN specific portion of the > > > aliased ppgtt code. Aside from some of the gen specific parts being > > > extracted, it also preps us for an upcoming patch that pulls out > > > the size, which has some nice benefits. > > > > Error handling looks a little wonky. > > I've not looked in much detail yet, but I'll assume you're right since > I did this pretty quickly. So given that assumption, what do you think > of the patches, useful, or waste of code complexity? The mention of transitioning ppgtt to use objects sounds intriguing, and without digging into the details, does seem a worthwhile design. This series looks fine as a mean of cutting the code up into manageable chunks. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 94d21aa..3b1bff1 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -108,29 +108,20 @@ static void i915_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt, } } -static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev) +static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt) { + struct drm_device *dev = ppgtt->dev; struct drm_i915_private *dev_priv = dev->dev_private; - struct i915_hw_ppgtt *ppgtt; unsigned first_pd_entry_in_global_pt; - int i; - int ret = -ENOMEM; + int i, ret = -ENOMEM; /* ppgtt PDEs reside in the global gtt pagetable, which has 512*1024 * entries. For aliasing ppgtt support we just steal them at the end for * now. */ first_pd_entry_in_global_pt = dev_priv->mm.gtt->gtt_total_entries - I915_PPGTT_PD_ENTRIES; - - ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL); - if (!ppgtt) - return ret; - - ppgtt->dev = dev; ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES; ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries, GFP_KERNEL); - if (!ppgtt->pt_pages) - goto err_ppgtt; for (i = 0; i < ppgtt->num_pd_entries; i++) { ppgtt->pt_pages[i] = alloc_page(GFP_KERNEL); @@ -152,8 +143,7 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev) 0, 4096, PCI_DMA_BIDIRECTIONAL); - if (pci_dma_mapping_error(dev->pdev, - pt_addr)) { + if (pci_dma_mapping_error(dev->pdev, pt_addr)) { ret = -EIO; goto err_pd_pin; @@ -162,15 +152,11 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev) } } - ppgtt->scratch_page_dma_addr = dev_priv->mm.gtt->scratch_page_dma; - i915_ppgtt_clear_range(ppgtt, 0, ppgtt->num_pd_entries*I915_PPGTT_PT_ENTRIES); ppgtt->pd_offset = (first_pd_entry_in_global_pt)*sizeof(gtt_pte_t); - dev_priv->mm.aliasing_ppgtt = ppgtt; - return 0; err_pd_pin: @@ -185,9 +171,32 @@ err_pt_alloc: if (ppgtt->pt_pages[i]) __free_page(ppgtt->pt_pages[i]); } - kfree(ppgtt->pt_pages); -err_ppgtt: - kfree(ppgtt); + + if (!ppgtt->pt_pages) + kfree(ppgtt->pt_pages); + + return ret; +} + + +static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct i915_hw_ppgtt *ppgtt; + int ret = -ENOMEM; + + ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL); + if (!ppgtt) + return ret; + + ppgtt->dev = dev; + ppgtt->scratch_page_dma_addr = dev_priv->mm.gtt->scratch_page_dma; + + ret = gen6_init_aliasing_ppgtt(ppgtt); + if (ret) + kfree(ppgtt); + else + dev_priv->mm.aliasing_ppgtt = ppgtt; return ret; }
This refactor clearly identifies the GEN specific portion of the aliased ppgtt code. Aside from some of the gen specific parts being extracted, it also preps us for an upcoming patch that pulls out the size, which has some nice benefits. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem_gtt.c | 51 ++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 21 deletions(-)