Re: [Intel-gfx] [PATCH v2] drm/i915: Sanitize engine context sizes

2017-04-27 Thread Joonas Lahtinen 
On ke, 2017-04-26 at 14:16 +0100, Tvrtko Ursulin wrote:
> On 26/04/2017 13:20, Joonas Lahtinen wrote:
> > 
> > @@ -443,21 +417,6 @@ int i915_gem_context_init(struct drm_i915_private 
> > *dev_priv)
> >     BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
> >     ida_init(_priv->context_hw_ida);
> > 
> > -   if (i915.enable_execlists) {
> > -   /* NB: intentionally left blank. We will allocate our own
> > -    * backing objects as we need them, thank you very much */
> > -   dev_priv->hw_context_size = 0;
> > -   } else if (HAS_HW_CONTEXTS(dev_priv)) {
> > -   dev_priv->hw_context_size =
> > -   round_up(get_context_size(dev_priv),
> > -    I915_GTT_PAGE_SIZE);
> 
> Is this rounding up lost when used from __create_hw_context?

Added it back for Gen 7 and 6 where it could do something. Others have
been rounded up in the heads of engineers already.

> > +static u32
> > +__intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
> Very minor, but Chris has been trying to establish i915 instead of 
> dev_priv in new code.

It'd shadow the global i915 in this case, so leaving it as is :/

> > @@ -134,6 +208,10 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
> >     engine->irq_shift = info->irq_shift;
> >     engine->class = info->class;
> >     engine->instance = info->instance;
> > +   engine->context_size = __intel_engine_context_size(dev_priv,
> > +      engine->class);
> > +   if (WARN_ON(engine->context_size > BIT(20)))
> > +   engine->context_size = 0;
> 
> Don't know the history to tell whether upgrade of DRM_DEBUG_DRIVER to a 
> WARN_ON is ok.

Talked with Chris, if it triggers, it should definitely be WARN_ON :)
Never seen in the past.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Sanitize engine context sizes

2017-04-26 Thread Tvrtko Ursulin 


On 26/04/2017 14:16, Tvrtko Ursulin wrote:

On 26/04/2017 13:20, Joonas Lahtinen wrote:

Pre-calculate engine context size based on engine class and device
generation and store it in the engine instance.

v2:
- Squash and get rid of hw_context_size (Chris)

Signed-off-by: Joonas Lahtinen 
Cc: Paulo Zanoni 
Cc: Rodrigo Vivi 
Cc: Chris Wilson 
Cc: Daniele Ceraolo Spurio 
Cc: Tvrtko Ursulin 
Cc: Oscar Mateo 
Cc: Zhenyu Wang 
Cc: intel-gvt-...@lists.freedesktop.org
---
 drivers/gpu/drm/i915/gvt/scheduler.c   |  6 +--
 drivers/gpu/drm/i915/i915_drv.h|  1 -
 drivers/gpu/drm/i915/i915_gem_context.c| 59 +++---
 drivers/gpu/drm/i915/i915_guc_submission.c |  3 +-
 drivers/gpu/drm/i915/i915_reg.h| 10 
 drivers/gpu/drm/i915/intel_engine_cs.c | 78
++
 drivers/gpu/drm/i915/intel_lrc.c   | 54 +
 drivers/gpu/drm/i915/intel_lrc.h   |  2 -
 drivers/gpu/drm/i915/intel_ringbuffer.h|  7 +--
 9 files changed, 93 insertions(+), 127 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c
b/drivers/gpu/drm/i915/gvt/scheduler.c
index a77db23..ac538dc 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -69,8 +69,7 @@ static int populate_shadow_context(struct
intel_vgpu_workload *workload)
 gvt_dbg_sched("ring id %d workload lrca %x", ring_id,
 workload->ctx_desc.lrca);

-context_page_num = intel_lr_context_size(
-gvt->dev_priv->engine[ring_id]);
+context_page_num = gvt->dev_priv->engine[ring_id]->context_size;

 context_page_num = context_page_num >> PAGE_SHIFT;

@@ -333,8 +332,7 @@ static void update_guest_context(struct
intel_vgpu_workload *workload)
 gvt_dbg_sched("ring id %d workload lrca %x\n", ring_id,
 workload->ctx_desc.lrca);

-context_page_num = intel_lr_context_size(
-gvt->dev_priv->engine[ring_id]);
+context_page_num = gvt->dev_priv->engine[ring_id]->context_size;

 context_page_num = context_page_num >> PAGE_SHIFT;

diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 357b6c6..4b54b92 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2359,7 +2359,6 @@ struct drm_i915_private {
  */
 struct mutex av_mutex;

-uint32_t hw_context_size;
 struct list_head context_list;

 u32 fdi_rx_config;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 8bd0c49..b69a026 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -92,33 +92,6 @@

 #define ALL_L3_SLICES(dev) (1 << NUM_L3_SLICES(dev)) - 1

-static int get_context_size(struct drm_i915_private *dev_priv)
-{
-int ret;
-u32 reg;
-
-switch (INTEL_GEN(dev_priv)) {
-case 6:
-reg = I915_READ(CXT_SIZE);
-ret = GEN6_CXT_TOTAL_SIZE(reg) * 64;
-break;
-case 7:
-reg = I915_READ(GEN7_CXT_SIZE);
-if (IS_HASWELL(dev_priv))
-ret = HSW_CXT_TOTAL_SIZE;
-else
-ret = GEN7_CXT_TOTAL_SIZE(reg) * 64;
-break;
-case 8:
-ret = GEN8_CXT_TOTAL_SIZE;
-break;
-default:
-BUG();
-}
-
-return ret;
-}
-
 void i915_gem_context_free(struct kref *ctx_ref)
 {
 struct i915_gem_context *ctx = container_of(ctx_ref,
typeof(*ctx), ref);
@@ -266,11 +239,12 @@ __create_hw_context(struct drm_i915_private
*dev_priv,
 list_add_tail(>link, _priv->context_list);
 ctx->i915 = dev_priv;

-if (dev_priv->hw_context_size) {
+if (dev_priv->engine[RCS]->context_size) {
 struct drm_i915_gem_object *obj;
 struct i915_vma *vma;

-obj = alloc_context_obj(dev_priv, dev_priv->hw_context_size);
+obj = alloc_context_obj(dev_priv,
+dev_priv->engine[RCS]->context_size);
 if (IS_ERR(obj)) {
 ret = PTR_ERR(obj);
 goto err_out;
@@ -443,21 +417,6 @@ int i915_gem_context_init(struct drm_i915_private
*dev_priv)
 BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
 ida_init(_priv->context_hw_ida);

-if (i915.enable_execlists) {
-/* NB: intentionally left blank. We will allocate our own
- * backing objects as we need them, thank you very much */
-dev_priv->hw_context_size = 0;
-} else if (HAS_HW_CONTEXTS(dev_priv)) {
-dev_priv->hw_context_size =
-round_up(get_context_size(dev_priv),
- I915_GTT_PAGE_SIZE);


Is this rounding up lost when used from __create_hw_context?


Also lost seems keeping hw_context_size at zero when !HAS_HW_CONTEXT, 
deliberate? Looks like the replacement of contexts_enabled

Re: [Intel-gfx] [PATCH v2] drm/i915: Sanitize engine context sizes

2017-04-26 Thread Tvrtko Ursulin 


On 26/04/2017 13:20, Joonas Lahtinen wrote:

Pre-calculate engine context size based on engine class and device
generation and store it in the engine instance.

v2:
- Squash and get rid of hw_context_size (Chris)

Signed-off-by: Joonas Lahtinen 
Cc: Paulo Zanoni 
Cc: Rodrigo Vivi 
Cc: Chris Wilson 
Cc: Daniele Ceraolo Spurio 
Cc: Tvrtko Ursulin 
Cc: Oscar Mateo 
Cc: Zhenyu Wang 
Cc: intel-gvt-...@lists.freedesktop.org
---
 drivers/gpu/drm/i915/gvt/scheduler.c   |  6 +--
 drivers/gpu/drm/i915/i915_drv.h|  1 -
 drivers/gpu/drm/i915/i915_gem_context.c| 59 +++---
 drivers/gpu/drm/i915/i915_guc_submission.c |  3 +-
 drivers/gpu/drm/i915/i915_reg.h| 10 
 drivers/gpu/drm/i915/intel_engine_cs.c | 78 ++
 drivers/gpu/drm/i915/intel_lrc.c   | 54 +
 drivers/gpu/drm/i915/intel_lrc.h   |  2 -
 drivers/gpu/drm/i915/intel_ringbuffer.h|  7 +--
 9 files changed, 93 insertions(+), 127 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c 
b/drivers/gpu/drm/i915/gvt/scheduler.c
index a77db23..ac538dc 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -69,8 +69,7 @@ static int populate_shadow_context(struct intel_vgpu_workload 
*workload)
gvt_dbg_sched("ring id %d workload lrca %x", ring_id,
workload->ctx_desc.lrca);

-   context_page_num = intel_lr_context_size(
-   gvt->dev_priv->engine[ring_id]);
+   context_page_num = gvt->dev_priv->engine[ring_id]->context_size;

context_page_num = context_page_num >> PAGE_SHIFT;

@@ -333,8 +332,7 @@ static void update_guest_context(struct intel_vgpu_workload 
*workload)
gvt_dbg_sched("ring id %d workload lrca %x\n", ring_id,
workload->ctx_desc.lrca);

-   context_page_num = intel_lr_context_size(
-   gvt->dev_priv->engine[ring_id]);
+   context_page_num = gvt->dev_priv->engine[ring_id]->context_size;

context_page_num = context_page_num >> PAGE_SHIFT;

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 357b6c6..4b54b92 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2359,7 +2359,6 @@ struct drm_i915_private {
 */
struct mutex av_mutex;

-   uint32_t hw_context_size;
struct list_head context_list;

u32 fdi_rx_config;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index 8bd0c49..b69a026 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -92,33 +92,6 @@

 #define ALL_L3_SLICES(dev) (1 << NUM_L3_SLICES(dev)) - 1

-static int get_context_size(struct drm_i915_private *dev_priv)
-{
-   int ret;
-   u32 reg;
-
-   switch (INTEL_GEN(dev_priv)) {
-   case 6:
-   reg = I915_READ(CXT_SIZE);
-   ret = GEN6_CXT_TOTAL_SIZE(reg) * 64;
-   break;
-   case 7:
-   reg = I915_READ(GEN7_CXT_SIZE);
-   if (IS_HASWELL(dev_priv))
-   ret = HSW_CXT_TOTAL_SIZE;
-   else
-   ret = GEN7_CXT_TOTAL_SIZE(reg) * 64;
-   break;
-   case 8:
-   ret = GEN8_CXT_TOTAL_SIZE;
-   break;
-   default:
-   BUG();
-   }
-
-   return ret;
-}
-
 void i915_gem_context_free(struct kref *ctx_ref)
 {
struct i915_gem_context *ctx = container_of(ctx_ref, typeof(*ctx), ref);
@@ -266,11 +239,12 @@ __create_hw_context(struct drm_i915_private *dev_priv,
list_add_tail(>link, _priv->context_list);
ctx->i915 = dev_priv;

-   if (dev_priv->hw_context_size) {
+   if (dev_priv->engine[RCS]->context_size) {
struct drm_i915_gem_object *obj;
struct i915_vma *vma;

-   obj = alloc_context_obj(dev_priv, dev_priv->hw_context_size);
+   obj = alloc_context_obj(dev_priv,
+   dev_priv->engine[RCS]->context_size);
if (IS_ERR(obj)) {
ret = PTR_ERR(obj);
goto err_out;
@@ -443,21 +417,6 @@ int i915_gem_context_init(struct drm_i915_private 
*dev_priv)
BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
ida_init(_priv->context_hw_ida);

-   if (i915.enable_execlists) {
-   /* NB: intentionally left blank. We will allocate our own
-* backing objects as we need them, thank you very much */
-   dev_priv->hw_context_size = 0;
-   } else if (HAS_HW_CONTEXTS(dev_priv)) {
-

[Intel-gfx] [PATCH v2] drm/i915: Sanitize engine context sizes

2017-04-26 Thread Joonas Lahtinen 
Pre-calculate engine context size based on engine class and device
generation and store it in the engine instance.

v2:
- Squash and get rid of hw_context_size (Chris)

Signed-off-by: Joonas Lahtinen 
Cc: Paulo Zanoni 
Cc: Rodrigo Vivi 
Cc: Chris Wilson 
Cc: Daniele Ceraolo Spurio 
Cc: Tvrtko Ursulin 
Cc: Oscar Mateo 
Cc: Zhenyu Wang 
Cc: intel-gvt-...@lists.freedesktop.org
---
 drivers/gpu/drm/i915/gvt/scheduler.c   |  6 +--
 drivers/gpu/drm/i915/i915_drv.h|  1 -
 drivers/gpu/drm/i915/i915_gem_context.c| 59 +++---
 drivers/gpu/drm/i915/i915_guc_submission.c |  3 +-
 drivers/gpu/drm/i915/i915_reg.h| 10 
 drivers/gpu/drm/i915/intel_engine_cs.c | 78 ++
 drivers/gpu/drm/i915/intel_lrc.c   | 54 +
 drivers/gpu/drm/i915/intel_lrc.h   |  2 -
 drivers/gpu/drm/i915/intel_ringbuffer.h|  7 +--
 9 files changed, 93 insertions(+), 127 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c 
b/drivers/gpu/drm/i915/gvt/scheduler.c
index a77db23..ac538dc 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -69,8 +69,7 @@ static int populate_shadow_context(struct intel_vgpu_workload 
*workload)
gvt_dbg_sched("ring id %d workload lrca %x", ring_id,
workload->ctx_desc.lrca);
 
-   context_page_num = intel_lr_context_size(
-   gvt->dev_priv->engine[ring_id]);
+   context_page_num = gvt->dev_priv->engine[ring_id]->context_size;
 
context_page_num = context_page_num >> PAGE_SHIFT;
 
@@ -333,8 +332,7 @@ static void update_guest_context(struct intel_vgpu_workload 
*workload)
gvt_dbg_sched("ring id %d workload lrca %x\n", ring_id,
workload->ctx_desc.lrca);
 
-   context_page_num = intel_lr_context_size(
-   gvt->dev_priv->engine[ring_id]);
+   context_page_num = gvt->dev_priv->engine[ring_id]->context_size;
 
context_page_num = context_page_num >> PAGE_SHIFT;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 357b6c6..4b54b92 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2359,7 +2359,6 @@ struct drm_i915_private {
 */
struct mutex av_mutex;
 
-   uint32_t hw_context_size;
struct list_head context_list;
 
u32 fdi_rx_config;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index 8bd0c49..b69a026 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -92,33 +92,6 @@
 
 #define ALL_L3_SLICES(dev) (1 << NUM_L3_SLICES(dev)) - 1
 
-static int get_context_size(struct drm_i915_private *dev_priv)
-{
-   int ret;
-   u32 reg;
-
-   switch (INTEL_GEN(dev_priv)) {
-   case 6:
-   reg = I915_READ(CXT_SIZE);
-   ret = GEN6_CXT_TOTAL_SIZE(reg) * 64;
-   break;
-   case 7:
-   reg = I915_READ(GEN7_CXT_SIZE);
-   if (IS_HASWELL(dev_priv))
-   ret = HSW_CXT_TOTAL_SIZE;
-   else
-   ret = GEN7_CXT_TOTAL_SIZE(reg) * 64;
-   break;
-   case 8:
-   ret = GEN8_CXT_TOTAL_SIZE;
-   break;
-   default:
-   BUG();
-   }
-
-   return ret;
-}
-
 void i915_gem_context_free(struct kref *ctx_ref)
 {
struct i915_gem_context *ctx = container_of(ctx_ref, typeof(*ctx), ref);
@@ -266,11 +239,12 @@ __create_hw_context(struct drm_i915_private *dev_priv,
list_add_tail(>link, _priv->context_list);
ctx->i915 = dev_priv;
 
-   if (dev_priv->hw_context_size) {
+   if (dev_priv->engine[RCS]->context_size) {
struct drm_i915_gem_object *obj;
struct i915_vma *vma;
 
-   obj = alloc_context_obj(dev_priv, dev_priv->hw_context_size);
+   obj = alloc_context_obj(dev_priv,
+   dev_priv->engine[RCS]->context_size);
if (IS_ERR(obj)) {
ret = PTR_ERR(obj);
goto err_out;
@@ -443,21 +417,6 @@ int i915_gem_context_init(struct drm_i915_private 
*dev_priv)
BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
ida_init(_priv->context_hw_ida);
 
-   if (i915.enable_execlists) {
-   /* NB: intentionally left blank. We will allocate our own
-* backing objects as we need them, thank you very much */
-   dev_priv->hw_context_size = 0;
-   } else if (HAS_HW_CONTEXTS(dev_priv)) {
-   dev_priv->hw_context_size =
-