* [PATCH 2/3] drm/i915: Fix some invalid requests cancellations
2016-01-29 16:49 [PATCH 1/3] drm/i915: Allow i915_gem_object_get_page() on userptr as well Chris Wilson
@ 2016-01-29 16:49 ` Chris Wilson
2016-02-11 13:01 ` Tvrtko Ursulin
2016-01-29 16:49 ` [PATCH 3/3] drm/i915: Don't ERROR for an expected intel_rcs_ctx_init() interruption Chris Wilson
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2016-01-29 16:49 UTC (permalink / raw)
To: intel-gfx; +Cc: Chris Wilson, Daniel Vetter, stable
As we add the VMA to the request early, it may be cancelled during
execbuf reservation. This will leave the context object pointing to a
dangling request; i915_wait_request() simply skips the wait and so we
may unbind the object whilst it is still active.
However, if at any point we make a change to the hardware (and equally
importantly our bookkeeping in the driver), we cannot cancel the request
as what has already been written must be submitted. Submitting a partial
request is far easier than trying to unwind the incomplete change.
Unfortunately this patch undoes the excess breadcrumb usage that olr
prevented, e.g. if we interrupt batchbuffer submission then we submit
the requests along with the memory writes and interrupt (even though we
do no real work). Disassociating requests from breadcrumbs (and
semaphores) is a topic for a past/future series, but now much more
important.
v2: Rebase
Note that igt/gem_concurrent_blit tiggers both misrendering and a GPF
that is fixed by this patch.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93907
Testcase: igt/gem_concurrent_blit
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
---
drivers/gpu/drm/i915/i915_drv.h | 1 -
drivers/gpu/drm/i915/i915_gem.c | 7 ++-----
drivers/gpu/drm/i915/i915_gem_context.c | 21 +++++++++------------
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 16 +++++-----------
drivers/gpu/drm/i915/intel_display.c | 2 +-
drivers/gpu/drm/i915/intel_lrc.c | 1 -
6 files changed, 17 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a2d2f08b7515..f828a7ffed37 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2823,7 +2823,6 @@ int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
void i915_gem_execbuffer_move_to_active(struct list_head *vmas,
struct drm_i915_gem_request *req);
-void i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params);
int i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
struct drm_i915_gem_execbuffer2 *args,
struct list_head *vmas);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e9b19bca1383..f764f33580fc 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3407,12 +3407,9 @@ int i915_gpu_idle(struct drm_device *dev)
return PTR_ERR(req);
ret = i915_switch_context(req);
- if (ret) {
- i915_gem_request_cancel(req);
- return ret;
- }
-
i915_add_request_no_flush(req);
+ if (ret)
+ return ret;
}
ret = intel_ring_idle(ring);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 83a097c94911..5da7adc3f7b2 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -652,7 +652,6 @@ static int do_switch(struct drm_i915_gem_request *req)
struct drm_i915_private *dev_priv = ring->dev->dev_private;
struct intel_context *from = ring->last_context;
u32 hw_flags = 0;
- bool uninitialized = false;
int ret, i;
if (from != NULL && ring == &dev_priv->ring[RCS]) {
@@ -759,6 +758,15 @@ static int do_switch(struct drm_i915_gem_request *req)
to->remap_slice &= ~(1<<i);
}
+ if (!to->legacy_hw_ctx.initialized) {
+ if (ring->init_context) {
+ ret = ring->init_context(req);
+ if (ret)
+ goto unpin_out;
+ }
+ to->legacy_hw_ctx.initialized = true;
+ }
+
/* The backing object for the context is done after switching to the
* *next* context. Therefore we cannot retire the previous context until
* the next context has already started running. In fact, the below code
@@ -782,21 +790,10 @@ static int do_switch(struct drm_i915_gem_request *req)
i915_gem_context_unreference(from);
}
- uninitialized = !to->legacy_hw_ctx.initialized;
- to->legacy_hw_ctx.initialized = true;
-
done:
i915_gem_context_reference(to);
ring->last_context = to;
- if (uninitialized) {
- if (ring->init_context) {
- ret = ring->init_context(req);
- if (ret)
- DRM_ERROR("ring init context: %d\n", ret);
- }
- }
-
return 0;
unpin_out:
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 8fd00d279447..61bb15507b30 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1133,7 +1133,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
}
}
-void
+static void
i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params)
{
/* Unconditionally force add_request to emit a full flush. */
@@ -1318,7 +1318,6 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
i915_gem_execbuffer_move_to_active(vmas, params->request);
- i915_gem_execbuffer_retire_commands(params);
return 0;
}
@@ -1616,8 +1615,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
}
ret = i915_gem_request_add_to_client(req, file);
- if (ret)
+ if (ret) {
+ i915_gem_request_cancel(req);
goto err_batch_unpin;
+ }
/*
* Save assorted stuff away to pass through to *_submission().
@@ -1634,6 +1635,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
params->request = req;
ret = dev_priv->gt.execbuf_submit(params, args, &eb->vmas);
+ i915_gem_execbuffer_retire_commands(params);
err_batch_unpin:
/*
@@ -1650,14 +1652,6 @@ err:
i915_gem_context_unreference(ctx);
eb_destroy(eb);
- /*
- * If the request was created but not successfully submitted then it
- * must be freed again. If it was submitted then it is being tracked
- * on the active request list and no clean up is required here.
- */
- if (ret && !IS_ERR_OR_NULL(req))
- i915_gem_request_cancel(req);
-
mutex_unlock(&dev->struct_mutex);
pre_mutex_err:
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 304fc9637026..732c915bef9e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11723,7 +11723,7 @@ cleanup_unpin:
intel_unpin_fb_obj(fb, crtc->primary->state);
cleanup_pending:
if (!IS_ERR_OR_NULL(request))
- i915_gem_request_cancel(request);
+ i915_add_request_no_flush(request);
atomic_dec(&intel_crtc->unpin_work_count);
mutex_unlock(&dev->struct_mutex);
cleanup:
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3a03646e343d..4edd700e6402 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1002,7 +1002,6 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
i915_gem_execbuffer_move_to_active(vmas, params->request);
- i915_gem_execbuffer_retire_commands(params);
return 0;
}
--
2.7.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 2/3] drm/i915: Fix some invalid requests cancellations
2016-01-29 16:49 ` [PATCH 2/3] drm/i915: Fix some invalid requests cancellations Chris Wilson
@ 2016-02-11 13:01 ` Tvrtko Ursulin
2016-02-15 9:47 ` Daniel Vetter
0 siblings, 1 reply; 12+ messages in thread
From: Tvrtko Ursulin @ 2016-02-11 13:01 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Daniel Vetter, stable
On 29/01/16 16:49, Chris Wilson wrote:
> As we add the VMA to the request early, it may be cancelled during
> execbuf reservation. This will leave the context object pointing to a
I don't get it, request is created after the reservation.
> dangling request; i915_wait_request() simply skips the wait and so we
> may unbind the object whilst it is still active.
>
> However, if at any point we make a change to the hardware (and equally
> importantly our bookkeeping in the driver), we cannot cancel the request
> as what has already been written must be submitted. Submitting a partial
> request is far easier than trying to unwind the incomplete change.
>
> Unfortunately this patch undoes the excess breadcrumb usage that olr
> prevented, e.g. if we interrupt batchbuffer submission then we submit
> the requests along with the memory writes and interrupt (even though we
> do no real work). Disassociating requests from breadcrumbs (and
> semaphores) is a topic for a past/future series, but now much more
> important.
>
> v2: Rebase
>
> Note that igt/gem_concurrent_blit tiggers both misrendering and a GPF
> that is fixed by this patch.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93907
> Testcase: igt/gem_concurrent_blit
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: stable@vger.kernel.org
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 -
> drivers/gpu/drm/i915/i915_gem.c | 7 ++-----
> drivers/gpu/drm/i915/i915_gem_context.c | 21 +++++++++------------
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 16 +++++-----------
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_lrc.c | 1 -
> 6 files changed, 17 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a2d2f08b7515..f828a7ffed37 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2823,7 +2823,6 @@ int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> void i915_gem_execbuffer_move_to_active(struct list_head *vmas,
> struct drm_i915_gem_request *req);
> -void i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params);
> int i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
> struct drm_i915_gem_execbuffer2 *args,
> struct list_head *vmas);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e9b19bca1383..f764f33580fc 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3407,12 +3407,9 @@ int i915_gpu_idle(struct drm_device *dev)
> return PTR_ERR(req);
>
> ret = i915_switch_context(req);
> - if (ret) {
> - i915_gem_request_cancel(req);
> - return ret;
> - }
> -
> i915_add_request_no_flush(req);
> + if (ret)
> + return ret;
> }
>
> ret = intel_ring_idle(ring);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 83a097c94911..5da7adc3f7b2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -652,7 +652,6 @@ static int do_switch(struct drm_i915_gem_request *req)
> struct drm_i915_private *dev_priv = ring->dev->dev_private;
> struct intel_context *from = ring->last_context;
> u32 hw_flags = 0;
> - bool uninitialized = false;
> int ret, i;
>
> if (from != NULL && ring == &dev_priv->ring[RCS]) {
> @@ -759,6 +758,15 @@ static int do_switch(struct drm_i915_gem_request *req)
> to->remap_slice &= ~(1<<i);
> }
>
> + if (!to->legacy_hw_ctx.initialized) {
> + if (ring->init_context) {
> + ret = ring->init_context(req);
> + if (ret)
> + goto unpin_out;
> + }
> + to->legacy_hw_ctx.initialized = true;
> + }
> +
> /* The backing object for the context is done after switching to the
> * *next* context. Therefore we cannot retire the previous context until
> * the next context has already started running. In fact, the below code
> @@ -782,21 +790,10 @@ static int do_switch(struct drm_i915_gem_request *req)
> i915_gem_context_unreference(from);
> }
>
> - uninitialized = !to->legacy_hw_ctx.initialized;
> - to->legacy_hw_ctx.initialized = true;
> -
> done:
> i915_gem_context_reference(to);
> ring->last_context = to;
>
> - if (uninitialized) {
> - if (ring->init_context) {
> - ret = ring->init_context(req);
> - if (ret)
> - DRM_ERROR("ring init context: %d\n", ret);
> - }
> - }
> -
> return 0;
>
> unpin_out:
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 8fd00d279447..61bb15507b30 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1133,7 +1133,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
> }
> }
>
> -void
> +static void
> i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params)
> {
> /* Unconditionally force add_request to emit a full flush. */
> @@ -1318,7 +1318,6 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
> trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
>
> i915_gem_execbuffer_move_to_active(vmas, params->request);
> - i915_gem_execbuffer_retire_commands(params);
Hm this whole block block from trace_... to
i915_gem_execbuffer_retire_commands could be moved to do_execbuffer as
and independent cleanup patch if under "if (ret == 0)".
So the concern is that something has been written to the ringbuffer but
not all of it?
Why do we have to submit that, I am assuming whatever was written is not
interesting to be executed unless the whole bb went in.
So could we just rewind the pointers? Store them at the beginning and
rewind if something failed.
>
> return 0;
> }
> @@ -1616,8 +1615,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> }
>
> ret = i915_gem_request_add_to_client(req, file);
> - if (ret)
> + if (ret) {
> + i915_gem_request_cancel(req);
> goto err_batch_unpin;
> + }
Doesn't look like this can fail so a side cleanup only?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/3] drm/i915: Fix some invalid requests cancellations
2016-02-11 13:01 ` Tvrtko Ursulin
@ 2016-02-15 9:47 ` Daniel Vetter
2016-02-19 12:29 ` Tvrtko Ursulin
0 siblings, 1 reply; 12+ messages in thread
From: Daniel Vetter @ 2016-02-15 9:47 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Daniel Vetter, intel-gfx, stable
On Thu, Feb 11, 2016 at 01:01:34PM +0000, Tvrtko Ursulin wrote:
> On 29/01/16 16:49, Chris Wilson wrote:
> >As we add the VMA to the request early, it may be cancelled during
> >execbuf reservation. This will leave the context object pointing to a
>
> I don't get it, request is created after the reservation.
That's the problem - we add vma for a given request (well ring/seqno pair)
before that request exists.
-Daniel
>
> >dangling request; i915_wait_request() simply skips the wait and so we
> >may unbind the object whilst it is still active.
> >
> >However, if at any point we make a change to the hardware (and equally
> >importantly our bookkeeping in the driver), we cannot cancel the request
> >as what has already been written must be submitted. Submitting a partial
> >request is far easier than trying to unwind the incomplete change.
> >
> >Unfortunately this patch undoes the excess breadcrumb usage that olr
> >prevented, e.g. if we interrupt batchbuffer submission then we submit
> >the requests along with the memory writes and interrupt (even though we
> >do no real work). Disassociating requests from breadcrumbs (and
> >semaphores) is a topic for a past/future series, but now much more
> >important.
> >
> >v2: Rebase
> >
> >Note that igt/gem_concurrent_blit tiggers both misrendering and a GPF
> >that is fixed by this patch.
> >
> >Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93907
> >Testcase: igt/gem_concurrent_blit
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >Cc: stable@vger.kernel.org
> >---
> > drivers/gpu/drm/i915/i915_drv.h | 1 -
> > drivers/gpu/drm/i915/i915_gem.c | 7 ++-----
> > drivers/gpu/drm/i915/i915_gem_context.c | 21 +++++++++------------
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 16 +++++-----------
> > drivers/gpu/drm/i915/intel_display.c | 2 +-
> > drivers/gpu/drm/i915/intel_lrc.c | 1 -
> > 6 files changed, 17 insertions(+), 31 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index a2d2f08b7515..f828a7ffed37 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -2823,7 +2823,6 @@ int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
> > struct drm_file *file_priv);
> > void i915_gem_execbuffer_move_to_active(struct list_head *vmas,
> > struct drm_i915_gem_request *req);
> >-void i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params);
> > int i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
> > struct drm_i915_gem_execbuffer2 *args,
> > struct list_head *vmas);
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index e9b19bca1383..f764f33580fc 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -3407,12 +3407,9 @@ int i915_gpu_idle(struct drm_device *dev)
> > return PTR_ERR(req);
> >
> > ret = i915_switch_context(req);
> >- if (ret) {
> >- i915_gem_request_cancel(req);
> >- return ret;
> >- }
> >-
> > i915_add_request_no_flush(req);
> >+ if (ret)
> >+ return ret;
> > }
> >
> > ret = intel_ring_idle(ring);
> >diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> >index 83a097c94911..5da7adc3f7b2 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_context.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_context.c
> >@@ -652,7 +652,6 @@ static int do_switch(struct drm_i915_gem_request *req)
> > struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > struct intel_context *from = ring->last_context;
> > u32 hw_flags = 0;
> >- bool uninitialized = false;
> > int ret, i;
> >
> > if (from != NULL && ring == &dev_priv->ring[RCS]) {
> >@@ -759,6 +758,15 @@ static int do_switch(struct drm_i915_gem_request *req)
> > to->remap_slice &= ~(1<<i);
> > }
> >
> >+ if (!to->legacy_hw_ctx.initialized) {
> >+ if (ring->init_context) {
> >+ ret = ring->init_context(req);
> >+ if (ret)
> >+ goto unpin_out;
> >+ }
> >+ to->legacy_hw_ctx.initialized = true;
> >+ }
> >+
> > /* The backing object for the context is done after switching to the
> > * *next* context. Therefore we cannot retire the previous context until
> > * the next context has already started running. In fact, the below code
> >@@ -782,21 +790,10 @@ static int do_switch(struct drm_i915_gem_request *req)
> > i915_gem_context_unreference(from);
> > }
> >
> >- uninitialized = !to->legacy_hw_ctx.initialized;
> >- to->legacy_hw_ctx.initialized = true;
> >-
> > done:
> > i915_gem_context_reference(to);
> > ring->last_context = to;
> >
> >- if (uninitialized) {
> >- if (ring->init_context) {
> >- ret = ring->init_context(req);
> >- if (ret)
> >- DRM_ERROR("ring init context: %d\n", ret);
> >- }
> >- }
> >-
> > return 0;
> >
> > unpin_out:
> >diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >index 8fd00d279447..61bb15507b30 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >@@ -1133,7 +1133,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
> > }
> > }
> >
> >-void
> >+static void
> > i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params)
> > {
> > /* Unconditionally force add_request to emit a full flush. */
> >@@ -1318,7 +1318,6 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
> > trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
> >
> > i915_gem_execbuffer_move_to_active(vmas, params->request);
> >- i915_gem_execbuffer_retire_commands(params);
>
> Hm this whole block block from trace_... to
> i915_gem_execbuffer_retire_commands could be moved to do_execbuffer as and
> independent cleanup patch if under "if (ret == 0)".
>
> So the concern is that something has been written to the ringbuffer but not
> all of it?
>
> Why do we have to submit that, I am assuming whatever was written is not
> interesting to be executed unless the whole bb went in.
>
> So could we just rewind the pointers? Store them at the beginning and rewind
> if something failed.
>
> >
> > return 0;
> > }
> >@@ -1616,8 +1615,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> > }
> >
> > ret = i915_gem_request_add_to_client(req, file);
> >- if (ret)
> >+ if (ret) {
> >+ i915_gem_request_cancel(req);
> > goto err_batch_unpin;
> >+ }
>
> Doesn't look like this can fail so a side cleanup only?
>
> Regards,
>
> Tvrtko
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/3] drm/i915: Fix some invalid requests cancellations
2016-02-15 9:47 ` Daniel Vetter
@ 2016-02-19 12:29 ` Tvrtko Ursulin
0 siblings, 0 replies; 12+ messages in thread
From: Tvrtko Ursulin @ 2016-02-19 12:29 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx, stable
On 15/02/16 09:47, Daniel Vetter wrote:
> On Thu, Feb 11, 2016 at 01:01:34PM +0000, Tvrtko Ursulin wrote:
>> On 29/01/16 16:49, Chris Wilson wrote:
>>> As we add the VMA to the request early, it may be cancelled during
>>> execbuf reservation. This will leave the context object pointing to a
>>
>> I don't get it, request is created after the reservation.
>
> That's the problem - we add vma for a given request (well ring/seqno pair)
> before that request exists.
Why can't we remove them?
Sorry I just can't penetrate the commit message. Don't get what
wait_request is it talking about when the request is canceled so no
more. Or how do contexts point to requests etc.
Maybe then find someone else to review this.
Regards,
Tvrtko
> -Daniel
>
>>
>>> dangling request; i915_wait_request() simply skips the wait and so we
>>> may unbind the object whilst it is still active.
>>>
>>> However, if at any point we make a change to the hardware (and equally
>>> importantly our bookkeeping in the driver), we cannot cancel the request
>>> as what has already been written must be submitted. Submitting a partial
>>> request is far easier than trying to unwind the incomplete change.
>>>
>>> Unfortunately this patch undoes the excess breadcrumb usage that olr
>>> prevented, e.g. if we interrupt batchbuffer submission then we submit
>>> the requests along with the memory writes and interrupt (even though we
>>> do no real work). Disassociating requests from breadcrumbs (and
>>> semaphores) is a topic for a past/future series, but now much more
>>> important.
>>>
>>> v2: Rebase
>>>
>>> Note that igt/gem_concurrent_blit tiggers both misrendering and a GPF
>>> that is fixed by this patch.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93907
>>> Testcase: igt/gem_concurrent_blit
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: stable@vger.kernel.org
>>> ---
>>> drivers/gpu/drm/i915/i915_drv.h | 1 -
>>> drivers/gpu/drm/i915/i915_gem.c | 7 ++-----
>>> drivers/gpu/drm/i915/i915_gem_context.c | 21 +++++++++------------
>>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 16 +++++-----------
>>> drivers/gpu/drm/i915/intel_display.c | 2 +-
>>> drivers/gpu/drm/i915/intel_lrc.c | 1 -
>>> 6 files changed, 17 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index a2d2f08b7515..f828a7ffed37 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2823,7 +2823,6 @@ int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
>>> struct drm_file *file_priv);
>>> void i915_gem_execbuffer_move_to_active(struct list_head *vmas,
>>> struct drm_i915_gem_request *req);
>>> -void i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params);
>>> int i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>>> struct drm_i915_gem_execbuffer2 *args,
>>> struct list_head *vmas);
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>> index e9b19bca1383..f764f33580fc 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -3407,12 +3407,9 @@ int i915_gpu_idle(struct drm_device *dev)
>>> return PTR_ERR(req);
>>>
>>> ret = i915_switch_context(req);
>>> - if (ret) {
>>> - i915_gem_request_cancel(req);
>>> - return ret;
>>> - }
>>> -
>>> i915_add_request_no_flush(req);
>>> + if (ret)
>>> + return ret;
>>> }
>>>
>>> ret = intel_ring_idle(ring);
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
>>> index 83a097c94911..5da7adc3f7b2 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>>> @@ -652,7 +652,6 @@ static int do_switch(struct drm_i915_gem_request *req)
>>> struct drm_i915_private *dev_priv = ring->dev->dev_private;
>>> struct intel_context *from = ring->last_context;
>>> u32 hw_flags = 0;
>>> - bool uninitialized = false;
>>> int ret, i;
>>>
>>> if (from != NULL && ring == &dev_priv->ring[RCS]) {
>>> @@ -759,6 +758,15 @@ static int do_switch(struct drm_i915_gem_request *req)
>>> to->remap_slice &= ~(1<<i);
>>> }
>>>
>>> + if (!to->legacy_hw_ctx.initialized) {
>>> + if (ring->init_context) {
>>> + ret = ring->init_context(req);
>>> + if (ret)
>>> + goto unpin_out;
>>> + }
>>> + to->legacy_hw_ctx.initialized = true;
>>> + }
>>> +
>>> /* The backing object for the context is done after switching to the
>>> * *next* context. Therefore we cannot retire the previous context until
>>> * the next context has already started running. In fact, the below code
>>> @@ -782,21 +790,10 @@ static int do_switch(struct drm_i915_gem_request *req)
>>> i915_gem_context_unreference(from);
>>> }
>>>
>>> - uninitialized = !to->legacy_hw_ctx.initialized;
>>> - to->legacy_hw_ctx.initialized = true;
>>> -
>>> done:
>>> i915_gem_context_reference(to);
>>> ring->last_context = to;
>>>
>>> - if (uninitialized) {
>>> - if (ring->init_context) {
>>> - ret = ring->init_context(req);
>>> - if (ret)
>>> - DRM_ERROR("ring init context: %d\n", ret);
>>> - }
>>> - }
>>> -
>>> return 0;
>>>
>>> unpin_out:
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> index 8fd00d279447..61bb15507b30 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> @@ -1133,7 +1133,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
>>> }
>>> }
>>>
>>> -void
>>> +static void
>>> i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params)
>>> {
>>> /* Unconditionally force add_request to emit a full flush. */
>>> @@ -1318,7 +1318,6 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>>> trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
>>>
>>> i915_gem_execbuffer_move_to_active(vmas, params->request);
>>> - i915_gem_execbuffer_retire_commands(params);
>>
>> Hm this whole block block from trace_... to
>> i915_gem_execbuffer_retire_commands could be moved to do_execbuffer as and
>> independent cleanup patch if under "if (ret == 0)".
>>
>> So the concern is that something has been written to the ringbuffer but not
>> all of it?
>>
>> Why do we have to submit that, I am assuming whatever was written is not
>> interesting to be executed unless the whole bb went in.
>>
>> So could we just rewind the pointers? Store them at the beginning and rewind
>> if something failed.
>>
>>>
>>> return 0;
>>> }
>>> @@ -1616,8 +1615,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>> }
>>>
>>> ret = i915_gem_request_add_to_client(req, file);
>>> - if (ret)
>>> + if (ret) {
>>> + i915_gem_request_cancel(req);
>>> goto err_batch_unpin;
>>> + }
>>
>> Doesn't look like this can fail so a side cleanup only?
>>
>> Regards,
>>
>> Tvrtko
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] drm/i915: Don't ERROR for an expected intel_rcs_ctx_init() interruption
2016-01-29 16:49 [PATCH 1/3] drm/i915: Allow i915_gem_object_get_page() on userptr as well Chris Wilson
2016-01-29 16:49 ` [PATCH 2/3] drm/i915: Fix some invalid requests cancellations Chris Wilson
@ 2016-01-29 16:49 ` Chris Wilson
2016-02-11 12:12 ` Tvrtko Ursulin
2016-02-01 7:55 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Allow i915_gem_object_get_page() on userptr as well Patchwork
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2016-01-29 16:49 UTC (permalink / raw)
To: intel-gfx
intel_rcs_ctx_init() can be interrupted by a signal (if it has to wait
upon a full ring to advance). Don't emit an error for this.
Testcase: igt/gem_concurrent_blit
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 6f5b511bdb5d..b813bbc8e41c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -746,9 +746,9 @@ static int intel_rcs_ctx_init(struct drm_i915_gem_request *req)
ret = i915_gem_render_state_init(req);
if (ret)
- DRM_ERROR("init render state: %d\n", ret);
+ return ret;
- return ret;
+ return 0;
}
static int wa_add(struct drm_i915_private *dev_priv,
--
2.7.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 3/3] drm/i915: Don't ERROR for an expected intel_rcs_ctx_init() interruption
2016-01-29 16:49 ` [PATCH 3/3] drm/i915: Don't ERROR for an expected intel_rcs_ctx_init() interruption Chris Wilson
@ 2016-02-11 12:12 ` Tvrtko Ursulin
2016-02-15 9:48 ` Daniel Vetter
0 siblings, 1 reply; 12+ messages in thread
From: Tvrtko Ursulin @ 2016-02-11 12:12 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 29/01/16 16:49, Chris Wilson wrote:
> intel_rcs_ctx_init() can be interrupted by a signal (if it has to wait
> upon a full ring to advance). Don't emit an error for this.
>
> Testcase: igt/gem_concurrent_blit
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 6f5b511bdb5d..b813bbc8e41c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -746,9 +746,9 @@ static int intel_rcs_ctx_init(struct drm_i915_gem_request *req)
>
> ret = i915_gem_render_state_init(req);
> if (ret)
> - DRM_ERROR("init render state: %d\n", ret);
> + return ret;
>
> - return ret;
> + return 0;
> }
>
> static int wa_add(struct drm_i915_private *dev_priv,
>
Or just "return i915_gem_render_state_init(req);", but that is way below
the threshold.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 3/3] drm/i915: Don't ERROR for an expected intel_rcs_ctx_init() interruption
2016-02-11 12:12 ` Tvrtko Ursulin
@ 2016-02-15 9:48 ` Daniel Vetter
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2016-02-15 9:48 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Thu, Feb 11, 2016 at 12:12:26PM +0000, Tvrtko Ursulin wrote:
>
> On 29/01/16 16:49, Chris Wilson wrote:
> >intel_rcs_ctx_init() can be interrupted by a signal (if it has to wait
> >upon a full ring to advance). Don't emit an error for this.
> >
> >Testcase: igt/gem_concurrent_blit
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >index 6f5b511bdb5d..b813bbc8e41c 100644
> >--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >@@ -746,9 +746,9 @@ static int intel_rcs_ctx_init(struct drm_i915_gem_request *req)
> >
> > ret = i915_gem_render_state_init(req);
> > if (ret)
> >- DRM_ERROR("init render state: %d\n", ret);
> >+ return ret;
> >
> >- return ret;
> >+ return 0;
> > }
> >
> > static int wa_add(struct drm_i915_private *dev_priv,
> >
>
> Or just "return i915_gem_render_state_init(req);", but that is way below the
> threshold.
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Queued for -next, thanks for the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Allow i915_gem_object_get_page() on userptr as well
2016-01-29 16:49 [PATCH 1/3] drm/i915: Allow i915_gem_object_get_page() on userptr as well Chris Wilson
2016-01-29 16:49 ` [PATCH 2/3] drm/i915: Fix some invalid requests cancellations Chris Wilson
2016-01-29 16:49 ` [PATCH 3/3] drm/i915: Don't ERROR for an expected intel_rcs_ctx_init() interruption Chris Wilson
@ 2016-02-01 7:55 ` Patchwork
2016-02-03 18:27 ` [PATCH 1/3] " Rodrigo Vivi
2016-02-03 19:45 ` Jani Nikula
4 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2016-02-01 7:55 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Summary ==
Series 2929v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/2929/revisions/1/mbox/
Test kms_flip:
Subgroup basic-flip-vs-dpms:
pass -> DMESG-WARN (ilk-hp8440p) UNSTABLE
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-c:
dmesg-warn -> PASS (bsw-nuc-2)
bdw-nuci7 total:156 pass:147 dwarn:0 dfail:0 fail:0 skip:9
bdw-ultra total:159 pass:147 dwarn:0 dfail:0 fail:0 skip:12
bsw-nuc-2 total:159 pass:129 dwarn:0 dfail:0 fail:0 skip:30
byt-nuc total:159 pass:136 dwarn:0 dfail:0 fail:0 skip:23
hsw-brixbox total:159 pass:146 dwarn:0 dfail:0 fail:0 skip:13
ilk-hp8440p total:159 pass:110 dwarn:1 dfail:0 fail:0 skip:48
ivb-t430s total:159 pass:145 dwarn:0 dfail:0 fail:0 skip:14
skl-i5k-2 total:159 pass:144 dwarn:1 dfail:0 fail:0 skip:14
snb-dellxps total:159 pass:137 dwarn:0 dfail:0 fail:0 skip:22
Results at /archive/results/CI_IGT_test/Patchwork_1327/
6b1049b84dcd979f631d15b2ada325d8e5b2c4e1 drm-intel-nightly: 2016y-01m-29d-22h-50m-57s UTC integration manifest
2d19f3bf56071780e20aab50b36771e1b1f15dfb drm/i915: Don't ERROR for an expected intel_rcs_ctx_init() interruption
cec39bb746117d306211645da8f7d7b0987552b7 drm/i915: Fix some invalid requests cancellations
3f539b201c3923aabf098c9edbf6cbea279876fc drm/i915: Allow i915_gem_object_get_page() on userptr as well
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] drm/i915: Allow i915_gem_object_get_page() on userptr as well
2016-01-29 16:49 [PATCH 1/3] drm/i915: Allow i915_gem_object_get_page() on userptr as well Chris Wilson
` (2 preceding siblings ...)
2016-02-01 7:55 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Allow i915_gem_object_get_page() on userptr as well Patchwork
@ 2016-02-03 18:27 ` Rodrigo Vivi
2016-02-03 19:45 ` Jani Nikula
4 siblings, 0 replies; 12+ messages in thread
From: Rodrigo Vivi @ 2016-02-03 18:27 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Daniel Vetter
[-- Attachment #1.1: Type: text/plain, Size: 4699 bytes --]
Apparently this patch was blocking other teams that were pinging us at irc
and with 2 rv-b, 2 tested-by and ci success I merged this patch.
However other 2 patches in this series are still pending review so not
merged.
On Fri, Jan 29, 2016 at 8:49 AM Chris Wilson <chris@chris-wilson.co.uk>
wrote:
> commit 033908aed5a596f6202c848c6bbc8a40fb1a8490
> Author: Dave Gordon <david.s.gordon@intel.com>
> Date: Thu Dec 10 18:51:23 2015 +0000
>
> drm/i915: mark GEM object pages dirty when mapped & written by the CPU
>
> introduced a check into i915_gem_object_get_dirty_pages() that returned
> a NULL pointer when called with a bad object, one that was not backed by
> shmemfs. This WARN was too strict as we can work on all struct page
> backed objects, and resulted in a WARN + GPF for existing userspace. In
> order to differentiate the various types of objects, add a new flags field
> to the i915_gem_object_ops struct to describe their capabilities, with
> the first flag being whether the object has struct pages.
>
> v2: Drop silly const before an integer in the structure declaration.
>
> Testcase: igt/gem_userptr_blits/relocations
> Reported-and-tested-by: Kristian Høgsberg Kristensen <krh@bitplanet.net>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Cc: Kristian Høgsberg Kristensen <krh@bitplanet.net>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
> Reviewed-by: Kristian Høgsberg Kristensen <krh@bitplanet.net>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 4 ++++
> drivers/gpu/drm/i915/i915_gem.c | 3 ++-
> drivers/gpu/drm/i915/i915_gem_userptr.c | 3 ++-
> 3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 905e90f25957..a2d2f08b7515 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2000,6 +2000,9 @@ enum hdmi_force_audio {
> #define I915_GTT_OFFSET_NONE ((u32)-1)
>
> struct drm_i915_gem_object_ops {
> + unsigned int flags;
> +#define I915_GEM_OBJECT_HAS_STRUCT_PAGE 0x1
> +
> /* Interface between the GEM object and its backing storage.
> * get_pages() is called once prior to the use of the associated
> set
> * of pages before to binding them into the GTT, and put_pages() is
> @@ -2015,6 +2018,7 @@ struct drm_i915_gem_object_ops {
> */
> int (*get_pages)(struct drm_i915_gem_object *);
> void (*put_pages)(struct drm_i915_gem_object *);
> +
> int (*dmabuf_export)(struct drm_i915_gem_object *);
> void (*release)(struct drm_i915_gem_object *);
> };
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index a928823507c5..e9b19bca1383 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4465,6 +4465,7 @@ void i915_gem_object_init(struct drm_i915_gem_object
> *obj,
> }
>
> static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
> + .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE,
> .get_pages = i915_gem_object_get_pages_gtt,
> .put_pages = i915_gem_object_put_pages_gtt,
> };
> @@ -5309,7 +5310,7 @@ i915_gem_object_get_dirty_page(struct
> drm_i915_gem_object *obj, int n)
> struct page *page;
>
> /* Only default objects have per-page dirty tracking */
> - if (WARN_ON(obj->ops != &i915_gem_object_ops))
> + if (WARN_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) ==
> 0))
> return NULL;
>
> page = i915_gem_object_get_page(obj, n);
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c
> b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 74a4d1714879..7107f2fd38f5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -708,9 +708,10 @@ i915_gem_userptr_dmabuf_export(struct
> drm_i915_gem_object *obj)
> }
>
> static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
> - .dmabuf_export = i915_gem_userptr_dmabuf_export,
> + .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE,
> .get_pages = i915_gem_userptr_get_pages,
> .put_pages = i915_gem_userptr_put_pages,
> + .dmabuf_export = i915_gem_userptr_dmabuf_export,
> .release = i915_gem_userptr_release,
> };
>
> --
> 2.7.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
[-- Attachment #1.2: Type: text/html, Size: 5968 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] drm/i915: Allow i915_gem_object_get_page() on userptr as well
2016-01-29 16:49 [PATCH 1/3] drm/i915: Allow i915_gem_object_get_page() on userptr as well Chris Wilson
` (3 preceding siblings ...)
2016-02-03 18:27 ` [PATCH 1/3] " Rodrigo Vivi
@ 2016-02-03 19:45 ` Jani Nikula
2016-02-04 12:37 ` Jani Nikula
4 siblings, 1 reply; 12+ messages in thread
From: Jani Nikula @ 2016-02-03 19:45 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Daniel Vetter, drm-intel-fixes
On Fri, 29 Jan 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> commit 033908aed5a596f6202c848c6bbc8a40fb1a8490
> Author: Dave Gordon <david.s.gordon@intel.com>
> Date: Thu Dec 10 18:51:23 2015 +0000
>
> drm/i915: mark GEM object pages dirty when mapped & written by the CPU
>
> introduced a check into i915_gem_object_get_dirty_pages() that returned
> a NULL pointer when called with a bad object, one that was not backed by
> shmemfs. This WARN was too strict as we can work on all struct page
> backed objects, and resulted in a WARN + GPF for existing userspace. In
> order to differentiate the various types of objects, add a new flags field
> to the i915_gem_object_ops struct to describe their capabilities, with
> the first flag being whether the object has struct pages.
>
> v2: Drop silly const before an integer in the structure declaration.
>
> Testcase: igt/gem_userptr_blits/relocations
> Reported-and-tested-by: Kristian Høgsberg Kristensen <krh@bitplanet.net>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Cc: Kristian Høgsberg Kristensen <krh@bitplanet.net>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
> Reviewed-by: Kristian Høgsberg Kristensen <krh@bitplanet.net>
Cc: drm-intel-fixes@lists.freedesktop.org
to pick for 4.5
> ---
> drivers/gpu/drm/i915/i915_drv.h | 4 ++++
> drivers/gpu/drm/i915/i915_gem.c | 3 ++-
> drivers/gpu/drm/i915/i915_gem_userptr.c | 3 ++-
> 3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 905e90f25957..a2d2f08b7515 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2000,6 +2000,9 @@ enum hdmi_force_audio {
> #define I915_GTT_OFFSET_NONE ((u32)-1)
>
> struct drm_i915_gem_object_ops {
> + unsigned int flags;
> +#define I915_GEM_OBJECT_HAS_STRUCT_PAGE 0x1
> +
> /* Interface between the GEM object and its backing storage.
> * get_pages() is called once prior to the use of the associated set
> * of pages before to binding them into the GTT, and put_pages() is
> @@ -2015,6 +2018,7 @@ struct drm_i915_gem_object_ops {
> */
> int (*get_pages)(struct drm_i915_gem_object *);
> void (*put_pages)(struct drm_i915_gem_object *);
> +
> int (*dmabuf_export)(struct drm_i915_gem_object *);
> void (*release)(struct drm_i915_gem_object *);
> };
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a928823507c5..e9b19bca1383 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4465,6 +4465,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
> }
>
> static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
> + .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE,
> .get_pages = i915_gem_object_get_pages_gtt,
> .put_pages = i915_gem_object_put_pages_gtt,
> };
> @@ -5309,7 +5310,7 @@ i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj, int n)
> struct page *page;
>
> /* Only default objects have per-page dirty tracking */
> - if (WARN_ON(obj->ops != &i915_gem_object_ops))
> + if (WARN_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0))
> return NULL;
>
> page = i915_gem_object_get_page(obj, n);
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 74a4d1714879..7107f2fd38f5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -708,9 +708,10 @@ i915_gem_userptr_dmabuf_export(struct drm_i915_gem_object *obj)
> }
>
> static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
> - .dmabuf_export = i915_gem_userptr_dmabuf_export,
> + .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE,
> .get_pages = i915_gem_userptr_get_pages,
> .put_pages = i915_gem_userptr_put_pages,
> + .dmabuf_export = i915_gem_userptr_dmabuf_export,
> .release = i915_gem_userptr_release,
> };
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] drm/i915: Allow i915_gem_object_get_page() on userptr as well
2016-02-03 19:45 ` Jani Nikula
@ 2016-02-04 12:37 ` Jani Nikula
0 siblings, 0 replies; 12+ messages in thread
From: Jani Nikula @ 2016-02-04 12:37 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Daniel Vetter, drm-intel-fixes
On Wed, 03 Feb 2016, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 29 Jan 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> commit 033908aed5a596f6202c848c6bbc8a40fb1a8490
>> Author: Dave Gordon <david.s.gordon@intel.com>
>> Date: Thu Dec 10 18:51:23 2015 +0000
>>
>> drm/i915: mark GEM object pages dirty when mapped & written by the CPU
>>
>> introduced a check into i915_gem_object_get_dirty_pages() that returned
>> a NULL pointer when called with a bad object, one that was not backed by
>> shmemfs. This WARN was too strict as we can work on all struct page
>> backed objects, and resulted in a WARN + GPF for existing userspace. In
>> order to differentiate the various types of objects, add a new flags field
>> to the i915_gem_object_ops struct to describe their capabilities, with
>> the first flag being whether the object has struct pages.
>>
>> v2: Drop silly const before an integer in the structure declaration.
>>
>> Testcase: igt/gem_userptr_blits/relocations
>> Reported-and-tested-by: Kristian Høgsberg Kristensen <krh@bitplanet.net>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Kristian Høgsberg Kristensen <krh@bitplanet.net>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
>> Reviewed-by: Kristian Høgsberg Kristensen <krh@bitplanet.net>
>
> Cc: drm-intel-fixes@lists.freedesktop.org
>
> to pick for 4.5
Picked up in drm-intel-fixes.
BR,
Jani.
>
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 4 ++++
>> drivers/gpu/drm/i915/i915_gem.c | 3 ++-
>> drivers/gpu/drm/i915/i915_gem_userptr.c | 3 ++-
>> 3 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 905e90f25957..a2d2f08b7515 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2000,6 +2000,9 @@ enum hdmi_force_audio {
>> #define I915_GTT_OFFSET_NONE ((u32)-1)
>>
>> struct drm_i915_gem_object_ops {
>> + unsigned int flags;
>> +#define I915_GEM_OBJECT_HAS_STRUCT_PAGE 0x1
>> +
>> /* Interface between the GEM object and its backing storage.
>> * get_pages() is called once prior to the use of the associated set
>> * of pages before to binding them into the GTT, and put_pages() is
>> @@ -2015,6 +2018,7 @@ struct drm_i915_gem_object_ops {
>> */
>> int (*get_pages)(struct drm_i915_gem_object *);
>> void (*put_pages)(struct drm_i915_gem_object *);
>> +
>> int (*dmabuf_export)(struct drm_i915_gem_object *);
>> void (*release)(struct drm_i915_gem_object *);
>> };
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index a928823507c5..e9b19bca1383 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -4465,6 +4465,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
>> }
>>
>> static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
>> + .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE,
>> .get_pages = i915_gem_object_get_pages_gtt,
>> .put_pages = i915_gem_object_put_pages_gtt,
>> };
>> @@ -5309,7 +5310,7 @@ i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj, int n)
>> struct page *page;
>>
>> /* Only default objects have per-page dirty tracking */
>> - if (WARN_ON(obj->ops != &i915_gem_object_ops))
>> + if (WARN_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0))
>> return NULL;
>>
>> page = i915_gem_object_get_page(obj, n);
>> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
>> index 74a4d1714879..7107f2fd38f5 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
>> @@ -708,9 +708,10 @@ i915_gem_userptr_dmabuf_export(struct drm_i915_gem_object *obj)
>> }
>>
>> static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>> - .dmabuf_export = i915_gem_userptr_dmabuf_export,
>> + .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE,
>> .get_pages = i915_gem_userptr_get_pages,
>> .put_pages = i915_gem_userptr_put_pages,
>> + .dmabuf_export = i915_gem_userptr_dmabuf_export,
>> .release = i915_gem_userptr_release,
>> };
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread