Re: [Intel-gfx] [PATCH 14/53] drm/i915: Update pin_to_display_plane() to do explicit request management

2015-03-05 Thread Tomas Elf

On 19/02/2015 17:17, john.c.harri...@intel.com wrote:

From: John Harrison john.c.harri...@intel.com

Added explicit creation creation and submission of the request structure to the
display object pinning code. This removes any reliance on the OLR keeping track
of the request and the unknown randomness that can ensue with other work
becoming part of the same request.

v2: Added semaphore enabled check to prevent allocating a pointless request
structure in the case where the sync just calls wait_rendering().

For: VIZ-5115
Signed-off-by: John Harrison john.c.harri...@intel.com
---
  drivers/gpu/drm/i915/i915_gem.c |   21 ++---
  1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4c29177..5897d54 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3943,9 +3943,24 @@ i915_gem_object_pin_to_display_plane(struct 
drm_i915_gem_object *obj,
int ret;

if (pipelined != i915_gem_request_get_ring(obj-last_read_req)) {
-   ret = i915_gem_object_sync(obj, pipelined);
-   if (ret)
-   return ret;
+   if (!pipelined || !i915_semaphore_is_enabled(obj-base.dev)) {
+   ret = i915_gem_object_wait_rendering(obj, false);


... Also, the ret value here is never checked and returned. It's 
overwritten a few lines further down.


Thanks,
Tomas


+   } else {
+   struct drm_i915_private *dev_priv = 
pipelined-dev-dev_private;
+   struct drm_i915_gem_request *req;
+
+   ret = dev_priv-gt.alloc_request(pipelined, 
pipelined-default_context, req);
+   if (ret)
+   return ret;
+
+   ret = i915_gem_object_sync(obj, req-ring);
+   if (ret)
+   return ret;
+
+   ret = i915_add_request_no_flush(req-ring);
+   if (ret)
+   return ret;
+   }
}

/* Mark the pin_display early so that we account for the



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


Re: [Intel-gfx] [PATCH 14/53] drm/i915: Update pin_to_display_plane() to do explicit request management

2015-03-05 Thread Tomas Elf

On 19/02/2015 17:17, john.c.harri...@intel.com wrote:

From: John Harrison john.c.harri...@intel.com

Added explicit creation creation and submission of the request structure to the


Nitpick: creation creation


display object pinning code. This removes any reliance on the OLR keeping track
of the request and the unknown randomness that can ensue with other work
becoming part of the same request.

v2: Added semaphore enabled check to prevent allocating a pointless request
structure in the case where the sync just calls wait_rendering().

For: VIZ-5115
Signed-off-by: John Harrison john.c.harri...@intel.com
---
  drivers/gpu/drm/i915/i915_gem.c |   21 ++---
  1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4c29177..5897d54 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3943,9 +3943,24 @@ i915_gem_object_pin_to_display_plane(struct 
drm_i915_gem_object *obj,
int ret;

if (pipelined != i915_gem_request_get_ring(obj-last_read_req)) {
-   ret = i915_gem_object_sync(obj, pipelined);
-   if (ret)
-   return ret;
+   if (!pipelined || !i915_semaphore_is_enabled(obj-base.dev)) {
+   ret = i915_gem_object_wait_rendering(obj, false);
+   } else {


The call to i915_gem_object_wait_rendering() was taken from the 
implementation of i915_gem_object_sync() below. Ripping out code from 
the function you're calling just to do it in advance is not very nice. 
Just imagine a scenario where the i915_gem_object_sync() implementation 
were to change but the code outside the function would not. We need to 
figure out a better way of doing this. In fact, allocating and managing 
a request that never gets to be used inside the i915_gem_object_sync() 
function could be a better way to go since it poses less potential 
future problem even though it's more wasteful. But there are probably 
better ways of doing this.


Thanks,
Tomas


+   struct drm_i915_private *dev_priv = 
pipelined-dev-dev_private;
+   struct drm_i915_gem_request *req;
+
+   ret = dev_priv-gt.alloc_request(pipelined, 
pipelined-default_context, req);
+   if (ret)
+   return ret;
+
+   ret = i915_gem_object_sync(obj, req-ring);
+   if (ret)
+   return ret;
+
+   ret = i915_add_request_no_flush(req-ring);
+   if (ret)
+   return ret;
+   }
}

/* Mark the pin_display early so that we account for the


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


[Intel-gfx] [PATCH 14/53] drm/i915: Update pin_to_display_plane() to do explicit request management

2015-02-19 Thread John . C . Harrison
From: John Harrison john.c.harri...@intel.com

Added explicit creation creation and submission of the request structure to the
display object pinning code. This removes any reliance on the OLR keeping track
of the request and the unknown randomness that can ensue with other work
becoming part of the same request.

v2: Added semaphore enabled check to prevent allocating a pointless request
structure in the case where the sync just calls wait_rendering().

For: VIZ-5115
Signed-off-by: John Harrison john.c.harri...@intel.com
---
 drivers/gpu/drm/i915/i915_gem.c |   21 ++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4c29177..5897d54 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3943,9 +3943,24 @@ i915_gem_object_pin_to_display_plane(struct 
drm_i915_gem_object *obj,
int ret;
 
if (pipelined != i915_gem_request_get_ring(obj-last_read_req)) {
-   ret = i915_gem_object_sync(obj, pipelined);
-   if (ret)
-   return ret;
+   if (!pipelined || !i915_semaphore_is_enabled(obj-base.dev)) {
+   ret = i915_gem_object_wait_rendering(obj, false);
+   } else {
+   struct drm_i915_private *dev_priv = 
pipelined-dev-dev_private;
+   struct drm_i915_gem_request *req;
+
+   ret = dev_priv-gt.alloc_request(pipelined, 
pipelined-default_context, req);
+   if (ret)
+   return ret;
+
+   ret = i915_gem_object_sync(obj, req-ring);
+   if (ret)
+   return ret;
+
+   ret = i915_add_request_no_flush(req-ring);
+   if (ret)
+   return ret;
+   }
}
 
/* Mark the pin_display early so that we account for the
-- 
1.7.9.5

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