* [PATCH v3 2/3] drm/i915: Enable resource streamer bits on MI_BATCH_BUFFER_START
2015-05-18 8:31 [PATCH v3 1/3] drm/i915: Expose I915_EXEC_RESOURCE_STREAMER flag Abdiel Janulgue
@ 2015-05-18 8:31 ` Abdiel Janulgue
2015-05-18 8:31 ` [PATCH v3 3/3] drm/i915: Enable Resource Streamer state save/restore in HSW Abdiel Janulgue
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Abdiel Janulgue @ 2015-05-18 8:31 UTC (permalink / raw)
To: intel-gfx
Adds support for executing the resource streamer on BDW and HSW
v2: Add support for Execlists (Minu Mathai <minu.mathai@intel.com>)
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/intel_lrc.c | 3 ++-
drivers/gpu/drm/i915/intel_ringbuffer.c | 6 ++++--
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b522eb6..238bb25 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -356,6 +356,7 @@
#define MI_BATCH_BUFFER_START MI_INSTR(0x31, 0)
#define MI_BATCH_GTT (2<<6) /* aliased with (1<<7) on gen4 */
#define MI_BATCH_BUFFER_START_GEN8 MI_INSTR(0x31, 1)
+#define MI_BATCH_RESOURCE_STREAMER (1<<10)
#define MI_PREDICATE_SRC0 (0x2400)
#define MI_PREDICATE_SRC1 (0x2408)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index fcb074b..d523494 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1172,7 +1172,8 @@ static int gen8_emit_bb_start(struct intel_ringbuffer *ringbuf,
return ret;
/* FIXME(BDW): Address space and security selectors. */
- intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8));
+ intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8) |
+ (I915_DISPATCH_RS ? MI_BATCH_RESOURCE_STREAMER : 0));
intel_logical_ring_emit(ringbuf, lower_32_bits(offset));
intel_logical_ring_emit(ringbuf, upper_32_bits(offset));
intel_logical_ring_emit(ringbuf, MI_NOOP);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 441e250..9045144 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2385,7 +2385,8 @@ gen8_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
return ret;
/* FIXME(BDW): Address space and security selectors. */
- intel_ring_emit(ring, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8));
+ intel_ring_emit(ring, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8) |
+ (dispatch_flags & I915_DISPATCH_RS ? MI_BATCH_RESOURCE_STREAMER : 0));
intel_ring_emit(ring, lower_32_bits(offset));
intel_ring_emit(ring, upper_32_bits(offset));
intel_ring_emit(ring, MI_NOOP);
@@ -2408,7 +2409,8 @@ hsw_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
intel_ring_emit(ring,
MI_BATCH_BUFFER_START |
(dispatch_flags & I915_DISPATCH_SECURE ?
- 0 : MI_BATCH_PPGTT_HSW | MI_BATCH_NON_SECURE_HSW));
+ 0 : MI_BATCH_PPGTT_HSW | MI_BATCH_NON_SECURE_HSW) |
+ (dispatch_flags & I915_DISPATCH_RS ? MI_BATCH_RESOURCE_STREAMER : 0));
/* bit0-7 is the length on GEN6+ */
intel_ring_emit(ring, offset);
intel_ring_advance(ring);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v3 3/3] drm/i915: Enable Resource Streamer state save/restore in HSW
2015-05-18 8:31 [PATCH v3 1/3] drm/i915: Expose I915_EXEC_RESOURCE_STREAMER flag Abdiel Janulgue
2015-05-18 8:31 ` [PATCH v3 2/3] drm/i915: Enable resource streamer bits on MI_BATCH_BUFFER_START Abdiel Janulgue
@ 2015-05-18 8:31 ` Abdiel Janulgue
2015-05-18 15:36 ` Ville Syrjälä
2015-05-18 9:01 ` [PATCH v3 1/3] drm/i915: Expose I915_EXEC_RESOURCE_STREAMER flag Daniel Vetter
2015-05-18 14:55 ` Chris Wilson
3 siblings, 1 reply; 14+ messages in thread
From: Abdiel Janulgue @ 2015-05-18 8:31 UTC (permalink / raw)
To: intel-gfx
Also clarify comments on context size that the extra state for
Resource Streamer is included.
Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
drivers/gpu/drm/i915/i915_reg.h | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f3e84c4..1db107a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -509,7 +509,7 @@ mi_set_context(struct intel_engine_cs *ring,
}
/* These flags are for resource streamer on HSW+ */
- if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
+ if (IS_HASWELL(ring->dev))
flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 238bb25..3db0596 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2498,7 +2498,8 @@ enum skl_disp_power_wells {
* valid. Now, docs explain in dwords what is in the context object. The full
* size is 70720 bytes, however, the power context and execlist context will
* never be saved (power context is stored elsewhere, and execlists don't work
- * on HSW) - so the final size is 66944 bytes, which rounds to 17 pages.
+ * on HSW) - so the final size, including the extra state required for the
+ * Resource Streamer, is 66944 bytes, which rounds to 17 pages.
*/
#define HSW_CXT_TOTAL_SIZE (17 * PAGE_SIZE)
/* Same as Haswell, but 72064 bytes now. */
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 3/3] drm/i915: Enable Resource Streamer state save/restore in HSW
2015-05-18 8:31 ` [PATCH v3 3/3] drm/i915: Enable Resource Streamer state save/restore in HSW Abdiel Janulgue
@ 2015-05-18 15:36 ` Ville Syrjälä
2015-05-18 15:41 ` Chris Wilson
0 siblings, 1 reply; 14+ messages in thread
From: Ville Syrjälä @ 2015-05-18 15:36 UTC (permalink / raw)
To: Abdiel Janulgue; +Cc: intel-gfx
On Mon, May 18, 2015 at 11:31:56AM +0300, Abdiel Janulgue wrote:
> Also clarify comments on context size that the extra state for
> Resource Streamer is included.
>
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
> drivers/gpu/drm/i915/i915_reg.h | 3 ++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index f3e84c4..1db107a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -509,7 +509,7 @@ mi_set_context(struct intel_engine_cs *ring,
> }
>
> /* These flags are for resource streamer on HSW+ */
> - if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
> + if (IS_HASWELL(ring->dev))
> flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
I don't get it. Previously we told the hardware to save the extended
context on !hsw, and now we don't. That doesn't seem correct to me.
>
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 238bb25..3db0596 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2498,7 +2498,8 @@ enum skl_disp_power_wells {
> * valid. Now, docs explain in dwords what is in the context object. The full
> * size is 70720 bytes, however, the power context and execlist context will
> * never be saved (power context is stored elsewhere, and execlists don't work
> - * on HSW) - so the final size is 66944 bytes, which rounds to 17 pages.
> + * on HSW) - so the final size, including the extra state required for the
> + * Resource Streamer, is 66944 bytes, which rounds to 17 pages.
> */
> #define HSW_CXT_TOTAL_SIZE (17 * PAGE_SIZE)
> /* Same as Haswell, but 72064 bytes now. */
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 3/3] drm/i915: Enable Resource Streamer state save/restore in HSW
2015-05-18 15:36 ` Ville Syrjälä
@ 2015-05-18 15:41 ` Chris Wilson
2015-05-18 16:07 ` Ville Syrjälä
0 siblings, 1 reply; 14+ messages in thread
From: Chris Wilson @ 2015-05-18 15:41 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Mon, May 18, 2015 at 06:36:18PM +0300, Ville Syrjälä wrote:
> On Mon, May 18, 2015 at 11:31:56AM +0300, Abdiel Janulgue wrote:
> > Also clarify comments on context size that the extra state for
> > Resource Streamer is included.
> >
> > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
> > drivers/gpu/drm/i915/i915_reg.h | 3 ++-
> > 2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index f3e84c4..1db107a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -509,7 +509,7 @@ mi_set_context(struct intel_engine_cs *ring,
> > }
> >
> > /* These flags are for resource streamer on HSW+ */
> > - if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
> > + if (IS_HASWELL(ring->dev))
> > flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
>
> I don't get it. Previously we told the hardware to save the extended
> context on !hsw, and now we don't. That doesn't seem correct to me.
We don't use the extended state elsewhere. I'd always been dubious of
the origins/intentions of this line of code since it claims only to be
for enabling RS on HSW...
i.e. commit e80f14b6d36e3e07111cf2ab084ef8dd5d015ce2
Author: Ben Widawsky <benjamin.widawsky@intel.com>
Date: Mon Aug 18 10:35:28 2014 -0700
drm/i915: Don't save/restore RS when not used
was backwards.
-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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 3/3] drm/i915: Enable Resource Streamer state save/restore in HSW
2015-05-18 15:41 ` Chris Wilson
@ 2015-05-18 16:07 ` Ville Syrjälä
2015-05-19 6:58 ` Abdiel Janulgue
0 siblings, 1 reply; 14+ messages in thread
From: Ville Syrjälä @ 2015-05-18 16:07 UTC (permalink / raw)
To: Chris Wilson, Abdiel Janulgue, intel-gfx
On Mon, May 18, 2015 at 04:41:51PM +0100, Chris Wilson wrote:
> On Mon, May 18, 2015 at 06:36:18PM +0300, Ville Syrjälä wrote:
> > On Mon, May 18, 2015 at 11:31:56AM +0300, Abdiel Janulgue wrote:
> > > Also clarify comments on context size that the extra state for
> > > Resource Streamer is included.
> > >
> > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
> > > drivers/gpu/drm/i915/i915_reg.h | 3 ++-
> > > 2 files changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > > index f3e84c4..1db107a 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > @@ -509,7 +509,7 @@ mi_set_context(struct intel_engine_cs *ring,
> > > }
> > >
> > > /* These flags are for resource streamer on HSW+ */
> > > - if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
> > > + if (IS_HASWELL(ring->dev))
> > > flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> >
> > I don't get it. Previously we told the hardware to save the extended
> > context on !hsw, and now we don't. That doesn't seem correct to me.
>
> We don't use the extended state elsewhere.
Umm. On SNB at least 3DSTATE_POLY_STIPPLE_PATTERN seems to be part of
the extended state, and on IVB/VLV SOL state is there. Mesa uses all of
that.
> I'd always been dubious of
> the origins/intentions of this line of code since it claims only to be
> for enabling RS on HSW...
>
> i.e. commit e80f14b6d36e3e07111cf2ab084ef8dd5d015ce2
> Author: Ben Widawsky <benjamin.widawsky@intel.com>
> Date: Mon Aug 18 10:35:28 2014 -0700
>
> drm/i915: Don't save/restore RS when not used
>
> was backwards.
? It did exactly what it said, ie. avoid setting the RS save/restore
bits on HSW+ while leaving the ext save/restore enabled on older
platforms.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: Enable Resource Streamer state save/restore in HSW
2015-05-18 16:07 ` Ville Syrjälä
@ 2015-05-19 6:58 ` Abdiel Janulgue
2015-05-19 8:26 ` Daniel Vetter
0 siblings, 1 reply; 14+ messages in thread
From: Abdiel Janulgue @ 2015-05-19 6:58 UTC (permalink / raw)
To: Ville Syrjälä, Chris Wilson, intel-gfx
On 05/18/2015 07:07 PM, Ville Syrjälä wrote:
> On Mon, May 18, 2015 at 04:41:51PM +0100, Chris Wilson wrote:
>> On Mon, May 18, 2015 at 06:36:18PM +0300, Ville Syrjälä wrote:
>>> On Mon, May 18, 2015 at 11:31:56AM +0300, Abdiel Janulgue wrote:
>>>> Also clarify comments on context size that the extra state for
>>>> Resource Streamer is included.
>>>>
>>>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
>>>> drivers/gpu/drm/i915/i915_reg.h | 3 ++-
>>>> 2 files changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
>>>> index f3e84c4..1db107a 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>>>> @@ -509,7 +509,7 @@ mi_set_context(struct intel_engine_cs *ring,
>>>> }
>>>>
>>>> /* These flags are for resource streamer on HSW+ */
>>>> - if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
>>>> + if (IS_HASWELL(ring->dev))
>>>> flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
>>>
>>> I don't get it. Previously we told the hardware to save the extended
>>> context on !hsw, and now we don't. That doesn't seem correct to me.
>>
>> We don't use the extended state elsewhere.
>
> Umm. On SNB at least 3DSTATE_POLY_STIPPLE_PATTERN seems to be part of
> the extended state, and on IVB/VLV SOL state is there. Mesa uses all of
> that.
>
>> I'd always been dubious of
>> the origins/intentions of this line of code since it claims only to be
>> for enabling RS on HSW...
>>
>> i.e. commit e80f14b6d36e3e07111cf2ab084ef8dd5d015ce2
>> Author: Ben Widawsky <benjamin.widawsky@intel.com>
>> Date: Mon Aug 18 10:35:28 2014 -0700
>>
>> drm/i915: Don't save/restore RS when not used
>>
>> was backwards.
>
> ? It did exactly what it said, ie. avoid setting the RS save/restore
> bits on HSW+ while leaving the ext save/restore enabled on older
> platforms.
>
Another option is to enable extended state save restore for both HSW and
the older platforms so we would get both features (RS save/restore and
Extended State Save enable)?
Seems the reason for this confusion is that the we have the exact same
bit positions in MI_SET_CONTEXT but it got renamed starting from HSW and up.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: Enable Resource Streamer state save/restore in HSW
2015-05-19 6:58 ` Abdiel Janulgue
@ 2015-05-19 8:26 ` Daniel Vetter
2015-05-19 8:31 ` Abdiel Janulgue
0 siblings, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2015-05-19 8:26 UTC (permalink / raw)
To: Abdiel Janulgue; +Cc: intel-gfx
On Tue, May 19, 2015 at 09:58:52AM +0300, Abdiel Janulgue wrote:
>
>
> On 05/18/2015 07:07 PM, Ville Syrjälä wrote:
> > On Mon, May 18, 2015 at 04:41:51PM +0100, Chris Wilson wrote:
> >> On Mon, May 18, 2015 at 06:36:18PM +0300, Ville Syrjälä wrote:
> >>> On Mon, May 18, 2015 at 11:31:56AM +0300, Abdiel Janulgue wrote:
> >>>> Also clarify comments on context size that the extra state for
> >>>> Resource Streamer is included.
> >>>>
> >>>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> >>>> ---
> >>>> drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
> >>>> drivers/gpu/drm/i915/i915_reg.h | 3 ++-
> >>>> 2 files changed, 3 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> >>>> index f3e84c4..1db107a 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> >>>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> >>>> @@ -509,7 +509,7 @@ mi_set_context(struct intel_engine_cs *ring,
> >>>> }
> >>>>
> >>>> /* These flags are for resource streamer on HSW+ */
> >>>> - if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
> >>>> + if (IS_HASWELL(ring->dev))
> >>>> flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> >>>
> >>> I don't get it. Previously we told the hardware to save the extended
> >>> context on !hsw, and now we don't. That doesn't seem correct to me.
> >>
> >> We don't use the extended state elsewhere.
> >
> > Umm. On SNB at least 3DSTATE_POLY_STIPPLE_PATTERN seems to be part of
> > the extended state, and on IVB/VLV SOL state is there. Mesa uses all of
> > that.
> >
> >> I'd always been dubious of
> >> the origins/intentions of this line of code since it claims only to be
> >> for enabling RS on HSW...
> >>
> >> i.e. commit e80f14b6d36e3e07111cf2ab084ef8dd5d015ce2
> >> Author: Ben Widawsky <benjamin.widawsky@intel.com>
> >> Date: Mon Aug 18 10:35:28 2014 -0700
> >>
> >> drm/i915: Don't save/restore RS when not used
> >>
> >> was backwards.
> >
> > ? It did exactly what it said, ie. avoid setting the RS save/restore
> > bits on HSW+ while leaving the ext save/restore enabled on older
> > platforms.
> >
>
> Another option is to enable extended state save restore for both HSW and
> the older platforms so we would get both features (RS save/restore and
> Extended State Save enable)?
>
> Seems the reason for this confusion is that the we have the exact same
> bit positions in MI_SET_CONTEXT but it got renamed starting from HSW and up.
If the bit has been renamed it might be good to add at least a new #define
with a _HSw suffix and better name, just to make it clear what's going on.
gcc will see through this and fold down the different conditions to just
one.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: Enable Resource Streamer state save/restore in HSW
2015-05-19 8:26 ` Daniel Vetter
@ 2015-05-19 8:31 ` Abdiel Janulgue
0 siblings, 0 replies; 14+ messages in thread
From: Abdiel Janulgue @ 2015-05-19 8:31 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On 05/19/2015 11:26 AM, Daniel Vetter wrote:
> On Tue, May 19, 2015 at 09:58:52AM +0300, Abdiel Janulgue wrote:
>>
>>
>> On 05/18/2015 07:07 PM, Ville Syrjälä wrote:
>>> On Mon, May 18, 2015 at 04:41:51PM +0100, Chris Wilson wrote:
>>>> On Mon, May 18, 2015 at 06:36:18PM +0300, Ville Syrjälä wrote:
>>>>> On Mon, May 18, 2015 at 11:31:56AM +0300, Abdiel Janulgue wrote:
>>>>>> Also clarify comments on context size that the extra state for
>>>>>> Resource Streamer is included.
>>>>>>
>>>>>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
>>>>>> drivers/gpu/drm/i915/i915_reg.h | 3 ++-
>>>>>> 2 files changed, 3 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
>>>>>> index f3e84c4..1db107a 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>>>>>> @@ -509,7 +509,7 @@ mi_set_context(struct intel_engine_cs *ring,
>>>>>> }
>>>>>>
>>>>>> /* These flags are for resource streamer on HSW+ */
>>>>>> - if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
>>>>>> + if (IS_HASWELL(ring->dev))
>>>>>> flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
>>>>>
>>>>> I don't get it. Previously we told the hardware to save the extended
>>>>> context on !hsw, and now we don't. That doesn't seem correct to me.
>>>>
>>>> We don't use the extended state elsewhere.
>>>
>>> Umm. On SNB at least 3DSTATE_POLY_STIPPLE_PATTERN seems to be part of
>>> the extended state, and on IVB/VLV SOL state is there. Mesa uses all of
>>> that.
>>>
>>>> I'd always been dubious of
>>>> the origins/intentions of this line of code since it claims only to be
>>>> for enabling RS on HSW...
>>>>
>>>> i.e. commit e80f14b6d36e3e07111cf2ab084ef8dd5d015ce2
>>>> Author: Ben Widawsky <benjamin.widawsky@intel.com>
>>>> Date: Mon Aug 18 10:35:28 2014 -0700
>>>>
>>>> drm/i915: Don't save/restore RS when not used
>>>>
>>>> was backwards.
>>>
>>> ? It did exactly what it said, ie. avoid setting the RS save/restore
>>> bits on HSW+ while leaving the ext save/restore enabled on older
>>> platforms.
>>>
>>
>> Another option is to enable extended state save restore for both HSW and
>> the older platforms so we would get both features (RS save/restore and
>> Extended State Save enable)?
>>
>> Seems the reason for this confusion is that the we have the exact same
>> bit positions in MI_SET_CONTEXT but it got renamed starting from HSW and up.
>
> If the bit has been renamed it might be good to add at least a new #define
> with a _HSw suffix and better name, just to make it clear what's going on.
> gcc will see through this and fold down the different conditions to just
> one.
I'll do that in the next revision and also add the igt tag you require.
In the meantime, please check the igt approach I sent is the correct one.
Thanks!
-Abdiel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: Expose I915_EXEC_RESOURCE_STREAMER flag
2015-05-18 8:31 [PATCH v3 1/3] drm/i915: Expose I915_EXEC_RESOURCE_STREAMER flag Abdiel Janulgue
2015-05-18 8:31 ` [PATCH v3 2/3] drm/i915: Enable resource streamer bits on MI_BATCH_BUFFER_START Abdiel Janulgue
2015-05-18 8:31 ` [PATCH v3 3/3] drm/i915: Enable Resource Streamer state save/restore in HSW Abdiel Janulgue
@ 2015-05-18 9:01 ` Daniel Vetter
2015-05-18 10:52 ` Abdiel Janulgue
2015-05-18 14:55 ` Chris Wilson
3 siblings, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2015-05-18 9:01 UTC (permalink / raw)
To: Abdiel Janulgue; +Cc: intel-gfx
On Mon, May 18, 2015 at 11:31:54AM +0300, Abdiel Janulgue wrote:
> Ensures that the batch buffer is executed by the resource streamer
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
Maybe I missed them, but we also need a patch to update gem_exec_params
from igt. At least the invalid-flag subtest should fail with this
applied. Also please add a Testcase: tag once you've added the testcase
for this new flag.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 15 +++++++++++++++
> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
> include/uapi/drm/i915_drm.h | 7 ++++++-
> 3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index a3190e79..8a0abbb 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1485,6 +1485,21 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> return -EINVAL;
> }
>
> + if (args->flags & I915_EXEC_RESOURCE_STREAMER) {
> + if (!IS_HASWELL(dev) && INTEL_INFO(dev)->gen < 8) {
> + DRM_DEBUG("RS is only allowed for Haswell, Gen8 "
> + "and above\n");
> + return -EINVAL;
> + }
> + if (ring->id != RCS) {
> + DRM_DEBUG("RS is not available on %s\n",
> + ring->name);
> + return -EINVAL;
> + }
> +
> + dispatch_flags |= I915_DISPATCH_RS;
> + }
> +
> intel_runtime_pm_get(dev_priv);
>
> ret = i915_mutex_lock_interruptible(dev);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index c761fe0..3521bc0 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -167,6 +167,7 @@ struct intel_engine_cs {
> unsigned dispatch_flags);
> #define I915_DISPATCH_SECURE 0x1
> #define I915_DISPATCH_PINNED 0x2
> +#define I915_DISPATCH_RS 0x4
> void (*cleanup)(struct intel_engine_cs *ring);
>
> /* GEN8 signal/wait table - never trust comments!
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 551b673..a4c1a5c 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -760,7 +760,12 @@ struct drm_i915_gem_execbuffer2 {
> #define I915_EXEC_BSD_RING1 (1<<13)
> #define I915_EXEC_BSD_RING2 (2<<13)
>
> -#define __I915_EXEC_UNKNOWN_FLAGS -(1<<15)
> +/** Tell the kernel that the batchbuffer is processed by
> + * the resource streamer.
> + */
> +#define I915_EXEC_RESOURCE_STREAMER (1<<16)
> +
> +#define __I915_EXEC_UNKNOWN_FLAGS -(I915_EXEC_RESOURCE_STREAMER <<1)
>
> #define I915_EXEC_CONTEXT_ID_MASK (0xffffffff)
> #define i915_execbuffer2_set_context_id(eb2, context) \
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/3] drm/i915: Expose I915_EXEC_RESOURCE_STREAMER flag
2015-05-18 9:01 ` [PATCH v3 1/3] drm/i915: Expose I915_EXEC_RESOURCE_STREAMER flag Daniel Vetter
@ 2015-05-18 10:52 ` Abdiel Janulgue
2015-05-18 14:51 ` Daniel Vetter
0 siblings, 1 reply; 14+ messages in thread
From: Abdiel Janulgue @ 2015-05-18 10:52 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On 05/18/2015 12:01 PM, Daniel Vetter wrote:
> On Mon, May 18, 2015 at 11:31:54AM +0300, Abdiel Janulgue wrote:
>> Ensures that the batch buffer is executed by the resource streamer
>>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
>
> Maybe I missed them, but we also need a patch to update gem_exec_params
> from igt. At least the invalid-flag subtest should fail with this
> applied. Also please add a Testcase: tag once you've added the testcase
> for this new flag.
I'm not sure what you mean here. When I run unmodified gem_exec_params
with this resource streamer patches applied. I get this results, at
least on the invalid-flag section:
Subtest invalid-flag: SUCCESS (0.000s)
Is this supposed to fail?
-abdiel
>
>> ---
>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 15 +++++++++++++++
>> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
>> include/uapi/drm/i915_drm.h | 7 ++++++-
>> 3 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index a3190e79..8a0abbb 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -1485,6 +1485,21 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>> return -EINVAL;
>> }
>>
>> + if (args->flags & I915_EXEC_RESOURCE_STREAMER) {
>> + if (!IS_HASWELL(dev) && INTEL_INFO(dev)->gen < 8) {
>> + DRM_DEBUG("RS is only allowed for Haswell, Gen8 "
>> + "and above\n");
>> + return -EINVAL;
>> + }
>> + if (ring->id != RCS) {
>> + DRM_DEBUG("RS is not available on %s\n",
>> + ring->name);
>> + return -EINVAL;
>> + }
>> +
>> + dispatch_flags |= I915_DISPATCH_RS;
>> + }
>> +
>> intel_runtime_pm_get(dev_priv);
>>
>> ret = i915_mutex_lock_interruptible(dev);
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index c761fe0..3521bc0 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -167,6 +167,7 @@ struct intel_engine_cs {
>> unsigned dispatch_flags);
>> #define I915_DISPATCH_SECURE 0x1
>> #define I915_DISPATCH_PINNED 0x2
>> +#define I915_DISPATCH_RS 0x4
>> void (*cleanup)(struct intel_engine_cs *ring);
>>
>> /* GEN8 signal/wait table - never trust comments!
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index 551b673..a4c1a5c 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -760,7 +760,12 @@ struct drm_i915_gem_execbuffer2 {
>> #define I915_EXEC_BSD_RING1 (1<<13)
>> #define I915_EXEC_BSD_RING2 (2<<13)
>>
>> -#define __I915_EXEC_UNKNOWN_FLAGS -(1<<15)
>> +/** Tell the kernel that the batchbuffer is processed by
>> + * the resource streamer.
>> + */
>> +#define I915_EXEC_RESOURCE_STREAMER (1<<16)
>> +
>> +#define __I915_EXEC_UNKNOWN_FLAGS -(I915_EXEC_RESOURCE_STREAMER <<1)
>>
>> #define I915_EXEC_CONTEXT_ID_MASK (0xffffffff)
>> #define i915_execbuffer2_set_context_id(eb2, context) \
>> --
>> 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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/3] drm/i915: Expose I915_EXEC_RESOURCE_STREAMER flag
2015-05-18 10:52 ` Abdiel Janulgue
@ 2015-05-18 14:51 ` Daniel Vetter
0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2015-05-18 14:51 UTC (permalink / raw)
To: Abdiel Janulgue; +Cc: intel-gfx
On Mon, May 18, 2015 at 01:52:12PM +0300, Abdiel Janulgue wrote:
>
>
> On 05/18/2015 12:01 PM, Daniel Vetter wrote:
> > On Mon, May 18, 2015 at 11:31:54AM +0300, Abdiel Janulgue wrote:
> >> Ensures that the batch buffer is executed by the resource streamer
> >>
> >> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> >
> > Maybe I missed them, but we also need a patch to update gem_exec_params
> > from igt. At least the invalid-flag subtest should fail with this
> > applied. Also please add a Testcase: tag once you've added the testcase
> > for this new flag.
>
> I'm not sure what you mean here. When I run unmodified gem_exec_params
> with this resource streamer patches applied. I get this results, at
> least on the invalid-flag section:
>
> Subtest invalid-flag: SUCCESS (0.000s)
>
> Is this supposed to fail?
Yup, but because it wasn't updated for the bsd mask feature and
accidentally still passes there it still passes when you add yet another
flag.
Anyway that one needs to be updated to hopefully make sure we'll catch
bit17. And you need flag checks to make sure the new RS flag doesn't go
through on !RCS and pre-hsw.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: Expose I915_EXEC_RESOURCE_STREAMER flag
2015-05-18 8:31 [PATCH v3 1/3] drm/i915: Expose I915_EXEC_RESOURCE_STREAMER flag Abdiel Janulgue
` (2 preceding siblings ...)
2015-05-18 9:01 ` [PATCH v3 1/3] drm/i915: Expose I915_EXEC_RESOURCE_STREAMER flag Daniel Vetter
@ 2015-05-18 14:55 ` Chris Wilson
2015-05-19 8:36 ` Abdiel Janulgue
3 siblings, 1 reply; 14+ messages in thread
From: Chris Wilson @ 2015-05-18 14:55 UTC (permalink / raw)
To: Abdiel Janulgue; +Cc: intel-gfx
On Mon, May 18, 2015 at 11:31:54AM +0300, Abdiel Janulgue wrote:
> Ensures that the batch buffer is executed by the resource streamer
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
1-3:
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Now all you have to do is satisfy Daniel with a few igt.
-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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/3] drm/i915: Expose I915_EXEC_RESOURCE_STREAMER flag
2015-05-18 14:55 ` Chris Wilson
@ 2015-05-19 8:36 ` Abdiel Janulgue
0 siblings, 0 replies; 14+ messages in thread
From: Abdiel Janulgue @ 2015-05-19 8:36 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 05/18/2015 05:55 PM, Chris Wilson wrote:
> On Mon, May 18, 2015 at 11:31:54AM +0300, Abdiel Janulgue wrote:
>> Ensures that the batch buffer is executed by the resource streamer
>>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
>
> 1-3:
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Now all you have to do is satisfy Daniel with a few igt.
> -Chris
>
Thanks for the review! :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread