[PATCH v1 0/4] Add Pooled EU support to BXT

[PATCH v1 0/4] Add Pooled EU support to BXT

[Arun Siluvery]

This patches adds support to enable Pooled EU feature in BXT.

This series has a dependency with Patch1 from Mika hence sending it
as part of the series (already reviewed).

Patch2 - adds a framework to extend the golden context batch through
which we can add Gen based commands to enable specific features, in this
case it is used to enabled Pooled EU (Patch3)

This is on the idea suggested by Chris Wilson to send two batches instead
of the previous approach of patching the binary data which is unnecessarily
complicated (http://www.spinics.net/lists/intel-gfx/msg71498.html).

Patch4 - option for the userspace to query it's availability.


Arun Siluvery (3):
  drm/i915: Add provision to extend Golden context batch
  drm/i915:bxt: Enable Pooled EU support
  drm/i915/bxt: Add get_param to query Pooled EU availability

Mika Kuoppala (1):
  drm/i915: Do kunmap if renderstate parsing fails

 drivers/gpu/drm/i915/i915_dma.c              |  3 ++
 drivers/gpu/drm/i915/i915_drv.c              |  1 +
 drivers/gpu/drm/i915/i915_drv.h              |  5 ++-
 drivers/gpu/drm/i915/i915_gem_render_state.c | 52 ++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_gem_render_state.h |  2 ++
 drivers/gpu/drm/i915/i915_reg.h              |  2 ++
 drivers/gpu/drm/i915/intel_lrc.c             |  6 ++++
 include/uapi/drm/i915_drm.h                  |  1 +
 8 files changed, 69 insertions(+), 3 deletions(-)

-- 
1.9.1

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

[PATCH v1 1/4] drm/i915: Do kunmap if renderstate parsing fails

[Arun Siluvery]

From: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Kunmap the renderstate page on error path.

Reviewed-by: Arun Siluvery <arun.siluvery@linux.intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_render_state.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
index a0201fc..b6492fe 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -96,8 +96,10 @@ static int render_state_setup(struct render_state *so)
 			s = lower_32_bits(r);
 			if (so->gen >= 8) {
 				if (i + 1 >= rodata->batch_items ||
-				    rodata->batch[i + 1] != 0)
-					return -EINVAL;
+				    rodata->batch[i + 1] != 0) {
+					ret = -EINVAL;
+					goto err_out;
+				}
 
 				d[i++] = s;
 				s = upper_32_bits(r);
@@ -120,6 +122,10 @@ static int render_state_setup(struct render_state *so)
 	}
 
 	return 0;
+
+err_out:
+	kunmap(page);
+	return ret;
 }
 
 void i915_gem_render_state_fini(struct render_state *so)
-- 
1.9.1

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

[PATCH v1 2/4] drm/i915: Add provision to extend Golden context batch

[Arun Siluvery]

The Golden batch carries 3D state at the beginning so that HW starts with
a known state. It is carried as a binary blob which is auto-generated from
source. The idea was it would be easier to maintain and keep the complexity
out of the kernel which makes sense as we don't really touch it. However if
you really need to update it then you need to update generator source and
keep the binary blob in sync with it.

There is a need to patch this in bxt to send one additional command to enable
a feature. A solution was to patch the binary data with some additional
data structures (included as part of auto-generator source) but it was
unnecessarily complicated.

Chris suggested the idea of having a secondary batch and execute two batch
buffers. It has clear advantages as we needn't touch the base golden batch,
can customize secondary/auxiliary batch depending on Gen and can be carried
in the driver with no dependencies.

This patch adds support for this auxiliary batch which is inserted at the
end of golden batch and is completely independent from it. Thanks to Mika
for the preliminary review.

Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Armin Reese <armin.c.reese@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_render_state.c | 27 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_render_state.h |  2 ++
 drivers/gpu/drm/i915/intel_lrc.c             |  6 ++++++
 3 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
index b6492fe..b86e382 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -73,6 +73,15 @@ free_gem:
 	return ret;
 }
 
+#define OUT_BATCH(batch, i, val)				\
+	do {							\
+		if (WARN_ON((i) >= PAGE_SIZE / sizeof(u32))) {	\
+			ret = -ENOSPC;				\
+			goto err_out;				\
+		}						\
+		(batch)[(i)++] = (val);				\
+	} while(0)
+
 static int render_state_setup(struct render_state *so)
 {
 	const struct intel_renderstate_rodata *rodata = so->rodata;
@@ -110,6 +119,15 @@ static int render_state_setup(struct render_state *so)
 
 		d[i++] = s;
 	}
+
+	while (i % CACHELINE_DWORDS)
+		OUT_BATCH(d, i, MI_NOOP);
+
+	so->aux_batch_offset = i * sizeof(u32);
+
+	OUT_BATCH(d, i, MI_BATCH_BUFFER_END);
+	so->aux_batch_size = (i * sizeof(u32)) - so->aux_batch_offset;
+
 	kunmap(page);
 
 	ret = i915_gem_object_set_to_gtt_domain(so->obj, false);
@@ -128,6 +146,8 @@ err_out:
 	return ret;
 }
 
+#undef OUT_BATCH
+
 void i915_gem_render_state_fini(struct render_state *so)
 {
 	i915_gem_object_ggtt_unpin(so->obj);
@@ -176,6 +196,13 @@ int i915_gem_render_state_init(struct drm_i915_gem_request *req)
 	if (ret)
 		goto out;
 
+	ret = req->ring->dispatch_execbuffer(req,
+					     (so.ggtt_offset + so.aux_batch_offset),
+					     so.aux_batch_size,
+					     I915_DISPATCH_SECURE);
+	if (ret)
+		goto out;
+
 	i915_vma_move_to_active(i915_gem_obj_to_ggtt(so.obj), req);
 
 out:
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.h b/drivers/gpu/drm/i915/i915_gem_render_state.h
index 7aa7372..79de101 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.h
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.h
@@ -37,6 +37,8 @@ struct render_state {
 	struct drm_i915_gem_object *obj;
 	u64 ggtt_offset;
 	int gen;
+	u32 aux_batch_size;
+	u64 aux_batch_offset;
 };
 
 int i915_gem_render_state_init(struct drm_i915_gem_request *req);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index adb386d..5e4771e 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1661,6 +1661,12 @@ static int intel_lr_context_render_state_init(struct drm_i915_gem_request *req)
 	if (ret)
 		goto out;
 
+	ret = req->ring->emit_bb_start(req,
+				       (so.ggtt_offset + so.aux_batch_offset),
+				       I915_DISPATCH_SECURE);
+	if (ret)
+		goto out;
+
 	i915_vma_move_to_active(i915_gem_obj_to_ggtt(so.obj), req);
 
 out:
-- 
1.9.1

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

[PATCH v1 3/4] drm/i915:bxt: Enable Pooled EU support

[Arun Siluvery]

This mode allows to assign EUs to pools.
The command to enable this mode is sent in auxiliary golden context batch
as this is only issued once with each context initialization. Thanks to
Mika for the preliminary review.

Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Armin Reese <armin.c.reese@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_render_state.c | 15 +++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h              |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
index b86e382..a41a1b6 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -84,6 +84,7 @@ free_gem:
 
 static int render_state_setup(struct render_state *so)
 {
+	struct drm_device *dev = so->obj->base.dev;
 	const struct intel_renderstate_rodata *rodata = so->rodata;
 	unsigned int i = 0, reloc_index = 0;
 	struct page *page;
@@ -125,6 +126,20 @@ static int render_state_setup(struct render_state *so)
 
 	so->aux_batch_offset = i * sizeof(u32);
 
+	if (IS_BROXTON(dev)) {
+		u32 pool_config = 0;
+		struct drm_i915_private *dev_priv = to_i915(dev);
+
+		OUT_BATCH(d, i, GEN9_MEDIA_POOL_STATE);
+		OUT_BATCH(d, i, GEN9_MEDIA_POOL_ENABLE);
+		if (dev_priv->info.subslice_total == 3)
+			pool_config = 0x00777000;
+		OUT_BATCH(d, i, pool_config);
+		OUT_BATCH(d, i, 0);
+		OUT_BATCH(d, i, 0);
+		OUT_BATCH(d, i, 0);
+	}
+
 	OUT_BATCH(d, i, MI_BATCH_BUFFER_END);
 	so->aux_batch_size = (i * sizeof(u32)) - so->aux_batch_offset;
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9a2ffad..e052499 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -379,6 +379,8 @@
  */
 #define GFX_INSTR(opcode, flags) ((0x3 << 29) | ((opcode) << 24) | (flags))
 
+#define GEN9_MEDIA_POOL_STATE     ((0x3 << 29) | (0x2 << 27) | (0x5 << 16) | 4)
+#define   GEN9_MEDIA_POOL_ENABLE  (1 << 31)
 #define GFX_OP_RASTER_RULES    ((0x3<<29)|(0x7<<24))
 #define GFX_OP_SCISSOR         ((0x3<<29)|(0x1c<<24)|(0x10<<19))
 #define   SC_UPDATE_SCISSOR       (0x1<<1)
-- 
1.9.1

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

[PATCH v1 4/4] drm/i915/bxt: Add get_param to query Pooled EU availability

[Arun Siluvery]

User space clients need to know when the pooled EU feature is present
and enabled on the hardware so that they can adapt work submissions.
Create a new device info flag for this purpose, and create a new GETPARAM
entry to allow user space to query its setting.

Set has_pooled_eu to true in the Broxton static device info - Broxton
supports the feature in hardware and the driver will enable it by
default.

Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 3 +++
 drivers/gpu/drm/i915/i915_drv.c | 1 +
 drivers/gpu/drm/i915/i915_drv.h | 5 ++++-
 include/uapi/drm/i915_drm.h     | 1 +
 4 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5e63076..6c31beb 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -170,6 +170,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
 	case I915_PARAM_HAS_RESOURCE_STREAMER:
 		value = HAS_RESOURCE_STREAMER(dev);
 		break;
+	case I915_PARAM_HAS_POOLED_EU:
+		value = HAS_POOLED_EU(dev);
+		break;
 	default:
 		DRM_DEBUG("Unknown parameter %d\n", param->param);
 		return -EINVAL;
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e44dc0d..213f74d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -389,6 +389,7 @@ static const struct intel_device_info intel_broxton_info = {
 	.num_pipes = 3,
 	.has_ddi = 1,
 	.has_fbc = 1,
+	.has_pooled_eu = 1,
 	GEN_DEFAULT_PIPEOFFSETS,
 	IVB_CURSOR_OFFSETS,
 };
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 768d1db..32850a8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -775,7 +775,8 @@ struct intel_csr {
 	func(supports_tv) sep \
 	func(has_llc) sep \
 	func(has_ddi) sep \
-	func(has_fpga_dbg)
+	func(has_fpga_dbg) sep \
+	func(has_pooled_eu)
 
 #define DEFINE_FLAG(name) u8 name:1
 #define SEP_SEMICOLON ;
@@ -2549,6 +2550,8 @@ struct drm_i915_cmd_table {
 #define HAS_RESOURCE_STREAMER(dev) (IS_HASWELL(dev) || \
 				    INTEL_INFO(dev)->gen >= 8)
 
+#define HAS_POOLED_EU(dev) (INTEL_INFO(dev)->has_pooled_eu)
+
 #define INTEL_PCH_DEVICE_ID_MASK		0xff00
 #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
 #define INTEL_PCH_CPT_DEVICE_ID_TYPE		0x1c00
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index e7c29f1..9649577 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -356,6 +356,7 @@ typedef struct drm_i915_irq_wait {
 #define I915_PARAM_EU_TOTAL		 34
 #define I915_PARAM_HAS_GPU_RESET	 35
 #define I915_PARAM_HAS_RESOURCE_STREAMER 36
+#define I915_PARAM_HAS_POOLED_EU         37
 
 typedef struct drm_i915_getparam {
 	int param;
-- 
1.9.1

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

Re: [PATCH v1 2/4] drm/i915: Add provision to extend Golden context batch

[Chris Wilson]

On Fri, Jul 17, 2015 at 05:08:52PM +0100, Arun Siluvery wrote:
> The Golden batch carries 3D state at the beginning so that HW starts with
> a known state. It is carried as a binary blob which is auto-generated from
> source. The idea was it would be easier to maintain and keep the complexity
> out of the kernel which makes sense as we don't really touch it. However if
> you really need to update it then you need to update generator source and
> keep the binary blob in sync with it.
> 
> There is a need to patch this in bxt to send one additional command to enable
> a feature. A solution was to patch the binary data with some additional
> data structures (included as part of auto-generator source) but it was
> unnecessarily complicated.
> 
> Chris suggested the idea of having a secondary batch and execute two batch
> buffers. It has clear advantages as we needn't touch the base golden batch,
> can customize secondary/auxiliary batch depending on Gen and can be carried
> in the driver with no dependencies.
> 
> This patch adds support for this auxiliary batch which is inserted at the
> end of golden batch and is completely independent from it. Thanks to Mika
> for the preliminary review.
> 
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Armin Reese <armin.c.reese@intel.com>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_render_state.c | 27 +++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem_render_state.h |  2 ++
>  drivers/gpu/drm/i915/intel_lrc.c             |  6 ++++++
>  3 files changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index b6492fe..b86e382 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -73,6 +73,15 @@ free_gem:
>  	return ret;
>  }
>  
> +#define OUT_BATCH(batch, i, val)				\
> +	do {							\
> +		if (WARN_ON((i) >= PAGE_SIZE / sizeof(u32))) {	\

We have to be slightly more careful here, as we don't have the full page
available since we put render state into the high arena of the golden
bb. Something like WARN_ON(i > PAGE/sizeof(u32) || (batch)[i]) should
suffice.

> @@ -110,6 +119,15 @@ static int render_state_setup(struct render_state *so)
>  
>  		d[i++] = s;
>  	}
> +
> +	while (i % CACHELINE_DWORDS)
> +		OUT_BATCH(d, i, MI_NOOP);
> +
> +	so->aux_batch_offset = i * sizeof(u32);
> +
> +	OUT_BATCH(d, i, MI_BATCH_BUFFER_END);
> +	so->aux_batch_size = (i * sizeof(u32)) - so->aux_batch_offset;

Strictly, and if we are passing the batch length we are being strictly
conformant, then the aux_batch_size must be a multiple of 8.

> +
>  	kunmap(page);
>  
>  	ret = i915_gem_object_set_to_gtt_domain(so->obj, false);
> @@ -128,6 +146,8 @@ err_out:
>  	return ret;
>  }
>  
> +#undef OUT_BATCH
> +
>  void i915_gem_render_state_fini(struct render_state *so)
>  {
>  	i915_gem_object_ggtt_unpin(so->obj);
> @@ -176,6 +196,13 @@ int i915_gem_render_state_init(struct drm_i915_gem_request *req)
>  	if (ret)
>  		goto out;
>  
Then we need only execute this BB if so.aux_batch_size > 8

> +	ret = req->ring->dispatch_execbuffer(req,
> +					     (so.ggtt_offset + so.aux_batch_offset),
> +					     so.aux_batch_size,
> +					     I915_DISPATCH_SECURE);
> +	if (ret)
> +		goto out;
> +
>  	i915_vma_move_to_active(i915_gem_obj_to_ggtt(so.obj), req);
-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: [PATCH v1 3/4] drm/i915:bxt: Enable Pooled EU support

[Chris Wilson]

On Fri, Jul 17, 2015 at 05:08:53PM +0100, Arun Siluvery wrote:
> This mode allows to assign EUs to pools.
> The command to enable this mode is sent in auxiliary golden context batch
> as this is only issued once with each context initialization. Thanks to
> Mika for the preliminary review.

A quick explanation for why this has to be in the kernel would be nice.
Privileged instruction?

Not fond of the split between this and patch 4. Patch 4 intoduces one
feature flag that looks different to the one we use here to enable
support.
-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: [PATCH v1 2/4] drm/i915: Add provision to extend Golden context batch

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Fri, Jul 17, 2015 at 05:08:52PM +0100, Arun Siluvery wrote:
>> The Golden batch carries 3D state at the beginning so that HW starts with
>> a known state. It is carried as a binary blob which is auto-generated from
>> source. The idea was it would be easier to maintain and keep the complexity
>> out of the kernel which makes sense as we don't really touch it. However if
>> you really need to update it then you need to update generator source and
>> keep the binary blob in sync with it.
>> 
>> There is a need to patch this in bxt to send one additional command to enable
>> a feature. A solution was to patch the binary data with some additional
>> data structures (included as part of auto-generator source) but it was
>> unnecessarily complicated.
>> 
>> Chris suggested the idea of having a secondary batch and execute two batch
>> buffers. It has clear advantages as we needn't touch the base golden batch,
>> can customize secondary/auxiliary batch depending on Gen and can be carried
>> in the driver with no dependencies.
>> 
>> This patch adds support for this auxiliary batch which is inserted at the
>> end of golden batch and is completely independent from it. Thanks to Mika
>> for the preliminary review.
>> 
>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Armin Reese <armin.c.reese@intel.com>
>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_render_state.c | 27 +++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/i915_gem_render_state.h |  2 ++
>>  drivers/gpu/drm/i915/intel_lrc.c             |  6 ++++++
>>  3 files changed, 35 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
>> index b6492fe..b86e382 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
>> @@ -73,6 +73,15 @@ free_gem:
>>  	return ret;
>>  }
>>  
>> +#define OUT_BATCH(batch, i, val)				\
>> +	do {							\
>> +		if (WARN_ON((i) >= PAGE_SIZE / sizeof(u32))) {	\
>
> We have to be slightly more careful here, as we don't have the full page
> available since we put render state into the high arena of the golden
> bb. Something like WARN_ON(i > PAGE/sizeof(u32) || (batch)[i]) should
> suffice.
>

Null state gen makes the final batch with two passes. First
it builds command and state separately. And when size of both
are know, it compacts by relocating the state right after
the commands (+some alignment).

So we should have the rest of the page usable for auxillary
commands here as we have already copied the state part
also.

-Mika

>> @@ -110,6 +119,15 @@ static int render_state_setup(struct render_state *so)
>>  
>>  		d[i++] = s;
>>  	}
>> +
>> +	while (i % CACHELINE_DWORDS)
>> +		OUT_BATCH(d, i, MI_NOOP);
>> +
>> +	so->aux_batch_offset = i * sizeof(u32);
>> +
>> +	OUT_BATCH(d, i, MI_BATCH_BUFFER_END);
>> +	so->aux_batch_size = (i * sizeof(u32)) - so->aux_batch_offset;
>
> Strictly, and if we are passing the batch length we are being strictly
> conformant, then the aux_batch_size must be a multiple of 8.
>
>> +
>>  	kunmap(page);
>>  
>>  	ret = i915_gem_object_set_to_gtt_domain(so->obj, false);
>> @@ -128,6 +146,8 @@ err_out:
>>  	return ret;
>>  }
>>  
>> +#undef OUT_BATCH
>> +
>>  void i915_gem_render_state_fini(struct render_state *so)
>>  {
>>  	i915_gem_object_ggtt_unpin(so->obj);
>> @@ -176,6 +196,13 @@ int i915_gem_render_state_init(struct drm_i915_gem_request *req)
>>  	if (ret)
>>  		goto out;
>>  
> Then we need only execute this BB if so.aux_batch_size > 8
>
>> +	ret = req->ring->dispatch_execbuffer(req,
>> +					     (so.ggtt_offset + so.aux_batch_offset),
>> +					     so.aux_batch_size,
>> +					     I915_DISPATCH_SECURE);
>> +	if (ret)
>> +		goto out;
>> +
>>  	i915_vma_move_to_active(i915_gem_obj_to_ggtt(so.obj), req);
> -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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v1 2/4] drm/i915: Add provision to extend Golden context batch

[Chris Wilson]

On Fri, Jul 17, 2015 at 07:37:45PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On Fri, Jul 17, 2015 at 05:08:52PM +0100, Arun Siluvery wrote:
> >> The Golden batch carries 3D state at the beginning so that HW starts with
> >> a known state. It is carried as a binary blob which is auto-generated from
> >> source. The idea was it would be easier to maintain and keep the complexity
> >> out of the kernel which makes sense as we don't really touch it. However if
> >> you really need to update it then you need to update generator source and
> >> keep the binary blob in sync with it.
> >> 
> >> There is a need to patch this in bxt to send one additional command to enable
> >> a feature. A solution was to patch the binary data with some additional
> >> data structures (included as part of auto-generator source) but it was
> >> unnecessarily complicated.
> >> 
> >> Chris suggested the idea of having a secondary batch and execute two batch
> >> buffers. It has clear advantages as we needn't touch the base golden batch,
> >> can customize secondary/auxiliary batch depending on Gen and can be carried
> >> in the driver with no dependencies.
> >> 
> >> This patch adds support for this auxiliary batch which is inserted at the
> >> end of golden batch and is completely independent from it. Thanks to Mika
> >> for the preliminary review.
> >> 
> >> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Armin Reese <armin.c.reese@intel.com>
> >> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_gem_render_state.c | 27 +++++++++++++++++++++++++++
> >>  drivers/gpu/drm/i915/i915_gem_render_state.h |  2 ++
> >>  drivers/gpu/drm/i915/intel_lrc.c             |  6 ++++++
> >>  3 files changed, 35 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> >> index b6492fe..b86e382 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> >> @@ -73,6 +73,15 @@ free_gem:
> >>  	return ret;
> >>  }
> >>  
> >> +#define OUT_BATCH(batch, i, val)				\
> >> +	do {							\
> >> +		if (WARN_ON((i) >= PAGE_SIZE / sizeof(u32))) {	\
> >
> > We have to be slightly more careful here, as we don't have the full page
> > available since we put render state into the high arena of the golden
> > bb. Something like WARN_ON(i > PAGE/sizeof(u32) || (batch)[i]) should
> > suffice.
> >
> 
> Null state gen makes the final batch with two passes. First
> it builds command and state separately. And when size of both
> are know, it compacts by relocating the state right after
> the commands (+some alignment).
> 
> So we should have the rest of the page usable for auxillary
> commands here as we have already copied the state part
> also.

Ta. Maybe add some words of enlightenment here for future me as well?
Also we will need to document that the kernel then relies on the packing
to add extra commands after the batch to the null state generator.
-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: [PATCH v1 3/4] drm/i915:bxt: Enable Pooled EU support

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Fri, Jul 17, 2015 at 05:08:53PM +0100, Arun Siluvery wrote:
>> This mode allows to assign EUs to pools.
>> The command to enable this mode is sent in auxiliary golden context batch
>> as this is only issued once with each context initialization. Thanks to
>> Mika for the preliminary review.
>
> A quick explanation for why this has to be in the kernel would be nice.
> Privileged instruction?
>

The pooled mode is global. Once set, it has to stay same
across all contexts until subsequent fw reset.

-Mika

> Not fond of the split between this and patch 4. Patch 4 intoduces one
> feature flag that looks different to the one we use here to enable
> support.
> -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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v1 3/4] drm/i915:bxt: Enable Pooled EU support

[Siluvery, Arun]

On 17/07/2015 17:27, Chris Wilson wrote:
> On Fri, Jul 17, 2015 at 05:08:53PM +0100, Arun Siluvery wrote:
>> This mode allows to assign EUs to pools.
>> The command to enable this mode is sent in auxiliary golden context batch
>> as this is only issued once with each context initialization. Thanks to
>> Mika for the preliminary review.
>
> A quick explanation for why this has to be in the kernel would be nice.
> Privileged instruction?

This purpose of auxiliary batch is explained in patch2, but I can add 
some explanation about this one also.

>
> Not fond of the split between this and patch 4. Patch 4 intoduces one
> feature flag that looks different to the one we use here to enable
> support.
I will patch4 as separate as it deals with libdrm changes but use the 
feature flag in this one.

regards
Arun

> -Chris
>

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

Re: [PATCH v1 3/4] drm/i915:bxt: Enable Pooled EU support

[Chris Wilson]

On Fri, Jul 17, 2015 at 05:54:20PM +0100, Siluvery, Arun wrote:
> On 17/07/2015 17:27, Chris Wilson wrote:
> >On Fri, Jul 17, 2015 at 05:08:53PM +0100, Arun Siluvery wrote:
> >>This mode allows to assign EUs to pools.
> >>The command to enable this mode is sent in auxiliary golden context batch
> >>as this is only issued once with each context initialization. Thanks to
> >>Mika for the preliminary review.
> >
> >A quick explanation for why this has to be in the kernel would be nice.
> >Privileged instruction?
> 
> This purpose of auxiliary batch is explained in patch2, but I can
> add some explanation about this one also.

Here, I am looking for an explanation of why these commands in
particular are desired. Mika's short explanation that must be the same
for all contexts on the system is sufficient.
-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: [PATCH v1 1/4] drm/i915: Do kunmap if renderstate parsing fails

[Daniel Vetter]

On Fri, Jul 17, 2015 at 05:08:51PM +0100, Arun Siluvery wrote:
> From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> 
> Kunmap the renderstate page on error path.
> 
> Reviewed-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_render_state.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index a0201fc..b6492fe 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -96,8 +96,10 @@ static int render_state_setup(struct render_state *so)
>  			s = lower_32_bits(r);
>  			if (so->gen >= 8) {
>  				if (i + 1 >= rodata->batch_items ||
> -				    rodata->batch[i + 1] != 0)
> -					return -EINVAL;
> +				    rodata->batch[i + 1] != 0) {
> +					ret = -EINVAL;
> +					goto err_out;
> +				}
>  
>  				d[i++] = s;
>  				s = upper_32_bits(r);
> @@ -120,6 +122,10 @@ static int render_state_setup(struct render_state *so)
>  	}
>  
>  	return 0;
> +
> +err_out:
> +	kunmap(page);
> +	return ret;
>  }
>  
>  void i915_gem_render_state_fini(struct render_state *so)
> -- 
> 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