[igt PATCH] gen9: fix gem_render_copy 3d state setup

[igt PATCH] gen9: fix gem_render_copy 3d state setup

[Imre Deak]

Without emitting the default 3DSTATE_WM_DEPTH_STENCIL state the test
will fail.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 lib/rendercopy_gen9.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/rendercopy_gen9.c b/lib/rendercopy_gen9.c
index e20a84f..b7b133c 100644
--- a/lib/rendercopy_gen9.c
+++ b/lib/rendercopy_gen9.c
@@ -821,7 +821,13 @@ gen8_emit_ps(struct intel_batchbuffer *batch, uint32_t kernel) {
 }
 
 static void
-gen8_emit_depth(struct intel_batchbuffer *batch) {
+gen9_emit_depth(struct intel_batchbuffer *batch)
+{
+	OUT_BATCH(GEN8_3DSTATE_WM_DEPTH_STENCIL | (4 - 2));
+	OUT_BATCH(0);
+	OUT_BATCH(0);
+	OUT_BATCH(0);
+
 	OUT_BATCH(GEN7_3DSTATE_DEPTH_BUFFER | (8-2));
 	OUT_BATCH(0);
 	OUT_BATCH(0);
@@ -999,7 +1005,7 @@ void gen9_render_copyfunc(struct intel_batchbuffer *batch,
 	OUT_BATCH(GEN6_3DSTATE_SCISSOR_STATE_POINTERS);
 	OUT_BATCH(scissor_state);
 
-	gen8_emit_depth(batch);
+	gen9_emit_depth(batch);
 
 	gen7_emit_clear(batch);
 
-- 
1.9.1

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

Re: [igt PATCH] gen9: fix gem_render_copy 3d state setup

[Damien Lespiau]

On Thu, Jan 29, 2015 at 12:03:19AM -0800, Imre Deak wrote:
> Without emitting the default 3DSTATE_WM_DEPTH_STENCIL state the test
> will fail.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Question: Wasn't the golden context supposed to paper over those?

-- 
Damien

> ---
>  lib/rendercopy_gen9.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/rendercopy_gen9.c b/lib/rendercopy_gen9.c
> index e20a84f..b7b133c 100644
> --- a/lib/rendercopy_gen9.c
> +++ b/lib/rendercopy_gen9.c
> @@ -821,7 +821,13 @@ gen8_emit_ps(struct intel_batchbuffer *batch, uint32_t kernel) {
>  }
>  
>  static void
> -gen8_emit_depth(struct intel_batchbuffer *batch) {
> +gen9_emit_depth(struct intel_batchbuffer *batch)
> +{
> +	OUT_BATCH(GEN8_3DSTATE_WM_DEPTH_STENCIL | (4 - 2));
> +	OUT_BATCH(0);
> +	OUT_BATCH(0);
> +	OUT_BATCH(0);
> +
>  	OUT_BATCH(GEN7_3DSTATE_DEPTH_BUFFER | (8-2));
>  	OUT_BATCH(0);
>  	OUT_BATCH(0);
> @@ -999,7 +1005,7 @@ void gen9_render_copyfunc(struct intel_batchbuffer *batch,
>  	OUT_BATCH(GEN6_3DSTATE_SCISSOR_STATE_POINTERS);
>  	OUT_BATCH(scissor_state);
>  
> -	gen8_emit_depth(batch);
> +	gen9_emit_depth(batch);
>  
>  	gen7_emit_clear(batch);
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt PATCH] gen9: fix gem_render_copy 3d state setup

[Imre Deak]

On Thu, 2015-01-29 at 10:51 +0000, Damien Lespiau wrote:
> On Thu, Jan 29, 2015 at 12:03:19AM -0800, Imre Deak wrote:
> > Without emitting the default 3DSTATE_WM_DEPTH_STENCIL state the test
> > will fail.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Question: Wasn't the golden context supposed to paper over those?

Perhaps, currently the golden context doesn't include this. I put it
here to match what GEN8 copy_render does, but I agree that eventually we
should move it to the golden context. I noticed that a bunch of other
3DSTATE commands are missing, which could explain the failures I see
with other tests (at least piglit-mesa). I'm experimenting now with an
updated golden context to see if it solves those failures.

--Imre


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

Re: [igt PATCH] gen9: fix gem_render_copy 3d state setup

[Chris Wilson]

On Thu, Jan 29, 2015 at 03:01:50AM -0800, Imre Deak wrote:
> On Thu, 2015-01-29 at 10:51 +0000, Damien Lespiau wrote:
> > On Thu, Jan 29, 2015 at 12:03:19AM -0800, Imre Deak wrote:
> > > Without emitting the default 3DSTATE_WM_DEPTH_STENCIL state the test
> > > will fail.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > 
> > Question: Wasn't the golden context supposed to paper over those?
> 
> Perhaps, currently the golden context doesn't include this.

Today, you cannot rely on the initial contents of the context even with
the golden render state. There is no pristine context, every client is
responsible for configuring the hardware exactly as they intend to use -
at least as regards the untrusted commands (e.g. 3DSTATE).
-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: [igt PATCH] gen9: fix gem_render_copy 3d state setup

[Damien Lespiau]

On Thu, Jan 29, 2015 at 11:12:46AM +0000, Chris Wilson wrote:
> On Thu, Jan 29, 2015 at 03:01:50AM -0800, Imre Deak wrote:
> > On Thu, 2015-01-29 at 10:51 +0000, Damien Lespiau wrote:
> > > On Thu, Jan 29, 2015 at 12:03:19AM -0800, Imre Deak wrote:
> > > > Without emitting the default 3DSTATE_WM_DEPTH_STENCIL state the test
> > > > will fail.
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > 
> > > Question: Wasn't the golden context supposed to paper over those?
> > 
> > Perhaps, currently the golden context doesn't include this.
> 
> Today, you cannot rely on the initial contents of the context even with
> the golden render state. There is no pristine context, every client is
> responsible for configuring the hardware exactly as they intend to use -
> at least as regards the untrusted commands (e.g. 3DSTATE).

Right. Now the question is, do we want to change that and have the
golden context with sane defaults?

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

Re: [igt PATCH] gen9: fix gem_render_copy 3d state setup

[Chris Wilson]

On Thu, Jan 29, 2015 at 11:17:04AM +0000, Damien Lespiau wrote:
> On Thu, Jan 29, 2015 at 11:12:46AM +0000, Chris Wilson wrote:
> > On Thu, Jan 29, 2015 at 03:01:50AM -0800, Imre Deak wrote:
> > > On Thu, 2015-01-29 at 10:51 +0000, Damien Lespiau wrote:
> > > > On Thu, Jan 29, 2015 at 12:03:19AM -0800, Imre Deak wrote:
> > > > > Without emitting the default 3DSTATE_WM_DEPTH_STENCIL state the test
> > > > > will fail.
> > > > > 
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > 
> > > > Question: Wasn't the golden context supposed to paper over those?
> > > 
> > > Perhaps, currently the golden context doesn't include this.
> > 
> > Today, you cannot rely on the initial contents of the context even with
> > the golden render state. There is no pristine context, every client is
> > responsible for configuring the hardware exactly as they intend to use -
> > at least as regards the untrusted commands (e.g. 3DSTATE).
> 
> Right. Now the question is, do we want to change that and have the
> golden context with sane defaults?

You missed the point. The point is that we don't keep initialise every
context from scratch. And there still doesn't seem to be any reason to
be papering over userspace bugs.
-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: [igt PATCH] gen9: fix gem_render_copy 3d state setup

[Damien Lespiau]

On Thu, Jan 29, 2015 at 11:32:46AM +0000, Chris Wilson wrote:
> On Thu, Jan 29, 2015 at 11:17:04AM +0000, Damien Lespiau wrote:
> > On Thu, Jan 29, 2015 at 11:12:46AM +0000, Chris Wilson wrote:
> > > On Thu, Jan 29, 2015 at 03:01:50AM -0800, Imre Deak wrote:
> > > > On Thu, 2015-01-29 at 10:51 +0000, Damien Lespiau wrote:
> > > > > On Thu, Jan 29, 2015 at 12:03:19AM -0800, Imre Deak wrote:
> > > > > > Without emitting the default 3DSTATE_WM_DEPTH_STENCIL state the test
> > > > > > will fail.
> > > > > > 
> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > 
> > > > > Question: Wasn't the golden context supposed to paper over those?
> > > > 
> > > > Perhaps, currently the golden context doesn't include this.
> > > 
> > > Today, you cannot rely on the initial contents of the context even with
> > > the golden render state. There is no pristine context, every client is
> > > responsible for configuring the hardware exactly as they intend to use -
> > > at least as regards the untrusted commands (e.g. 3DSTATE).
> > 
> > Right. Now the question is, do we want to change that and have the
> > golden context with sane defaults?
> 
> You missed the point. The point is that we don't keep initialise every
> context from scratch. And there still doesn't seem to be any reason to
> be papering over userspace bugs.

That's because I still think the end of the journey is a fully
initialized golden context image + copy of that context on context
creation.

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

Re: [igt PATCH] gen9: fix gem_render_copy 3d state setup

[Mika Kuoppala]

Damien Lespiau <damien.lespiau@intel.com> writes:

> On Thu, Jan 29, 2015 at 11:32:46AM +0000, Chris Wilson wrote:
>> On Thu, Jan 29, 2015 at 11:17:04AM +0000, Damien Lespiau wrote:
>> > On Thu, Jan 29, 2015 at 11:12:46AM +0000, Chris Wilson wrote:
>> > > On Thu, Jan 29, 2015 at 03:01:50AM -0800, Imre Deak wrote:
>> > > > On Thu, 2015-01-29 at 10:51 +0000, Damien Lespiau wrote:
>> > > > > On Thu, Jan 29, 2015 at 12:03:19AM -0800, Imre Deak wrote:
>> > > > > > Without emitting the default 3DSTATE_WM_DEPTH_STENCIL state the test
>> > > > > > will fail.
>> > > > > > 
>> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> > > > > 
>> > > > > Question: Wasn't the golden context supposed to paper over those?
>> > > > 
>> > > > Perhaps, currently the golden context doesn't include this.
>> > > 
>> > > Today, you cannot rely on the initial contents of the context even with
>> > > the golden render state. There is no pristine context, every client is
>> > > responsible for configuring the hardware exactly as they intend to use -
>> > > at least as regards the untrusted commands (e.g. 3DSTATE).
>> > 
>> > Right. Now the question is, do we want to change that and have the
>> > golden context with sane defaults?
>> 
>> You missed the point. The point is that we don't keep initialise every
>> context from scratch. And there still doesn't seem to be any reason to
>> be papering over userspace bugs.
>
> That's because I still think the end of the journey is a fully
> initialized golden context image + copy of that context on context
> creation.
>

For me the end journey has looked like this:

For first (default) context:
- run golden/null batch
- emit workarounds
- take a master copy from ctx_obj

Then:
 - copy from master (with gpu or cpu) for every new fd/ctx

-Mika

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

Re: [igt PATCH] gen9: fix gem_render_copy 3d state setup

[Daniel Vetter]

On Thu, Jan 29, 2015 at 04:15:55PM +0200, Mika Kuoppala wrote:
> Damien Lespiau <damien.lespiau@intel.com> writes:
> 
> > On Thu, Jan 29, 2015 at 11:32:46AM +0000, Chris Wilson wrote:
> >> On Thu, Jan 29, 2015 at 11:17:04AM +0000, Damien Lespiau wrote:
> >> > On Thu, Jan 29, 2015 at 11:12:46AM +0000, Chris Wilson wrote:
> >> > > On Thu, Jan 29, 2015 at 03:01:50AM -0800, Imre Deak wrote:
> >> > > > On Thu, 2015-01-29 at 10:51 +0000, Damien Lespiau wrote:
> >> > > > > On Thu, Jan 29, 2015 at 12:03:19AM -0800, Imre Deak wrote:
> >> > > > > > Without emitting the default 3DSTATE_WM_DEPTH_STENCIL state the test
> >> > > > > > will fail.
> >> > > > > > 
> >> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> >> > > > > 
> >> > > > > Question: Wasn't the golden context supposed to paper over those?
> >> > > > 
> >> > > > Perhaps, currently the golden context doesn't include this.
> >> > > 
> >> > > Today, you cannot rely on the initial contents of the context even with
> >> > > the golden render state. There is no pristine context, every client is
> >> > > responsible for configuring the hardware exactly as they intend to use -
> >> > > at least as regards the untrusted commands (e.g. 3DSTATE).
> >> > 
> >> > Right. Now the question is, do we want to change that and have the
> >> > golden context with sane defaults?
> >> 
> >> You missed the point. The point is that we don't keep initialise every
> >> context from scratch. And there still doesn't seem to be any reason to
> >> be papering over userspace bugs.
> >
> > That's because I still think the end of the journey is a fully
> > initialized golden context image + copy of that context on context
> > creation.
> >
> 
> For me the end journey has looked like this:
> 
> For first (default) context:
> - run golden/null batch
> - emit workarounds
> - take a master copy from ctx_obj
> 
> Then:
>  - copy from master (with gpu or cpu) for every new fd/ctx

Can we just have it? I think we only need to do this here:


diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 8603bf48d3ee..e20cfa1ac88c 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -633,7 +633,7 @@ static int do_switch(struct intel_engine_cs *ring,
 			goto unpin_out;
 	}
 
-	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
+	if (!to->legacy_hw_ctx.initialized)
 		hw_flags |= MI_RESTORE_INHIBIT;
 
 	ret = mi_set_context(ring, to, hw_flags);


Chris doesn't like it though for the perf implications. I've sent this out
a while ago but never got around to doing the microbenchmark Chris
requested. So testing (and if it works, volunteers for the microbenchmark)
highly welcome ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx