Re: [Intel-gfx] [PATCH 05/13] drm/i915: Convert requests to use struct fence

2016-01-08 Thread Chris Wilson 
On Mon, Jan 04, 2016 at 01:16:54PM -0800, Jesse Barnes wrote:
> On 01/04/2016 12:57 PM, Chris Wilson wrote:
> > On Mon, Jan 04, 2016 at 09:20:44AM -0800, Jesse Barnes wrote:
> >> So this one has my ack.
> > 
> > This series makes a number of fundamental mistakes in seqno-interrupt
> > handling, so no.
> 
> Well unless you can enumerate the issues in enough detail for us to address 
> them, we don't have much choice but to go ahead.  I know you've replied to a 
> few of these threads in the past, but I don't see a current list of 
> outstanding bugs aside from the one about modifying input params on the 
> execbuf error path (though the code comment seems to indicate some care is 
> being taken there at least, so should be a small fix).

Other than the series addressing the reported bugs which this is direct
conflict with?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 05/13] drm/i915: Convert requests to use struct fence

2016-01-08 Thread Jesse Barnes 
On 01/08/2016 01:47 PM, Chris Wilson wrote:
> On Mon, Jan 04, 2016 at 01:16:54PM -0800, Jesse Barnes wrote:
>> On 01/04/2016 12:57 PM, Chris Wilson wrote:
>>> On Mon, Jan 04, 2016 at 09:20:44AM -0800, Jesse Barnes wrote:
 So this one has my ack.
>>>
>>> This series makes a number of fundamental mistakes in seqno-interrupt
>>> handling, so no.
>>
>> Well unless you can enumerate the issues in enough detail for us to address 
>> them, we don't have much choice but to go ahead.  I know you've replied to a 
>> few of these threads in the past, but I don't see a current list of 
>> outstanding bugs aside from the one about modifying input params on the 
>> execbuf error path (though the code comment seems to indicate some care is 
>> being taken there at least, so should be a small fix).
> 
> Other than the series addressing the reported bugs which this is direct
> conflict with?

Which patchset came first?  And yes, clearly enumerating the issues is
helpful regardless.  It doesn't really matter which came first though,
we've agreed to move forward with John's version since the scheduler has
been outstanding for so long, so your bug fixes will have to be rebased
on top of this work.  I hope that's acceptable, since I think we all
have the same ultimate goal here...

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

Re: [Intel-gfx] [PATCH 05/13] drm/i915: Convert requests to use struct fence

2016-01-04 Thread Jesse Barnes 
On 01/04/2016 12:57 PM, Chris Wilson wrote:
> On Mon, Jan 04, 2016 at 09:20:44AM -0800, Jesse Barnes wrote:
>> So this one has my ack.
> 
> This series makes a number of fundamental mistakes in seqno-interrupt
> handling, so no.

Well unless you can enumerate the issues in enough detail for us to address 
them, we don't have much choice but to go ahead.  I know you've replied to a 
few of these threads in the past, but I don't see a current list of outstanding 
bugs aside from the one about modifying input params on the execbuf error path 
(though the code comment seems to indicate some care is being taken there at 
least, so should be a small fix).

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

Re: [Intel-gfx] [PATCH 05/13] drm/i915: Convert requests to use struct fence

2016-01-04 Thread Chris Wilson 
On Mon, Jan 04, 2016 at 09:20:44AM -0800, Jesse Barnes wrote:
> So this one has my ack.

This series makes a number of fundamental mistakes in seqno-interrupt
handling, so no.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 05/13] drm/i915: Convert requests to use struct fence

2016-01-04 Thread Jesse Barnes 
On 12/17/2015 09:43 AM, Jesse Barnes wrote:
> On 12/11/2015 05:11 AM, john.c.harri...@intel.com wrote:
>> From: John Harrison 
>>
>> There is a construct in the linux kernel called 'struct fence' that is
>> intended to keep track of work that is executed on hardware. I.e. it
>> solves the basic problem that the drivers 'struct
>> drm_i915_gem_request' is trying to address. The request structure does
>> quite a lot more than simply track the execution progress so is very
>> definitely still required. However, the basic completion status side
>> could be updated to use the ready made fence implementation and gain
>> all the advantages that provides.
>>
>> This patch makes the first step of integrating a struct fence into the
>> request. It replaces the explicit reference count with that of the
>> fence. It also replaces the 'is completed' test with the fence's
>> equivalent. Currently, that simply chains on to the original request
>> implementation. A future patch will improve this.
>>
>> v3: Updated after review comments by Tvrtko Ursulin. Added fence
>> context/seqno pair to the debugfs request info. Renamed fence 'driver
>> name' to just 'i915'. Removed BUG_ONs.
>>
>> For: VIZ-5190
>> Signed-off-by: John Harrison 
>> Cc: Tvrtko Ursulin 
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c |  5 +--
>>  drivers/gpu/drm/i915/i915_drv.h | 45 +-
>>  drivers/gpu/drm/i915/i915_gem.c | 56 
>> ++---
>>  drivers/gpu/drm/i915/intel_lrc.c|  1 +
>>  drivers/gpu/drm/i915/intel_ringbuffer.c |  1 +
>>  drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
>>  6 files changed, 81 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 7415606..5b31186 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -709,11 +709,12 @@ static int i915_gem_request_info(struct seq_file *m, 
>> void *data)
>>  task = NULL;
>>  if (req->pid)
>>  task = pid_task(req->pid, PIDTYPE_PID);
>> -seq_printf(m, "%x @ %d: %s [%d]\n",
>> +seq_printf(m, "%x @ %d: %s [%d], fence = %u.%u\n",
>> req->seqno,
>> (int) (jiffies - req->emitted_jiffies),
>> task ? task->comm : "",
>> -   task ? task->pid : -1);
>> +   task ? task->pid : -1,
>> +   req->fence.context, req->fence.seqno);
>>  rcu_read_unlock();
>>  }
>>  
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 436149e..aa5cba7 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -51,6 +51,7 @@
>>  #include 
>>  #include 
>>  #include "intel_guc.h"
>> +#include 
>>  
>>  /* General customization:
>>   */
>> @@ -2174,7 +2175,17 @@ void i915_gem_track_fb(struct drm_i915_gem_object 
>> *old,
>>   * initial reference taken using kref_init
>>   */
>>  struct drm_i915_gem_request {
>> -struct kref ref;
>> +/**
>> + * Underlying object for implementing the signal/wait stuff.
>> + * NB: Never call fence_later() or return this fence object to user
>> + * land! Due to lazy allocation, scheduler re-ordering, pre-emption,
>> + * etc., there is no guarantee at all about the validity or
>> + * sequentiality of the fence's seqno! It is also unsafe to let
>> + * anything outside of the i915 driver get hold of the fence object
>> + * as the clean up when decrementing the reference count requires
>> + * holding the driver mutex lock.
>> + */
>> +struct fence fence;
>>  
>>  /** On Which ring this request was generated */
>>  struct drm_i915_private *i915;
>> @@ -2251,7 +2262,13 @@ int i915_gem_request_alloc(struct intel_engine_cs 
>> *ring,
>> struct intel_context *ctx,
>> struct drm_i915_gem_request **req_out);
>>  void i915_gem_request_cancel(struct drm_i915_gem_request *req);
>> -void i915_gem_request_free(struct kref *req_ref);
>> +
>> +static inline bool i915_gem_request_completed(struct drm_i915_gem_request 
>> *req,
>> +  bool lazy_coherency)
>> +{
>> +return fence_is_signaled(>fence);
>> +}
>> +
>>  int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
>> struct drm_file *file);
>>  
>> @@ -2271,7 +2288,7 @@ static inline struct drm_i915_gem_request *
>>  i915_gem_request_reference(struct drm_i915_gem_request *req)
>>  {
>>  if (req)
>> -kref_get(>ref);
>> +fence_get(>fence);
>>  return req;
>>  }
>>  
>> @@

Re: [Intel-gfx] [PATCH 05/13] drm/i915: Convert requests to use struct fence

2015-12-17 Thread Jesse Barnes 
On 12/11/2015 05:11 AM, john.c.harri...@intel.com wrote:
> From: John Harrison 
> 
> There is a construct in the linux kernel called 'struct fence' that is
> intended to keep track of work that is executed on hardware. I.e. it
> solves the basic problem that the drivers 'struct
> drm_i915_gem_request' is trying to address. The request structure does
> quite a lot more than simply track the execution progress so is very
> definitely still required. However, the basic completion status side
> could be updated to use the ready made fence implementation and gain
> all the advantages that provides.
> 
> This patch makes the first step of integrating a struct fence into the
> request. It replaces the explicit reference count with that of the
> fence. It also replaces the 'is completed' test with the fence's
> equivalent. Currently, that simply chains on to the original request
> implementation. A future patch will improve this.
> 
> v3: Updated after review comments by Tvrtko Ursulin. Added fence
> context/seqno pair to the debugfs request info. Renamed fence 'driver
> name' to just 'i915'. Removed BUG_ONs.
> 
> For: VIZ-5190
> Signed-off-by: John Harrison 
> Cc: Tvrtko Ursulin 
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  5 +--
>  drivers/gpu/drm/i915/i915_drv.h | 45 +-
>  drivers/gpu/drm/i915/i915_gem.c | 56 
> ++---
>  drivers/gpu/drm/i915/intel_lrc.c|  1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
>  6 files changed, 81 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7415606..5b31186 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -709,11 +709,12 @@ static int i915_gem_request_info(struct seq_file *m, 
> void *data)
>   task = NULL;
>   if (req->pid)
>   task = pid_task(req->pid, PIDTYPE_PID);
> - seq_printf(m, "%x @ %d: %s [%d]\n",
> + seq_printf(m, "%x @ %d: %s [%d], fence = %u.%u\n",
>  req->seqno,
>  (int) (jiffies - req->emitted_jiffies),
>  task ? task->comm : "",
> -task ? task->pid : -1);
> +task ? task->pid : -1,
> +req->fence.context, req->fence.seqno);
>   rcu_read_unlock();
>   }
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 436149e..aa5cba7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -51,6 +51,7 @@
>  #include 
>  #include 
>  #include "intel_guc.h"
> +#include 
>  
>  /* General customization:
>   */
> @@ -2174,7 +2175,17 @@ void i915_gem_track_fb(struct drm_i915_gem_object *old,
>   * initial reference taken using kref_init
>   */
>  struct drm_i915_gem_request {
> - struct kref ref;
> + /**
> +  * Underlying object for implementing the signal/wait stuff.
> +  * NB: Never call fence_later() or return this fence object to user
> +  * land! Due to lazy allocation, scheduler re-ordering, pre-emption,
> +  * etc., there is no guarantee at all about the validity or
> +  * sequentiality of the fence's seqno! It is also unsafe to let
> +  * anything outside of the i915 driver get hold of the fence object
> +  * as the clean up when decrementing the reference count requires
> +  * holding the driver mutex lock.
> +  */
> + struct fence fence;
>  
>   /** On Which ring this request was generated */
>   struct drm_i915_private *i915;
> @@ -2251,7 +2262,13 @@ int i915_gem_request_alloc(struct intel_engine_cs 
> *ring,
>  struct intel_context *ctx,
>  struct drm_i915_gem_request **req_out);
>  void i915_gem_request_cancel(struct drm_i915_gem_request *req);
> -void i915_gem_request_free(struct kref *req_ref);
> +
> +static inline bool i915_gem_request_completed(struct drm_i915_gem_request 
> *req,
> +   bool lazy_coherency)
> +{
> + return fence_is_signaled(>fence);
> +}
> +
>  int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
>  struct drm_file *file);
>  
> @@ -2271,7 +2288,7 @@ static inline struct drm_i915_gem_request *
>  i915_gem_request_reference(struct drm_i915_gem_request *req)
>  {
>   if (req)
> - kref_get(>ref);
> + fence_get(>fence);
>   return req;
>  }
>  
> @@ -2279,7 +2296,7 @@ static inline void
>  i915_gem_request_unreference(struct drm_i915_gem_request *req)
>  {
>

[Intel-gfx] [PATCH 05/13] drm/i915: Convert requests to use struct fence

2015-12-11 Thread John . C . Harrison 
From: John Harrison 

There is a construct in the linux kernel called 'struct fence' that is
intended to keep track of work that is executed on hardware. I.e. it
solves the basic problem that the drivers 'struct
drm_i915_gem_request' is trying to address. The request structure does
quite a lot more than simply track the execution progress so is very
definitely still required. However, the basic completion status side
could be updated to use the ready made fence implementation and gain
all the advantages that provides.

This patch makes the first step of integrating a struct fence into the
request. It replaces the explicit reference count with that of the
fence. It also replaces the 'is completed' test with the fence's
equivalent. Currently, that simply chains on to the original request
implementation. A future patch will improve this.

v3: Updated after review comments by Tvrtko Ursulin. Added fence
context/seqno pair to the debugfs request info. Renamed fence 'driver
name' to just 'i915'. Removed BUG_ONs.

For: VIZ-5190
Signed-off-by: John Harrison 
Cc: Tvrtko Ursulin 
---
 drivers/gpu/drm/i915/i915_debugfs.c |  5 +--
 drivers/gpu/drm/i915/i915_drv.h | 45 +-
 drivers/gpu/drm/i915/i915_gem.c | 56 ++---
 drivers/gpu/drm/i915/intel_lrc.c|  1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c |  1 +
 drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
 6 files changed, 81 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 7415606..5b31186 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -709,11 +709,12 @@ static int i915_gem_request_info(struct seq_file *m, void 
*data)
task = NULL;
if (req->pid)
task = pid_task(req->pid, PIDTYPE_PID);
-   seq_printf(m, "%x @ %d: %s [%d]\n",
+   seq_printf(m, "%x @ %d: %s [%d], fence = %u.%u\n",
   req->seqno,
   (int) (jiffies - req->emitted_jiffies),
   task ? task->comm : "",
-  task ? task->pid : -1);
+  task ? task->pid : -1,
+  req->fence.context, req->fence.seqno);
rcu_read_unlock();
}
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 436149e..aa5cba7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -51,6 +51,7 @@
 #include 
 #include 
 #include "intel_guc.h"
+#include 
 
 /* General customization:
  */
@@ -2174,7 +2175,17 @@ void i915_gem_track_fb(struct drm_i915_gem_object *old,
  * initial reference taken using kref_init
  */
 struct drm_i915_gem_request {
-   struct kref ref;
+   /**
+* Underlying object for implementing the signal/wait stuff.
+* NB: Never call fence_later() or return this fence object to user
+* land! Due to lazy allocation, scheduler re-ordering, pre-emption,
+* etc., there is no guarantee at all about the validity or
+* sequentiality of the fence's seqno! It is also unsafe to let
+* anything outside of the i915 driver get hold of the fence object
+* as the clean up when decrementing the reference count requires
+* holding the driver mutex lock.
+*/
+   struct fence fence;
 
/** On Which ring this request was generated */
struct drm_i915_private *i915;
@@ -2251,7 +2262,13 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring,
   struct intel_context *ctx,
   struct drm_i915_gem_request **req_out);
 void i915_gem_request_cancel(struct drm_i915_gem_request *req);
-void i915_gem_request_free(struct kref *req_ref);
+
+static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req,
+ bool lazy_coherency)
+{
+   return fence_is_signaled(>fence);
+}
+
 int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
   struct drm_file *file);
 
@@ -2271,7 +2288,7 @@ static inline struct drm_i915_gem_request *
 i915_gem_request_reference(struct drm_i915_gem_request *req)
 {
if (req)
-   kref_get(>ref);
+   fence_get(>fence);
return req;
 }
 
@@ -2279,7 +2296,7 @@ static inline void
 i915_gem_request_unreference(struct drm_i915_gem_request *req)
 {
WARN_ON(!mutex_is_locked(>ring->dev->struct_mutex));
-   kref_put(>ref, i915_gem_request_free);
+   fence_put(>fence);
 }
 
 static inline void
@@ -2291,7 +2308,7 @@ i915_gem_request_unreference__unlocked(struct