* [PATCH] drm/i915: Add support for resetting the SO write pointers on gen7.
@ 2011-12-30 1:52 Eric Anholt
2012-01-02 15:04 ` Eugeni Dodonov
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Eric Anholt @ 2011-12-30 1:52 UTC (permalink / raw)
To: intel-gfx
These registers are automatically incremented by the hardware during
transform feedback to track where the next streamed vertex output
should go. Unlike the previous generation, which had a packet for
setting the corresponding registers to a defined value, gen7 only has
MI_LOAD_REGISTER_IMM to do so. That's a secure packet (since it loads
an arbitrary register), so we need to do it from the kernel, and it
needs to be settable atomically with the batchbuffer execution so that
two clients doing transform feedback don't stomp on each others'
state.
Instead of building a more complicated interface involcing setting the
registers to a specific value, just set them to 0 when asked and
userland can tweak its pointers accordingly.
Signed-off-by: Eric Anholt <eric@anholt.net>
---
drivers/gpu/drm/i915/i915_dma.c | 3 ++
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 31 ++++++++++++++++++++++++++++
drivers/gpu/drm/i915/i915_reg.h | 6 +++++
include/drm/i915_drm.h | 4 +++
4 files changed, 44 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index a9ae374..1add685 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -781,6 +781,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
case I915_PARAM_HAS_RELAXED_DELTA:
value = 1;
break;
+ case I915_PARAM_HAS_GEN7_SOL_RESET:
+ value = 1;
+ break;
default:
DRM_DEBUG_DRIVER("Unknown parameter %d\n",
param->param);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a4e4f3a..126144a 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -971,6 +971,31 @@ i915_gem_execbuffer_retire_commands(struct drm_device *dev,
}
static int
+i915_reset_gen7_sol_offsets(struct drm_device *dev,
+ struct intel_ring_buffer *ring)
+{
+ drm_i915_private_t *dev_priv = dev->dev_private;
+ int ret, i;
+
+ if (!IS_GEN7(dev) || ring != &dev_priv->ring[RCS])
+ return 0;
+
+ ret = intel_ring_begin(ring, 4 * 3);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < 4; i++) {
+ intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
+ intel_ring_emit(ring, GEN7_SO_WRITE_OFFSET(i));
+ intel_ring_emit(ring, 0);
+ }
+
+ intel_ring_advance(ring);
+
+ return 0;
+}
+
+static int
i915_gem_do_execbuffer(struct drm_device *dev, void *data,
struct drm_file *file,
struct drm_i915_gem_execbuffer2 *args,
@@ -1182,6 +1207,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
goto err;
}
+ if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
+ ret = i915_reset_gen7_sol_offsets(dev, ring);
+ if (ret)
+ goto err;
+ }
+
trace_i915_gem_ring_dispatch(ring, seqno);
exec_start = batch_obj->gtt_offset + args->batch_start_offset;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9d15474..54a18a4 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3583,4 +3583,10 @@
#define GEN7_AUD_CNTRL_ST_A 0xE50B4
#define GEN7_AUD_CNTRL_ST2 0xE50C0
+/* These are the 4 32-bit write offset registers for each stream
+ * output buffer. It determines the offset from the
+ * 3DSTATE_SO_BUFFERs that the next streamed vertex output goes to.
+ */
+#define GEN7_SO_WRITE_OFFSET(n) (0x5280 + (n) * 4)
+
#endif /* _I915_REG_H_ */
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index 28c0d11..5bb6a6a 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -291,6 +291,7 @@ typedef struct drm_i915_irq_wait {
#define I915_PARAM_HAS_COHERENT_RINGS 13
#define I915_PARAM_HAS_EXEC_CONSTANTS 14
#define I915_PARAM_HAS_RELAXED_DELTA 15
+#define I915_PARAM_HAS_GEN7_SOL_RESET 16
typedef struct drm_i915_getparam {
int param;
@@ -653,6 +654,9 @@ struct drm_i915_gem_execbuffer2 {
__u64 rsvd2;
};
+/** Resets the SO write offset registers for transform feedback on gen7. */
+#define I915_EXEC_GEN7_SOL_RESET (1<<8)
+
struct drm_i915_gem_pin {
/** Handle of the buffer to be pinned. */
__u32 handle;
--
1.7.7.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] drm/i915: Add support for resetting the SO write pointers on gen7.
2011-12-30 1:52 [PATCH] drm/i915: Add support for resetting the SO write pointers on gen7 Eric Anholt
@ 2012-01-02 15:04 ` Eugeni Dodonov
2012-01-04 1:56 ` Eric Anholt
2012-01-02 19:51 ` Kenneth Graunke
2012-01-04 3:40 ` Ben Widawsky
2 siblings, 1 reply; 6+ messages in thread
From: Eugeni Dodonov @ 2012-01-02 15:04 UTC (permalink / raw)
To: Eric Anholt; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 2026 bytes --]
On Thu, Dec 29, 2011 at 23:52, Eric Anholt <eric@anholt.net> wrote:
> These registers are automatically incremented by the hardware during
> transform feedback to track where the next streamed vertex output
> should go. Unlike the previous generation, which had a packet for
> setting the corresponding registers to a defined value, gen7 only has
> MI_LOAD_REGISTER_IMM to do so. That's a secure packet (since it loads
> an arbitrary register), so we need to do it from the kernel, and it
> needs to be settable atomically with the batchbuffer execution so that
> two clients doing transform feedback don't stomp on each others'
> state.
>
> Instead of building a more complicated interface involcing setting the
> registers to a specific value, just set them to 0 when asked and
> userland can tweak its pointers accordingly.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
>
---
> drivers/gpu/drm/i915/i915_dma.c | 3 ++
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 31
> ++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_reg.h | 6 +++++
> include/drm/i915_drm.h | 4 +++
> 4 files changed, 44 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c
> b/drivers/gpu/drm/i915/i915_dma.c
> index a9ae374..1add685 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -781,6 +781,9 @@ static int i915_getparam(struct drm_device *dev, void
> *data,
> case I915_PARAM_HAS_RELAXED_DELTA:
> value = 1;
> break;
> + case I915_PARAM_HAS_GEN7_SOL_RESET:
> + value = 1;
>
Wouldn't it be better to have:
value = IS_GEN7(dev);
as it is gen7+-specific item. This way, userspace could check for this
support early, and avoid setting the flag on the batchbuffer in vain on
pre-gen7 architectures.
Either way, it will work, so:
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
--
Eugeni Dodonov
<http://eugeni.dodonov.net/>
[-- Attachment #1.2: Type: text/html, Size: 2695 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] 6+ messages in thread* Re: [PATCH] drm/i915: Add support for resetting the SO write pointers on gen7.
2012-01-02 15:04 ` Eugeni Dodonov
@ 2012-01-04 1:56 ` Eric Anholt
0 siblings, 0 replies; 6+ messages in thread
From: Eric Anholt @ 2012-01-04 1:56 UTC (permalink / raw)
To: Eugeni Dodonov; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 1898 bytes --]
On Mon, 2 Jan 2012 13:04:37 -0200, Eugeni Dodonov <eugeni@dodonov.net> wrote:
> On Thu, Dec 29, 2011 at 23:52, Eric Anholt <eric@anholt.net> wrote:
>
> > These registers are automatically incremented by the hardware during
> > transform feedback to track where the next streamed vertex output
> > should go. Unlike the previous generation, which had a packet for
> > setting the corresponding registers to a defined value, gen7 only has
> > MI_LOAD_REGISTER_IMM to do so. That's a secure packet (since it loads
> > an arbitrary register), so we need to do it from the kernel, and it
> > needs to be settable atomically with the batchbuffer execution so that
> > two clients doing transform feedback don't stomp on each others'
> > state.
> >
> > Instead of building a more complicated interface involcing setting the
> > registers to a specific value, just set them to 0 when asked and
> > userland can tweak its pointers accordingly.
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > b/drivers/gpu/drm/i915/i915_dma.c
> > index a9ae374..1add685 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -781,6 +781,9 @@ static int i915_getparam(struct drm_device *dev, void
> > *data,
> > case I915_PARAM_HAS_RELAXED_DELTA:
> > value = 1;
> > break;
> > + case I915_PARAM_HAS_GEN7_SOL_RESET:
> > + value = 1;
> >
>
> Wouldn't it be better to have:
> value = IS_GEN7(dev);
>
> as it is gen7+-specific item. This way, userspace could check for this
> support early, and avoid setting the flag on the batchbuffer in vain on
> pre-gen7 architectures.
>
> Either way, it will work, so:
>
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
The flag only gets set by userland in the gen7 code, so it wouldn't
change anything.
[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 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] 6+ messages in thread
* Re: [PATCH] drm/i915: Add support for resetting the SO write pointers on gen7.
2011-12-30 1:52 [PATCH] drm/i915: Add support for resetting the SO write pointers on gen7 Eric Anholt
2012-01-02 15:04 ` Eugeni Dodonov
@ 2012-01-02 19:51 ` Kenneth Graunke
2012-01-04 3:40 ` Ben Widawsky
2 siblings, 0 replies; 6+ messages in thread
From: Kenneth Graunke @ 2012-01-02 19:51 UTC (permalink / raw)
To: Eric Anholt; +Cc: intel-gfx
On 12/29/2011 05:52 PM, Eric Anholt wrote:
> These registers are automatically incremented by the hardware during
> transform feedback to track where the next streamed vertex output
> should go. Unlike the previous generation, which had a packet for
> setting the corresponding registers to a defined value, gen7 only has
> MI_LOAD_REGISTER_IMM to do so. That's a secure packet (since it loads
> an arbitrary register), so we need to do it from the kernel, and it
> needs to be settable atomically with the batchbuffer execution so that
> two clients doing transform feedback don't stomp on each others'
> state.
>
> Instead of building a more complicated interface involcing setting the
> registers to a specific value, just set them to 0 when asked and
> userland can tweak its pointers accordingly.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
Looks great!
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Add support for resetting the SO write pointers on gen7.
2011-12-30 1:52 [PATCH] drm/i915: Add support for resetting the SO write pointers on gen7 Eric Anholt
2012-01-02 15:04 ` Eugeni Dodonov
2012-01-02 19:51 ` Kenneth Graunke
@ 2012-01-04 3:40 ` Ben Widawsky
2012-01-04 8:34 ` Eric Anholt
2 siblings, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2012-01-04 3:40 UTC (permalink / raw)
To: Eric Anholt; +Cc: intel-gfx
On 12/29/2011 05:52 PM, Eric Anholt wrote:
> These registers are automatically incremented by the hardware during
> transform feedback to track where the next streamed vertex output
> should go. Unlike the previous generation, which had a packet for
> setting the corresponding registers to a defined value, gen7 only has
> MI_LOAD_REGISTER_IMM to do so. That's a secure packet (since it loads
> an arbitrary register), so we need to do it from the kernel, and it
> needs to be settable atomically with the batchbuffer execution so that
> two clients doing transform feedback don't stomp on each others'
> state.
>
> Instead of building a more complicated interface involcing setting the
> registers to a specific value, just set them to 0 when asked and
> userland can tweak its pointers accordingly.
>
> Signed-off-by: Eric Anholt<eric@anholt.net>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 3 ++
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 31 ++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_reg.h | 6 +++++
> include/drm/i915_drm.h | 4 +++
> 4 files changed, 44 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index a9ae374..1add685 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -781,6 +781,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
> case I915_PARAM_HAS_RELAXED_DELTA:
> value = 1;
> break;
> + case I915_PARAM_HAS_GEN7_SOL_RESET:
> + value = 1;
> + break;
> default:
> DRM_DEBUG_DRIVER("Unknown parameter %d\n",
> param->param);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index a4e4f3a..126144a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -971,6 +971,31 @@ i915_gem_execbuffer_retire_commands(struct drm_device *dev,
> }
>
> static int
> +i915_reset_gen7_sol_offsets(struct drm_device *dev,
> + struct intel_ring_buffer *ring)
> +{
> + drm_i915_private_t *dev_priv = dev->dev_private;
> + int ret, i;
> +
> + if (!IS_GEN7(dev) || ring !=&dev_priv->ring[RCS])
> + return 0;
> +
> + ret = intel_ring_begin(ring, 4 * 3);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i< 4; i++) {
> + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> + intel_ring_emit(ring, GEN7_SO_WRITE_OFFSET(i));
> + intel_ring_emit(ring, 0);
> + }
> +
> + intel_ring_advance(ring);
> +
> + return 0;
> +}
> +
> +static int
> i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> struct drm_file *file,
> struct drm_i915_gem_execbuffer2 *args,
> @@ -1182,6 +1207,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> goto err;
> }
>
> + if (args->flags& I915_EXEC_GEN7_SOL_RESET) {
> + ret = i915_reset_gen7_sol_offsets(dev, ring);
> + if (ret)
> + goto err;
> + }
> +
> trace_i915_gem_ring_dispatch(ring, seqno);
>
> exec_start = batch_obj->gtt_offset + args->batch_start_offset;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9d15474..54a18a4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3583,4 +3583,10 @@
> #define GEN7_AUD_CNTRL_ST_A 0xE50B4
> #define GEN7_AUD_CNTRL_ST2 0xE50C0
>
> +/* These are the 4 32-bit write offset registers for each stream
> + * output buffer. It determines the offset from the
> + * 3DSTATE_SO_BUFFERs that the next streamed vertex output goes to.
> + */
> +#define GEN7_SO_WRITE_OFFSET(n) (0x5280 + (n) * 4)
> +
> #endif /* _I915_REG_H_ */
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index 28c0d11..5bb6a6a 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -291,6 +291,7 @@ typedef struct drm_i915_irq_wait {
> #define I915_PARAM_HAS_COHERENT_RINGS 13
> #define I915_PARAM_HAS_EXEC_CONSTANTS 14
> #define I915_PARAM_HAS_RELAXED_DELTA 15
> +#define I915_PARAM_HAS_GEN7_SOL_RESET 16
>
> typedef struct drm_i915_getparam {
> int param;
> @@ -653,6 +654,9 @@ struct drm_i915_gem_execbuffer2 {
> __u64 rsvd2;
> };
>
> +/** Resets the SO write offset registers for transform feedback on gen7. */
> +#define I915_EXEC_GEN7_SOL_RESET (1<<8)
> +
> struct drm_i915_gem_pin {
> /** Handle of the buffer to be pinned. */
> __u32 handle;
Is this something we want to carry long term wrt ABI. I really want to
get context/ppgtt stuff out the door relatively soon (context should be
ready to test really soon actually).
I'm totally unfamiliar with this register, but is this what you want the
interface to be permanently?
Ben
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/i915: Add support for resetting the SO write pointers on gen7.
2012-01-04 3:40 ` Ben Widawsky
@ 2012-01-04 8:34 ` Eric Anholt
0 siblings, 0 replies; 6+ messages in thread
From: Eric Anholt @ 2012-01-04 8:34 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 1620 bytes --]
On Tue, 03 Jan 2012 19:40:36 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> On 12/29/2011 05:52 PM, Eric Anholt wrote:
> > These registers are automatically incremented by the hardware during
> > transform feedback to track where the next streamed vertex output
> > should go. Unlike the previous generation, which had a packet for
> > setting the corresponding registers to a defined value, gen7 only has
> > MI_LOAD_REGISTER_IMM to do so. That's a secure packet (since it loads
> > an arbitrary register), so we need to do it from the kernel, and it
> > needs to be settable atomically with the batchbuffer execution so that
> > two clients doing transform feedback don't stomp on each others'
> > state.
> >
> > Instead of building a more complicated interface involcing setting the
> > registers to a specific value, just set them to 0 when asked and
> > userland can tweak its pointers accordingly.
> >
> > Signed-off-by: Eric Anholt<eric@anholt.net>
> > +/** Resets the SO write offset registers for transform feedback on gen7. */
> > +#define I915_EXEC_GEN7_SOL_RESET (1<<8)
> > +
> > struct drm_i915_gem_pin {
> > /** Handle of the buffer to be pinned. */
> > __u32 handle;
>
> Is this something we want to carry long term wrt ABI. I really want to
> get context/ppgtt stuff out the door relatively soon (context should be
> ready to test really soon actually).
>
> I'm totally unfamiliar with this register, but is this what you want the
> interface to be permanently?
Ultimately, we want this ABI plus contexts. Until then, we can live
with this ABI alone.
[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 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] 6+ messages in thread
end of thread, other threads:[~2012-01-04 8:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-30 1:52 [PATCH] drm/i915: Add support for resetting the SO write pointers on gen7 Eric Anholt
2012-01-02 15:04 ` Eugeni Dodonov
2012-01-04 1:56 ` Eric Anholt
2012-01-02 19:51 ` Kenneth Graunke
2012-01-04 3:40 ` Ben Widawsky
2012-01-04 8:34 ` Eric Anholt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.