Re: [Intel-gfx] [PATCH 07/12] drm/i915/gvt: Hold a reference on the request

2016-10-20 Thread Zhenyu Wang 
On 2016.10.20 07:52:18 +0100, Chris Wilson wrote:
> On Thu, Oct 20, 2016 at 08:22:00AM +0800, Zhenyu Wang wrote:
> > On 2016.10.19 11:11:42 +0100, Chris Wilson wrote:
> > > The workload took a pointer to the request, and even waited upon,
> > > without holding a reference on the request. Take that reference
> > > explicitly and fix up the error path following request allocation that
> > > missed flushing the request.
> > > 
> > > Signed-off-by: Chris Wilson 
> > > ---
> > >  drivers/gpu/drm/i915/gvt/scheduler.c | 24 +---
> > >  1 file changed, 13 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c 
> > > b/drivers/gpu/drm/i915/gvt/scheduler.c
> > > index b15cdf5978a9..224f19ae61ab 100644
> > > --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> > > +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> > > @@ -163,6 +163,7 @@ static int dispatch_workload(struct 
> > > intel_vgpu_workload *workload)
> > >   int ring_id = workload->ring_id;
> > >   struct i915_gem_context *shadow_ctx = workload->vgpu->shadow_ctx;
> > >   struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv;
> > > + struct drm_i915_gem_request *rq;
> > >   int ret;
> > >  
> > >   gvt_dbg_sched("ring id %d prepare to dispatch workload %p\n",
> > > @@ -171,17 +172,16 @@ static int dispatch_workload(struct 
> > > intel_vgpu_workload *workload)
> > >   shadow_ctx->desc_template = workload->ctx_desc.addressing_mode <<
> > >   GEN8_CTX_ADDRESSING_MODE_SHIFT;
> > >  
> > > - workload->req = i915_gem_request_alloc(dev_priv->engine[ring_id],
> > > -shadow_ctx);
> > > - if (IS_ERR_OR_NULL(workload->req)) {
> > > + rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
> > > + if (IS_ERR(rq)) {
> > >   gvt_err("fail to allocate gem request\n");
> > > - workload->status = PTR_ERR(workload->req);
> > > - workload->req = NULL;
> > > + workload->status = PTR_ERR(rq);
> > >   return workload->status;
> > >   }
> > >  
> > > - gvt_dbg_sched("ring id %d get i915 gem request %p\n",
> > > - ring_id, workload->req);
> > > + gvt_dbg_sched("ring id %d get i915 gem request %p\n", ring_id, rq);
> > > +
> > > + workload->req = i915_gem_request_get(rq);
> > >  
> > >   mutex_lock(>lock);
> > >  
> > > @@ -208,16 +208,16 @@ static int dispatch_workload(struct 
> > > intel_vgpu_workload *workload)
> > >   gvt_dbg_sched("ring id %d submit workload to i915 %p\n",
> > >   ring_id, workload->req);
> > >  
> > > - i915_add_request_no_flush(workload->req);
> > > -
> > > + i915_add_request_no_flush(rq);
> > >   workload->dispatched = true;
> > >   return 0;
> > >  err:
> > >   workload->status = ret;
> > > - if (workload->req)
> > > - workload->req = NULL;
> > > + i915_gem_request_put(fetch_and_zero(>req));
> > >
> > 
> > Looks we don't need put here as in error path from dispatch_workload()
> > we will go with below put path too in main thread.
> 
> If we clear the request pointer, then we need the put. But yes, we don't
> necessarily need to clear the pointer on error for the caller, as the
> caller doesn't distinguish the error path and the no-op request can be
> handled identically to a real request.

Would you refresh this one? So I'd send out next pull request with this.

thanks

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 07/12] drm/i915/gvt: Hold a reference on the request

2016-10-20 Thread Chris Wilson 
On Thu, Oct 20, 2016 at 08:22:00AM +0800, Zhenyu Wang wrote:
> On 2016.10.19 11:11:42 +0100, Chris Wilson wrote:
> > The workload took a pointer to the request, and even waited upon,
> > without holding a reference on the request. Take that reference
> > explicitly and fix up the error path following request allocation that
> > missed flushing the request.
> > 
> > Signed-off-by: Chris Wilson 
> > ---
> >  drivers/gpu/drm/i915/gvt/scheduler.c | 24 +---
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c 
> > b/drivers/gpu/drm/i915/gvt/scheduler.c
> > index b15cdf5978a9..224f19ae61ab 100644
> > --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> > +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> > @@ -163,6 +163,7 @@ static int dispatch_workload(struct intel_vgpu_workload 
> > *workload)
> > int ring_id = workload->ring_id;
> > struct i915_gem_context *shadow_ctx = workload->vgpu->shadow_ctx;
> > struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv;
> > +   struct drm_i915_gem_request *rq;
> > int ret;
> >  
> > gvt_dbg_sched("ring id %d prepare to dispatch workload %p\n",
> > @@ -171,17 +172,16 @@ static int dispatch_workload(struct 
> > intel_vgpu_workload *workload)
> > shadow_ctx->desc_template = workload->ctx_desc.addressing_mode <<
> > GEN8_CTX_ADDRESSING_MODE_SHIFT;
> >  
> > -   workload->req = i915_gem_request_alloc(dev_priv->engine[ring_id],
> > -  shadow_ctx);
> > -   if (IS_ERR_OR_NULL(workload->req)) {
> > +   rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
> > +   if (IS_ERR(rq)) {
> > gvt_err("fail to allocate gem request\n");
> > -   workload->status = PTR_ERR(workload->req);
> > -   workload->req = NULL;
> > +   workload->status = PTR_ERR(rq);
> > return workload->status;
> > }
> >  
> > -   gvt_dbg_sched("ring id %d get i915 gem request %p\n",
> > -   ring_id, workload->req);
> > +   gvt_dbg_sched("ring id %d get i915 gem request %p\n", ring_id, rq);
> > +
> > +   workload->req = i915_gem_request_get(rq);
> >  
> > mutex_lock(>lock);
> >  
> > @@ -208,16 +208,16 @@ static int dispatch_workload(struct 
> > intel_vgpu_workload *workload)
> > gvt_dbg_sched("ring id %d submit workload to i915 %p\n",
> > ring_id, workload->req);
> >  
> > -   i915_add_request_no_flush(workload->req);
> > -
> > +   i915_add_request_no_flush(rq);
> > workload->dispatched = true;
> > return 0;
> >  err:
> > workload->status = ret;
> > -   if (workload->req)
> > -   workload->req = NULL;
> > +   i915_gem_request_put(fetch_and_zero(>req));
> >
> 
> Looks we don't need put here as in error path from dispatch_workload()
> we will go with below put path too in main thread.

If we clear the request pointer, then we need the put. But yes, we don't
necessarily need to clear the pointer on error for the caller, as the
caller doesn't distinguish the error path and the no-op request can be
handled identically to a real request.
-Chris

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

Re: [Intel-gfx] [PATCH 07/12] drm/i915/gvt: Hold a reference on the request

2016-10-19 Thread Zhenyu Wang 
On 2016.10.19 11:11:42 +0100, Chris Wilson wrote:
> The workload took a pointer to the request, and even waited upon,
> without holding a reference on the request. Take that reference
> explicitly and fix up the error path following request allocation that
> missed flushing the request.
> 
> Signed-off-by: Chris Wilson 
> ---
>  drivers/gpu/drm/i915/gvt/scheduler.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c 
> b/drivers/gpu/drm/i915/gvt/scheduler.c
> index b15cdf5978a9..224f19ae61ab 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -163,6 +163,7 @@ static int dispatch_workload(struct intel_vgpu_workload 
> *workload)
>   int ring_id = workload->ring_id;
>   struct i915_gem_context *shadow_ctx = workload->vgpu->shadow_ctx;
>   struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv;
> + struct drm_i915_gem_request *rq;
>   int ret;
>  
>   gvt_dbg_sched("ring id %d prepare to dispatch workload %p\n",
> @@ -171,17 +172,16 @@ static int dispatch_workload(struct intel_vgpu_workload 
> *workload)
>   shadow_ctx->desc_template = workload->ctx_desc.addressing_mode <<
>   GEN8_CTX_ADDRESSING_MODE_SHIFT;
>  
> - workload->req = i915_gem_request_alloc(dev_priv->engine[ring_id],
> -shadow_ctx);
> - if (IS_ERR_OR_NULL(workload->req)) {
> + rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
> + if (IS_ERR(rq)) {
>   gvt_err("fail to allocate gem request\n");
> - workload->status = PTR_ERR(workload->req);
> - workload->req = NULL;
> + workload->status = PTR_ERR(rq);
>   return workload->status;
>   }
>  
> - gvt_dbg_sched("ring id %d get i915 gem request %p\n",
> - ring_id, workload->req);
> + gvt_dbg_sched("ring id %d get i915 gem request %p\n", ring_id, rq);
> +
> + workload->req = i915_gem_request_get(rq);
>  
>   mutex_lock(>lock);
>  
> @@ -208,16 +208,16 @@ static int dispatch_workload(struct intel_vgpu_workload 
> *workload)
>   gvt_dbg_sched("ring id %d submit workload to i915 %p\n",
>   ring_id, workload->req);
>  
> - i915_add_request_no_flush(workload->req);
> -
> + i915_add_request_no_flush(rq);
>   workload->dispatched = true;
>   return 0;
>  err:
>   workload->status = ret;
> - if (workload->req)
> - workload->req = NULL;
> + i915_gem_request_put(fetch_and_zero(>req));
>

Looks we don't need put here as in error path from dispatch_workload()
we will go with below put path too in main thread.

>   mutex_unlock(>lock);
> +
> + i915_add_request_no_flush(rq);
>   return ret;
>  }
>  
> @@ -458,6 +458,8 @@ static int workload_thread(void *priv)
>  
>   complete_current_workload(gvt, ring_id);
>  
> + i915_gem_request_put(fetch_and_zero(>req));
> +
>   if (need_force_wake)
>   intel_uncore_forcewake_put(gvt->dev_priv,
>   FORCEWAKE_ALL);


-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 07/12] drm/i915/gvt: Hold a reference on the request

2016-10-19 Thread Chris Wilson 
On Wed, Oct 19, 2016 at 06:32:54PM +0800, Zhenyu Wang wrote:
> On 2016.10.19 11:11:42 +0100, Chris Wilson wrote:
> > The workload took a pointer to the request, and even waited upon,
> > without holding a reference on the request. Take that reference
> > explicitly and fix up the error path following request allocation that
> > missed flushing the request.
> > 
> > Signed-off-by: Chris Wilson 
> > ---
> >  drivers/gpu/drm/i915/gvt/scheduler.c | 24 +---
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c 
> > b/drivers/gpu/drm/i915/gvt/scheduler.c
> > index b15cdf5978a9..224f19ae61ab 100644
> > --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> > +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> > @@ -163,6 +163,7 @@ static int dispatch_workload(struct intel_vgpu_workload 
> > *workload)
> > int ring_id = workload->ring_id;
> > struct i915_gem_context *shadow_ctx = workload->vgpu->shadow_ctx;
> > struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv;
> > +   struct drm_i915_gem_request *rq;
> > int ret;
> >  
> > gvt_dbg_sched("ring id %d prepare to dispatch workload %p\n",
> > @@ -171,17 +172,16 @@ static int dispatch_workload(struct 
> > intel_vgpu_workload *workload)
> > shadow_ctx->desc_template = workload->ctx_desc.addressing_mode <<
> > GEN8_CTX_ADDRESSING_MODE_SHIFT;
> >  
> > -   workload->req = i915_gem_request_alloc(dev_priv->engine[ring_id],
> > -  shadow_ctx);
> > -   if (IS_ERR_OR_NULL(workload->req)) {
> > +   rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
> > +   if (IS_ERR(rq)) {
> > gvt_err("fail to allocate gem request\n");
> > -   workload->status = PTR_ERR(workload->req);
> > -   workload->req = NULL;
> > +   workload->status = PTR_ERR(rq);
> > return workload->status;
> > }
> >  
> > -   gvt_dbg_sched("ring id %d get i915 gem request %p\n",
> > -   ring_id, workload->req);
> > +   gvt_dbg_sched("ring id %d get i915 gem request %p\n", ring_id, rq);
> > +
> > +   workload->req = i915_gem_request_get(rq);
> >  
> > mutex_lock(>lock);
> >  
> > @@ -208,16 +208,16 @@ static int dispatch_workload(struct 
> > intel_vgpu_workload *workload)
> > gvt_dbg_sched("ring id %d submit workload to i915 %p\n",
> > ring_id, workload->req);
> >  
> > -   i915_add_request_no_flush(workload->req);
> > -
> > +   i915_add_request_no_flush(rq);
> > workload->dispatched = true;
> > return 0;
> >  err:
> > workload->status = ret;
> > -   if (workload->req)
> > -   workload->req = NULL;
> > +   i915_gem_request_put(fetch_and_zero(>req));
> >  
> > mutex_unlock(>lock);
> 
> Might not need to hold gvt->lock when put request?

I was just updating the current worklod->req = NULL which was under the
lock, using the same code as later. You can drop i915_gem_request_put(rq)
afterwards which is what I did at first, before deciding using the same
style for both was nicer.

> > +
> > +   i915_add_request_no_flush(rq);
> 
> Why still add request in error path?

The request may have changed global state which is now associated with
the request. You have to pass it back upon completion, whether or not
you have added your own workload to the request.
-Chris

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

Re: [Intel-gfx] [PATCH 07/12] drm/i915/gvt: Hold a reference on the request

2016-10-19 Thread Zhenyu Wang 
On 2016.10.19 11:11:42 +0100, Chris Wilson wrote:
> The workload took a pointer to the request, and even waited upon,
> without holding a reference on the request. Take that reference
> explicitly and fix up the error path following request allocation that
> missed flushing the request.
> 
> Signed-off-by: Chris Wilson 
> ---
>  drivers/gpu/drm/i915/gvt/scheduler.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c 
> b/drivers/gpu/drm/i915/gvt/scheduler.c
> index b15cdf5978a9..224f19ae61ab 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -163,6 +163,7 @@ static int dispatch_workload(struct intel_vgpu_workload 
> *workload)
>   int ring_id = workload->ring_id;
>   struct i915_gem_context *shadow_ctx = workload->vgpu->shadow_ctx;
>   struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv;
> + struct drm_i915_gem_request *rq;
>   int ret;
>  
>   gvt_dbg_sched("ring id %d prepare to dispatch workload %p\n",
> @@ -171,17 +172,16 @@ static int dispatch_workload(struct intel_vgpu_workload 
> *workload)
>   shadow_ctx->desc_template = workload->ctx_desc.addressing_mode <<
>   GEN8_CTX_ADDRESSING_MODE_SHIFT;
>  
> - workload->req = i915_gem_request_alloc(dev_priv->engine[ring_id],
> -shadow_ctx);
> - if (IS_ERR_OR_NULL(workload->req)) {
> + rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
> + if (IS_ERR(rq)) {
>   gvt_err("fail to allocate gem request\n");
> - workload->status = PTR_ERR(workload->req);
> - workload->req = NULL;
> + workload->status = PTR_ERR(rq);
>   return workload->status;
>   }
>  
> - gvt_dbg_sched("ring id %d get i915 gem request %p\n",
> - ring_id, workload->req);
> + gvt_dbg_sched("ring id %d get i915 gem request %p\n", ring_id, rq);
> +
> + workload->req = i915_gem_request_get(rq);
>  
>   mutex_lock(>lock);
>  
> @@ -208,16 +208,16 @@ static int dispatch_workload(struct intel_vgpu_workload 
> *workload)
>   gvt_dbg_sched("ring id %d submit workload to i915 %p\n",
>   ring_id, workload->req);
>  
> - i915_add_request_no_flush(workload->req);
> -
> + i915_add_request_no_flush(rq);
>   workload->dispatched = true;
>   return 0;
>  err:
>   workload->status = ret;
> - if (workload->req)
> - workload->req = NULL;
> + i915_gem_request_put(fetch_and_zero(>req));
>  
>   mutex_unlock(>lock);

Might not need to hold gvt->lock when put request?

> +
> + i915_add_request_no_flush(rq);

Why still add request in error path?

>   return ret;
>  }
>  
> @@ -458,6 +458,8 @@ static int workload_thread(void *priv)
>  
>   complete_current_workload(gvt, ring_id);
>  
> + i915_gem_request_put(fetch_and_zero(>req));
> +
>   if (need_force_wake)
>   intel_uncore_forcewake_put(gvt->dev_priv,
>   FORCEWAKE_ALL);
> -- 
> 2.9.3
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 07/12] drm/i915/gvt: Hold a reference on the request

2016-10-19 Thread Chris Wilson 
The workload took a pointer to the request, and even waited upon,
without holding a reference on the request. Take that reference
explicitly and fix up the error path following request allocation that
missed flushing the request.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/gvt/scheduler.c | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c 
b/drivers/gpu/drm/i915/gvt/scheduler.c
index b15cdf5978a9..224f19ae61ab 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -163,6 +163,7 @@ static int dispatch_workload(struct intel_vgpu_workload 
*workload)
int ring_id = workload->ring_id;
struct i915_gem_context *shadow_ctx = workload->vgpu->shadow_ctx;
struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv;
+   struct drm_i915_gem_request *rq;
int ret;
 
gvt_dbg_sched("ring id %d prepare to dispatch workload %p\n",
@@ -171,17 +172,16 @@ static int dispatch_workload(struct intel_vgpu_workload 
*workload)
shadow_ctx->desc_template = workload->ctx_desc.addressing_mode <<
GEN8_CTX_ADDRESSING_MODE_SHIFT;
 
-   workload->req = i915_gem_request_alloc(dev_priv->engine[ring_id],
-  shadow_ctx);
-   if (IS_ERR_OR_NULL(workload->req)) {
+   rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
+   if (IS_ERR(rq)) {
gvt_err("fail to allocate gem request\n");
-   workload->status = PTR_ERR(workload->req);
-   workload->req = NULL;
+   workload->status = PTR_ERR(rq);
return workload->status;
}
 
-   gvt_dbg_sched("ring id %d get i915 gem request %p\n",
-   ring_id, workload->req);
+   gvt_dbg_sched("ring id %d get i915 gem request %p\n", ring_id, rq);
+
+   workload->req = i915_gem_request_get(rq);
 
mutex_lock(>lock);
 
@@ -208,16 +208,16 @@ static int dispatch_workload(struct intel_vgpu_workload 
*workload)
gvt_dbg_sched("ring id %d submit workload to i915 %p\n",
ring_id, workload->req);
 
-   i915_add_request_no_flush(workload->req);
-
+   i915_add_request_no_flush(rq);
workload->dispatched = true;
return 0;
 err:
workload->status = ret;
-   if (workload->req)
-   workload->req = NULL;
+   i915_gem_request_put(fetch_and_zero(>req));
 
mutex_unlock(>lock);
+
+   i915_add_request_no_flush(rq);
return ret;
 }
 
@@ -458,6 +458,8 @@ static int workload_thread(void *priv)
 
complete_current_workload(gvt, ring_id);
 
+   i915_gem_request_put(fetch_and_zero(>req));
+
if (need_force_wake)
intel_uncore_forcewake_put(gvt->dev_priv,
FORCEWAKE_ALL);
-- 
2.9.3

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