| Message ID | 1405098936-3992-2-git-send-email-armin.c.reese@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 11, 2014 at 10:15:36AM -0700, armin.c.reese@intel.com wrote: > From: Armin Reese <armin.c.reese@intel.com> > > Signed-off-by: Armin Reese <armin.c.reese@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index afd4eef..7e2190e 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -1624,18 +1624,17 @@ static void ggtt_unbind_vma(struct i915_vma *vma) > > void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj) > { > - struct drm_device *dev = obj->base.dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > - bool interruptible; > - > - interruptible = do_idling(dev_priv); > + if (!obj->has_dma_mapping) { > + struct drm_device *dev = obj->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + bool interruptible = do_idling(dev_priv); > > - if (!obj->has_dma_mapping) > dma_unmap_sg(&dev->pdev->dev, > obj->pages->sgl, obj->pages->nents, > PCI_DMA_BIDIRECTIONAL); > > - undo_idling(dev_priv, interruptible); > + undo_idling(dev_priv, interruptible); > + } Are you really beating the compiler here? -Chris
Originally, the function always executes do_idling() followed by undo_idling() regardless of whether obj->has_dma_mapping is TRUE or not. This change idles the GPU only if needed. However, if the intention of the function is to ALWAYS idle the GPU, then my change is a mistake. - Armin -----Original Message----- From: Chris Wilson [mailto:chris@chris-wilson.co.uk] Sent: Friday, July 11, 2014 10:20 AM To: Reese, Armin C Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915: Optimize the i915_gem_gtt_finish_object function On Fri, Jul 11, 2014 at 10:15:36AM -0700, armin.c.reese@intel.com wrote: > From: Armin Reese <armin.c.reese@intel.com> > > Signed-off-by: Armin Reese <armin.c.reese@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > b/drivers/gpu/drm/i915/i915_gem_gtt.c > index afd4eef..7e2190e 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -1624,18 +1624,17 @@ static void ggtt_unbind_vma(struct i915_vma > *vma) > > void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj) { > - struct drm_device *dev = obj->base.dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > - bool interruptible; > - > - interruptible = do_idling(dev_priv); > + if (!obj->has_dma_mapping) { > + struct drm_device *dev = obj->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + bool interruptible = do_idling(dev_priv); > > - if (!obj->has_dma_mapping) > dma_unmap_sg(&dev->pdev->dev, > obj->pages->sgl, obj->pages->nents, > PCI_DMA_BIDIRECTIONAL); > > - undo_idling(dev_priv, interruptible); > + undo_idling(dev_priv, interruptible); > + } Are you really beating the compiler here? -Chris -- Chris Wilson, Intel Open Source Technology Centre
On Fri, Jul 11, 2014 at 05:29:48PM +0000, Reese, Armin C wrote:
> Originally, the function always executes do_idling() followed by undo_idling() regardless of whether obj->has_dma_mapping is TRUE or not. This change idles the GPU only if needed.
But those functions will be inlined by the compiler, and if it is doing
its job corectly will see more opportunity for optimising the branching.
-Chris
On Fri, Jul 11, 2014 at 05:29:48PM +0000, Reese, Armin C wrote: > Originally, the function always executes do_idling() followed by undo_idling() regardless of whether obj->has_dma_mapping is TRUE or not. This change idles the GPU only if needed. > > However, if the intention of the function is to ALWAYS idle the GPU, then my change is a mistake. Ah yes. It is unclear whether the workaround also needs to be applied for stolen objects, but it is best to presume that it is. -Chris
So the code should stay the way it is originally and assume GPU idling is always needed? - Armin -----Original Message----- From: Chris Wilson [mailto:chris@chris-wilson.co.uk] Sent: Friday, July 11, 2014 10:37 AM To: Reese, Armin C Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915: Optimize the i915_gem_gtt_finish_object function On Fri, Jul 11, 2014 at 05:29:48PM +0000, Reese, Armin C wrote: > Originally, the function always executes do_idling() followed by undo_idling() regardless of whether obj->has_dma_mapping is TRUE or not. This change idles the GPU only if needed. > > However, if the intention of the function is to ALWAYS idle the GPU, then my change is a mistake. Ah yes. It is unclear whether the workaround also needs to be applied for stolen objects, but it is best to presume that it is. -Chris -- Chris Wilson, Intel Open Source Technology Centre
On Fri, Jul 11, 2014 at 05:46:01PM +0000, Reese, Armin C wrote:
> So the code should stay the way it is originally and assume GPU idling is always needed?
Yes, it also cover dmabuf imports as well. Definitely likely that we
need to keep the wa for those.
-Chris
OK, sounds like it's best to ignore this patch then. - Armin -----Original Message----- From: Chris Wilson [mailto:chris@chris-wilson.co.uk] Sent: Friday, July 11, 2014 10:49 AM To: Reese, Armin C Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915: Optimize the i915_gem_gtt_finish_object function On Fri, Jul 11, 2014 at 05:46:01PM +0000, Reese, Armin C wrote: > So the code should stay the way it is originally and assume GPU idling is always needed? Yes, it also cover dmabuf imports as well. Definitely likely that we need to keep the wa for those. -Chris -- Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index afd4eef..7e2190e 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1624,18 +1624,17 @@ static void ggtt_unbind_vma(struct i915_vma *vma) void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj) { - struct drm_device *dev = obj->base.dev; - struct drm_i915_private *dev_priv = dev->dev_private; - bool interruptible; - - interruptible = do_idling(dev_priv); + if (!obj->has_dma_mapping) { + struct drm_device *dev = obj->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + bool interruptible = do_idling(dev_priv); - if (!obj->has_dma_mapping) dma_unmap_sg(&dev->pdev->dev, obj->pages->sgl, obj->pages->nents, PCI_DMA_BIDIRECTIONAL); - undo_idling(dev_priv, interruptible); + undo_idling(dev_priv, interruptible); + } } static void i915_gtt_color_adjust(struct drm_mm_node *node,