Re: [Intel-gfx] [PATCH] drm/i915: pin sprite fb only if it changed

2014-09-10 Thread Ville Syrjälä
On Tue, Sep 09, 2014 at 05:49:46PM -0300, Gustavo Padovan wrote:
 From: Gustavo Padovan gustavo.pado...@collabora.co.uk
 
 Optimize code avoiding helding dev mutex if old fb and current fb
 are the same.
 
 Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
 ---
  drivers/gpu/drm/i915/intel_sprite.c | 23 ++-
  1 file changed, 14 insertions(+), 9 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
 b/drivers/gpu/drm/i915/intel_sprite.c
 index a4306cf..a301838 100644
 --- a/drivers/gpu/drm/i915/intel_sprite.c
 +++ b/drivers/gpu/drm/i915/intel_sprite.c
 @@ -1038,21 +1038,23 @@ intel_commit_sprite_plane(struct drm_plane *plane,
   primary_enabled = !drm_rect_equals(dst, clip) || 
 colorkey_enabled(intel_plane);
   WARN_ON(!primary_enabled  !state-visible  intel_crtc-active);
  
 - mutex_lock(dev-struct_mutex);
  
   /* Note that this will apply the VT-d workaround for scanouts,
* which is more restrictive than required for sprites. (The
* primary plane requires 256KiB alignment with 64 PTE padding,
* the sprite planes only require 128KiB alignment and 32 PTE padding.
*/

The comment should probably remain just before the
intel_pin_and_fence_fb_obj() call.

 - ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 + if (old_obj != obj) {
 + mutex_lock(dev-struct_mutex);
 + ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
  
 - i915_gem_track_fb(old_obj, obj,
 -   INTEL_FRONTBUFFER_SPRITE(pipe));
 - mutex_unlock(dev-struct_mutex);
 + i915_gem_track_fb(old_obj, obj,
 +   INTEL_FRONTBUFFER_SPRITE(pipe));

I don't think we should be calling this when things failed. This bug was
already present in the code, but might as well fix it while you're at
it.

 + mutex_unlock(dev-struct_mutex);
  
 - if (ret)
 - return ret;
 + if (ret)
 + return ret;
 + }
  
   intel_plane-crtc_x = state-orig_dst.x1;
   intel_plane-crtc_y = state-orig_dst.y1;
 @@ -1098,15 +1100,18 @@ intel_commit_sprite_plane(struct drm_plane *plane,
   intel_post_enable_primary(crtc);
   }
  
 + if (!old_obj)
 + return 0;
 +
   /* Unpin old obj after new one is active to avoid ugliness */
 - if (old_obj) {
 + if (old_obj != obj) {

I'd prefer 
if (old_obj  old_obj != obj) {
in this case.

   /*
* It's fairly common to simply update the position of
* an existing object.  In that case, we don't need to
* wait for vblank to avoid ugliness, we only need to
* do the pin  ref bookkeeping.
*/
 - if (old_obj != obj  intel_crtc-active)
 + if (intel_crtc-active)
   intel_wait_for_vblank(dev, intel_crtc-pipe);
  
   mutex_lock(dev-struct_mutex);
 -- 
 1.9.3
 
 ___
 Intel-gfx mailing list
 Intel-gfx@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [PATCH] drm/i915: pin sprite fb only if it changed

2014-09-09 Thread Gustavo Padovan
From: Gustavo Padovan gustavo.pado...@collabora.co.uk

Optimize code avoiding helding dev mutex if old fb and current fb
are the same.

Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
---
 drivers/gpu/drm/i915/intel_sprite.c | 23 ++-
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index a4306cf..a301838 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1038,21 +1038,23 @@ intel_commit_sprite_plane(struct drm_plane *plane,
primary_enabled = !drm_rect_equals(dst, clip) || 
colorkey_enabled(intel_plane);
WARN_ON(!primary_enabled  !state-visible  intel_crtc-active);
 
-   mutex_lock(dev-struct_mutex);
 
/* Note that this will apply the VT-d workaround for scanouts,
 * which is more restrictive than required for sprites. (The
 * primary plane requires 256KiB alignment with 64 PTE padding,
 * the sprite planes only require 128KiB alignment and 32 PTE padding.
 */
-   ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+   if (old_obj != obj) {
+   mutex_lock(dev-struct_mutex);
+   ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 
-   i915_gem_track_fb(old_obj, obj,
- INTEL_FRONTBUFFER_SPRITE(pipe));
-   mutex_unlock(dev-struct_mutex);
+   i915_gem_track_fb(old_obj, obj,
+ INTEL_FRONTBUFFER_SPRITE(pipe));
+   mutex_unlock(dev-struct_mutex);
 
-   if (ret)
-   return ret;
+   if (ret)
+   return ret;
+   }
 
intel_plane-crtc_x = state-orig_dst.x1;
intel_plane-crtc_y = state-orig_dst.y1;
@@ -1098,15 +1100,18 @@ intel_commit_sprite_plane(struct drm_plane *plane,
intel_post_enable_primary(crtc);
}
 
+   if (!old_obj)
+   return 0;
+
/* Unpin old obj after new one is active to avoid ugliness */
-   if (old_obj) {
+   if (old_obj != obj) {
/*
 * It's fairly common to simply update the position of
 * an existing object.  In that case, we don't need to
 * wait for vblank to avoid ugliness, we only need to
 * do the pin  ref bookkeeping.
 */
-   if (old_obj != obj  intel_crtc-active)
+   if (intel_crtc-active)
intel_wait_for_vblank(dev, intel_crtc-pipe);
 
mutex_lock(dev-struct_mutex);
-- 
1.9.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx