[PATCH 1/2] drm/i915: Teach record_defaults to operate on the intel_gt

[PATCH 1/2] drm/i915: Teach record_defaults to operate on the intel_gt

[Chris Wilson]

Again we wish to operate on the engines, which are owned by the
intel_gt. As such it is easier, and much more consistent, to pass the
intel_gt parameter.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ca64a0c9b762..b882988056bd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -48,6 +48,7 @@
 #include "gt/intel_engine_user.h"
 #include "gt/intel_gt.h"
 #include "gt/intel_gt_pm.h"
+#include "gt/intel_gt_requests.h"
 #include "gt/intel_mocs.h"
 #include "gt/intel_reset.h"
 #include "gt/intel_renderstate.h"
@@ -1072,7 +1073,7 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
 	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
 }
 
-static int __intel_engines_record_defaults(struct drm_i915_private *i915)
+static int __intel_engines_record_defaults(struct intel_gt *gt)
 {
 	struct i915_request *requests[I915_NUM_ENGINES] = {};
 	struct intel_engine_cs *engine;
@@ -1088,7 +1089,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 	 * from the same default HW values.
 	 */
 
-	for_each_engine(engine, i915, id) {
+	for_each_engine(engine, gt, id) {
 		struct intel_context *ce;
 		struct i915_request *rq;
 
@@ -1096,7 +1097,8 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 		GEM_BUG_ON(!engine->kernel_context);
 		engine->serial++; /* force the kernel context switch */
 
-		ce = intel_context_create(i915->kernel_context, engine);
+		ce = intel_context_create(engine->kernel_context->gem_context,
+					  engine);
 		if (IS_ERR(ce)) {
 			err = PTR_ERR(ce);
 			goto out;
@@ -1125,7 +1127,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 	}
 
 	/* Flush the default context image to memory, and enable powersaving. */
-	if (!i915_gem_load_power_context(i915)) {
+	if (intel_gt_wait_for_idle(gt, I915_GEM_IDLE_TIMEOUT) == -ETIME) {
 		err = -EIO;
 		goto out;
 	}
@@ -1184,7 +1186,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 	 * this is by declaring ourselves wedged.
 	 */
 	if (err)
-		intel_gt_set_wedged(&i915->gt);
+		intel_gt_set_wedged(gt);
 
 	for (id = 0; id < ARRAY_SIZE(requests); id++) {
 		struct intel_context *ce;
@@ -1298,7 +1300,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_gt;
 
-	ret = __intel_engines_record_defaults(dev_priv);
+	ret = __intel_engines_record_defaults(&dev_priv->gt);
 	if (ret)
 		goto err_gt;
 
-- 
2.24.0.rc0

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

[PATCH 2/2] drm/i915/selftests: Teach switch_to_context() to use the context

[Chris Wilson]

The context details which engines to use, so use the ctx->engines[] to
generate the requests to cause the context switch.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/selftests/i915_gem.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/selftests/i915_gem.c b/drivers/gpu/drm/i915/selftests/i915_gem.c
index 97f89f744ee2..84e7ca778b7b 100644
--- a/drivers/gpu/drm/i915/selftests/i915_gem.c
+++ b/drivers/gpu/drm/i915/selftests/i915_gem.c
@@ -15,16 +15,15 @@
 #include "igt_flush_test.h"
 #include "mock_drm.h"
 
-static int switch_to_context(struct drm_i915_private *i915,
-			     struct i915_gem_context *ctx)
+static int switch_to_context(struct i915_gem_context *ctx)
 {
-	struct intel_engine_cs *engine;
-	enum intel_engine_id id;
+	struct i915_gem_engines_iter it;
+	struct intel_context *ce;
 
-	for_each_engine(engine, i915, id) {
+	for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
 		struct i915_request *rq;
 
-		rq = igt_request_alloc(ctx, engine);
+		rq = intel_context_create_request(ce);
 		if (IS_ERR(rq))
 			return PTR_ERR(rq);
 
@@ -140,7 +139,7 @@ static int igt_gem_suspend(void *arg)
 	err = -ENOMEM;
 	ctx = live_context(i915, file);
 	if (!IS_ERR(ctx))
-		err = switch_to_context(i915, ctx);
+		err = switch_to_context(ctx);
 	if (err)
 		goto out;
 
@@ -155,7 +154,7 @@ static int igt_gem_suspend(void *arg)
 
 	pm_resume(i915);
 
-	err = switch_to_context(i915, ctx);
+	err = switch_to_context(ctx);
 out:
 	mock_file_free(i915, file);
 	return err;
@@ -175,7 +174,7 @@ static int igt_gem_hibernate(void *arg)
 	err = -ENOMEM;
 	ctx = live_context(i915, file);
 	if (!IS_ERR(ctx))
-		err = switch_to_context(i915, ctx);
+		err = switch_to_context(ctx);
 	if (err)
 		goto out;
 
@@ -190,7 +189,7 @@ static int igt_gem_hibernate(void *arg)
 
 	pm_resume(i915);
 
-	err = switch_to_context(i915, ctx);
+	err = switch_to_context(ctx);
 out:
 	mock_file_free(i915, file);
 	return err;
-- 
2.24.0.rc0

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

Re: [PATCH 1/2] drm/i915: Teach record_defaults to operate on the intel_gt

[Tvrtko Ursulin]

On 22/10/2019 14:02, Chris Wilson wrote:
> Again we wish to operate on the engines, which are owned by the
> intel_gt. As such it is easier, and much more consistent, to pass the
> intel_gt parameter.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ca64a0c9b762..b882988056bd 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -48,6 +48,7 @@
>   #include "gt/intel_engine_user.h"
>   #include "gt/intel_gt.h"
>   #include "gt/intel_gt_pm.h"
> +#include "gt/intel_gt_requests.h"
>   #include "gt/intel_mocs.h"
>   #include "gt/intel_reset.h"
>   #include "gt/intel_renderstate.h"
> @@ -1072,7 +1073,7 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
>   	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
>   }
>   
> -static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> +static int __intel_engines_record_defaults(struct intel_gt *gt)
>   {
>   	struct i915_request *requests[I915_NUM_ENGINES] = {};
>   	struct intel_engine_cs *engine;
> @@ -1088,7 +1089,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   	 * from the same default HW values.
>   	 */
>   
> -	for_each_engine(engine, i915, id) {
> +	for_each_engine(engine, gt, id) {
>   		struct intel_context *ce;
>   		struct i915_request *rq;
>   
> @@ -1096,7 +1097,8 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   		GEM_BUG_ON(!engine->kernel_context);
>   		engine->serial++; /* force the kernel context switch */
>   
> -		ce = intel_context_create(i915->kernel_context, engine);
> +		ce = intel_context_create(engine->kernel_context->gem_context,
> +					  engine);
>   		if (IS_ERR(ce)) {
>   			err = PTR_ERR(ce);
>   			goto out;
> @@ -1125,7 +1127,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   	}
>   
>   	/* Flush the default context image to memory, and enable powersaving. */
> -	if (!i915_gem_load_power_context(i915)) {
> +	if (intel_gt_wait_for_idle(gt, I915_GEM_IDLE_TIMEOUT) == -ETIME) {

What are the plans for i915_gem_load_power_context? It does a little bit 
extra. But also becomes confined to i915_gem_pm.c if not needed here any 
more so could be unexported.

Regards,

Tvrtko

>   		err = -EIO;
>   		goto out;
>   	}
> @@ -1184,7 +1186,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   	 * this is by declaring ourselves wedged.
>   	 */
>   	if (err)
> -		intel_gt_set_wedged(&i915->gt);
> +		intel_gt_set_wedged(gt);
>   
>   	for (id = 0; id < ARRAY_SIZE(requests); id++) {
>   		struct intel_context *ce;
> @@ -1298,7 +1300,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
>   	if (ret)
>   		goto err_gt;
>   
> -	ret = __intel_engines_record_defaults(dev_priv);
> +	ret = __intel_engines_record_defaults(&dev_priv->gt);
>   	if (ret)
>   		goto err_gt;
>   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/selftests: Teach switch_to_context() to use the context

[Tvrtko Ursulin]

On 22/10/2019 14:02, Chris Wilson wrote:
> The context details which engines to use, so use the ctx->engines[] to
> generate the requests to cause the context switch.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/selftests/i915_gem.c | 19 +++++++++----------
>   1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem.c b/drivers/gpu/drm/i915/selftests/i915_gem.c
> index 97f89f744ee2..84e7ca778b7b 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem.c
> @@ -15,16 +15,15 @@
>   #include "igt_flush_test.h"
>   #include "mock_drm.h"
>   
> -static int switch_to_context(struct drm_i915_private *i915,
> -			     struct i915_gem_context *ctx)
> +static int switch_to_context(struct i915_gem_context *ctx)
>   {
> -	struct intel_engine_cs *engine;
> -	enum intel_engine_id id;
> +	struct i915_gem_engines_iter it;
> +	struct intel_context *ce;
>   
> -	for_each_engine(engine, i915, id) {
> +	for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
>   		struct i915_request *rq;
>   
> -		rq = igt_request_alloc(ctx, engine);
> +		rq = intel_context_create_request(ce);
>   		if (IS_ERR(rq))
>   			return PTR_ERR(rq);
>   
> @@ -140,7 +139,7 @@ static int igt_gem_suspend(void *arg)
>   	err = -ENOMEM;
>   	ctx = live_context(i915, file);
>   	if (!IS_ERR(ctx))
> -		err = switch_to_context(i915, ctx);
> +		err = switch_to_context(ctx);
>   	if (err)
>   		goto out;
>   
> @@ -155,7 +154,7 @@ static int igt_gem_suspend(void *arg)
>   
>   	pm_resume(i915);
>   
> -	err = switch_to_context(i915, ctx);
> +	err = switch_to_context(ctx);
>   out:
>   	mock_file_free(i915, file);
>   	return err;
> @@ -175,7 +174,7 @@ static int igt_gem_hibernate(void *arg)
>   	err = -ENOMEM;
>   	ctx = live_context(i915, file);
>   	if (!IS_ERR(ctx))
> -		err = switch_to_context(i915, ctx);
> +		err = switch_to_context(ctx);
>   	if (err)
>   		goto out;
>   
> @@ -190,7 +189,7 @@ static int igt_gem_hibernate(void *arg)
>   
>   	pm_resume(i915);
>   
> -	err = switch_to_context(i915, ctx);
> +	err = switch_to_context(ctx);
>   out:
>   	mock_file_free(i915, file);
>   	return err;
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 1/2] drm/i915: Teach record_defaults to operate on the intel_gt

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-10-22 14:40:42)
> 
> On 22/10/2019 14:02, Chris Wilson wrote:
> > Again we wish to operate on the engines, which are owned by the
> > intel_gt. As such it is easier, and much more consistent, to pass the
> > intel_gt parameter.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem.c | 14 ++++++++------
> >   1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index ca64a0c9b762..b882988056bd 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -48,6 +48,7 @@
> >   #include "gt/intel_engine_user.h"
> >   #include "gt/intel_gt.h"
> >   #include "gt/intel_gt_pm.h"
> > +#include "gt/intel_gt_requests.h"
> >   #include "gt/intel_mocs.h"
> >   #include "gt/intel_reset.h"
> >   #include "gt/intel_renderstate.h"
> > @@ -1072,7 +1073,7 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
> >       intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> >   }
> >   
> > -static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> > +static int __intel_engines_record_defaults(struct intel_gt *gt)
> >   {
> >       struct i915_request *requests[I915_NUM_ENGINES] = {};
> >       struct intel_engine_cs *engine;
> > @@ -1088,7 +1089,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >        * from the same default HW values.
> >        */
> >   
> > -     for_each_engine(engine, i915, id) {
> > +     for_each_engine(engine, gt, id) {
> >               struct intel_context *ce;
> >               struct i915_request *rq;
> >   
> > @@ -1096,7 +1097,8 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >               GEM_BUG_ON(!engine->kernel_context);
> >               engine->serial++; /* force the kernel context switch */
> >   
> > -             ce = intel_context_create(i915->kernel_context, engine);
> > +             ce = intel_context_create(engine->kernel_context->gem_context,
> > +                                       engine);
> >               if (IS_ERR(ce)) {
> >                       err = PTR_ERR(ce);
> >                       goto out;
> > @@ -1125,7 +1127,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >       }
> >   
> >       /* Flush the default context image to memory, and enable powersaving. */
> > -     if (!i915_gem_load_power_context(i915)) {
> > +     if (intel_gt_wait_for_idle(gt, I915_GEM_IDLE_TIMEOUT) == -ETIME) {
> 
> What are the plans for i915_gem_load_power_context? It does a little bit 
> extra. But also becomes confined to i915_gem_pm.c if not needed here any 
> more so could be unexported.

It's to be subsumed entirely onto the gt. On resume, we simply call
intel_gt_resume() which does the power saving setup and ensures we have
a kernel_context primed. I'm still waiting on Andi's overhaul of GT
powersaving to land before pulling the plug.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Teach record_defaults to operate on the intel_gt

[Chris Wilson]

Quoting Chris Wilson (2019-10-22 14:56:42)
> Quoting Tvrtko Ursulin (2019-10-22 14:40:42)
> > 
> > On 22/10/2019 14:02, Chris Wilson wrote:
> > > Again we wish to operate on the engines, which are owned by the
> > > intel_gt. As such it is easier, and much more consistent, to pass the
> > > intel_gt parameter.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_gem.c | 14 ++++++++------
> > >   1 file changed, 8 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index ca64a0c9b762..b882988056bd 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -48,6 +48,7 @@
> > >   #include "gt/intel_engine_user.h"
> > >   #include "gt/intel_gt.h"
> > >   #include "gt/intel_gt_pm.h"
> > > +#include "gt/intel_gt_requests.h"
> > >   #include "gt/intel_mocs.h"
> > >   #include "gt/intel_reset.h"
> > >   #include "gt/intel_renderstate.h"
> > > @@ -1072,7 +1073,7 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
> > >       intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> > >   }
> > >   
> > > -static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> > > +static int __intel_engines_record_defaults(struct intel_gt *gt)
> > >   {
> > >       struct i915_request *requests[I915_NUM_ENGINES] = {};
> > >       struct intel_engine_cs *engine;
> > > @@ -1088,7 +1089,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> > >        * from the same default HW values.
> > >        */
> > >   
> > > -     for_each_engine(engine, i915, id) {
> > > +     for_each_engine(engine, gt, id) {
> > >               struct intel_context *ce;
> > >               struct i915_request *rq;
> > >   
> > > @@ -1096,7 +1097,8 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> > >               GEM_BUG_ON(!engine->kernel_context);
> > >               engine->serial++; /* force the kernel context switch */
> > >   
> > > -             ce = intel_context_create(i915->kernel_context, engine);
> > > +             ce = intel_context_create(engine->kernel_context->gem_context,
> > > +                                       engine);
> > >               if (IS_ERR(ce)) {
> > >                       err = PTR_ERR(ce);
> > >                       goto out;
> > > @@ -1125,7 +1127,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> > >       }
> > >   
> > >       /* Flush the default context image to memory, and enable powersaving. */
> > > -     if (!i915_gem_load_power_context(i915)) {
> > > +     if (intel_gt_wait_for_idle(gt, I915_GEM_IDLE_TIMEOUT) == -ETIME) {
> > 
> > What are the plans for i915_gem_load_power_context? It does a little bit 
> > extra. But also becomes confined to i915_gem_pm.c if not needed here any 
> > more so could be unexported.
> 
> It's to be subsumed entirely onto the gt. On resume, we simply call
> intel_gt_resume() which does the power saving setup and ensures we have
> a kernel_context primed. I'm still waiting on Andi's overhaul of GT
> powersaving to land before pulling the plug.

Hmm, the only caveat is that I think we need to have
intel_gt_pm_wait_for_idle() along here as some selftests demand that the 
system is completely idle when they start.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Teach record_defaults to operate on the intel_gt

[Chris Wilson]

Again we wish to operate on the engines, which are owned by the
intel_gt. As such it is easier, and much more consistent, to pass the
intel_gt parameter.

v2: Unexport i915_gem_load_power_context()

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_pm.c         |  7 +------
 drivers/gpu/drm/i915/gem/i915_gem_pm.h         |  1 -
 drivers/gpu/drm/i915/i915_gem.c                | 14 ++++++++------
 drivers/gpu/drm/i915/selftests/i915_selftest.c | 13 +++++++++++--
 4 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
index ee3279c76566..9bc0cf3139e3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
@@ -37,11 +37,6 @@ static bool switch_to_kernel_context_sync(struct intel_gt *gt)
 	return result;
 }
 
-bool i915_gem_load_power_context(struct drm_i915_private *i915)
-{
-	return switch_to_kernel_context_sync(&i915->gt);
-}
-
 static void user_forcewake(struct intel_gt *gt, bool suspend)
 {
 	int count = atomic_read(&gt->user_wakeref);
@@ -171,7 +166,7 @@ void i915_gem_resume(struct drm_i915_private *i915)
 	intel_uc_resume(&i915->gt.uc);
 
 	/* Always reload a context for powersaving. */
-	if (!i915_gem_load_power_context(i915))
+	if (!switch_to_kernel_context_sync(&i915->gt))
 		goto err_wedged;
 
 	user_forcewake(&i915->gt, false);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.h b/drivers/gpu/drm/i915/gem/i915_gem_pm.h
index a017572778d5..26b78dbdc225 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pm.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.h
@@ -12,7 +12,6 @@
 struct drm_i915_private;
 struct work_struct;
 
-bool i915_gem_load_power_context(struct drm_i915_private *i915);
 void i915_gem_resume(struct drm_i915_private *i915);
 
 void i915_gem_idle_work_handler(struct work_struct *work);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ca64a0c9b762..b882988056bd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -48,6 +48,7 @@
 #include "gt/intel_engine_user.h"
 #include "gt/intel_gt.h"
 #include "gt/intel_gt_pm.h"
+#include "gt/intel_gt_requests.h"
 #include "gt/intel_mocs.h"
 #include "gt/intel_reset.h"
 #include "gt/intel_renderstate.h"
@@ -1072,7 +1073,7 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
 	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
 }
 
-static int __intel_engines_record_defaults(struct drm_i915_private *i915)
+static int __intel_engines_record_defaults(struct intel_gt *gt)
 {
 	struct i915_request *requests[I915_NUM_ENGINES] = {};
 	struct intel_engine_cs *engine;
@@ -1088,7 +1089,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 	 * from the same default HW values.
 	 */
 
-	for_each_engine(engine, i915, id) {
+	for_each_engine(engine, gt, id) {
 		struct intel_context *ce;
 		struct i915_request *rq;
 
@@ -1096,7 +1097,8 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 		GEM_BUG_ON(!engine->kernel_context);
 		engine->serial++; /* force the kernel context switch */
 
-		ce = intel_context_create(i915->kernel_context, engine);
+		ce = intel_context_create(engine->kernel_context->gem_context,
+					  engine);
 		if (IS_ERR(ce)) {
 			err = PTR_ERR(ce);
 			goto out;
@@ -1125,7 +1127,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 	}
 
 	/* Flush the default context image to memory, and enable powersaving. */
-	if (!i915_gem_load_power_context(i915)) {
+	if (intel_gt_wait_for_idle(gt, I915_GEM_IDLE_TIMEOUT) == -ETIME) {
 		err = -EIO;
 		goto out;
 	}
@@ -1184,7 +1186,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 	 * this is by declaring ourselves wedged.
 	 */
 	if (err)
-		intel_gt_set_wedged(&i915->gt);
+		intel_gt_set_wedged(gt);
 
 	for (id = 0; id < ARRAY_SIZE(requests); id++) {
 		struct intel_context *ce;
@@ -1298,7 +1300,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_gt;
 
-	ret = __intel_engines_record_defaults(dev_priv);
+	ret = __intel_engines_record_defaults(&dev_priv->gt);
 	if (ret)
 		goto err_gt;
 
diff --git a/drivers/gpu/drm/i915/selftests/i915_selftest.c b/drivers/gpu/drm/i915/selftests/i915_selftest.c
index 825a8286cbe8..92c9193cdc85 100644
--- a/drivers/gpu/drm/i915/selftests/i915_selftest.c
+++ b/drivers/gpu/drm/i915/selftests/i915_selftest.c
@@ -23,8 +23,9 @@
 
 #include <linux/random.h>
 
-#include "../i915_drv.h"
-#include "../i915_selftest.h"
+#include "gt/intel_gt_pm.h"
+#include "i915_drv.h"
+#include "i915_selftest.h"
 
 #include "igt_flush_test.h"
 
@@ -256,6 +257,10 @@ int __i915_live_setup(void *data)
 {
 	struct drm_i915_private *i915 = data;
 
+	/* The selftests expect an idle system */
+	if (intel_gt_pm_wait_for_idle(&i915->gt))
+		return -EIO;
+
 	return intel_gt_terminally_wedged(&i915->gt);
 }
 
@@ -275,6 +280,10 @@ int __intel_gt_live_setup(void *data)
 {
 	struct intel_gt *gt = data;
 
+	/* The selftests expect an idle system */
+	if (intel_gt_pm_wait_for_idle(gt))
+		return -EIO;
+
 	return intel_gt_terminally_wedged(gt);
 }
 
-- 
2.24.0.rc0

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

Re: [PATCH 1/2] drm/i915: Teach record_defaults to operate on the intel_gt

[Tvrtko Ursulin]

On 22/10/2019 14:56, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-10-22 14:40:42)
>>
>> On 22/10/2019 14:02, Chris Wilson wrote:
>>> Again we wish to operate on the engines, which are owned by the
>>> intel_gt. As such it is easier, and much more consistent, to pass the
>>> intel_gt parameter.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_gem.c | 14 ++++++++------
>>>    1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>> index ca64a0c9b762..b882988056bd 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -48,6 +48,7 @@
>>>    #include "gt/intel_engine_user.h"
>>>    #include "gt/intel_gt.h"
>>>    #include "gt/intel_gt_pm.h"
>>> +#include "gt/intel_gt_requests.h"
>>>    #include "gt/intel_mocs.h"
>>>    #include "gt/intel_reset.h"
>>>    #include "gt/intel_renderstate.h"
>>> @@ -1072,7 +1073,7 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
>>>        intel_runtime_pm_put(&i915->runtime_pm, wakeref);
>>>    }
>>>    
>>> -static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>>> +static int __intel_engines_record_defaults(struct intel_gt *gt)
>>>    {
>>>        struct i915_request *requests[I915_NUM_ENGINES] = {};
>>>        struct intel_engine_cs *engine;
>>> @@ -1088,7 +1089,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>>>         * from the same default HW values.
>>>         */
>>>    
>>> -     for_each_engine(engine, i915, id) {
>>> +     for_each_engine(engine, gt, id) {
>>>                struct intel_context *ce;
>>>                struct i915_request *rq;
>>>    
>>> @@ -1096,7 +1097,8 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>>>                GEM_BUG_ON(!engine->kernel_context);
>>>                engine->serial++; /* force the kernel context switch */
>>>    
>>> -             ce = intel_context_create(i915->kernel_context, engine);
>>> +             ce = intel_context_create(engine->kernel_context->gem_context,
>>> +                                       engine);
>>>                if (IS_ERR(ce)) {
>>>                        err = PTR_ERR(ce);
>>>                        goto out;
>>> @@ -1125,7 +1127,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>>>        }
>>>    
>>>        /* Flush the default context image to memory, and enable powersaving. */
>>> -     if (!i915_gem_load_power_context(i915)) {
>>> +     if (intel_gt_wait_for_idle(gt, I915_GEM_IDLE_TIMEOUT) == -ETIME) {
>>
>> What are the plans for i915_gem_load_power_context? It does a little bit
>> extra. But also becomes confined to i915_gem_pm.c if not needed here any
>> more so could be unexported.
> 
> It's to be subsumed entirely onto the gt. On resume, we simply call
> intel_gt_resume() which does the power saving setup and ensures we have
> a kernel_context primed. I'm still waiting on Andi's overhaul of GT
> powersaving to land before pulling the plug.

Why it is bad, or not needed, to call intel_gt_pm_wait_for_idle at this 
point?

Regards,

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

Re: [PATCH 1/2] drm/i915: Teach record_defaults to operate on the intel_gt

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-10-22 16:43:17)
> 
> On 22/10/2019 14:56, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-10-22 14:40:42)
> >>
> >> On 22/10/2019 14:02, Chris Wilson wrote:
> >>> Again we wish to operate on the engines, which are owned by the
> >>> intel_gt. As such it is easier, and much more consistent, to pass the
> >>> intel_gt parameter.
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_gem.c | 14 ++++++++------
> >>>    1 file changed, 8 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>> index ca64a0c9b762..b882988056bd 100644
> >>> --- a/drivers/gpu/drm/i915/i915_gem.c
> >>> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >>> @@ -48,6 +48,7 @@
> >>>    #include "gt/intel_engine_user.h"
> >>>    #include "gt/intel_gt.h"
> >>>    #include "gt/intel_gt_pm.h"
> >>> +#include "gt/intel_gt_requests.h"
> >>>    #include "gt/intel_mocs.h"
> >>>    #include "gt/intel_reset.h"
> >>>    #include "gt/intel_renderstate.h"
> >>> @@ -1072,7 +1073,7 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
> >>>        intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> >>>    }
> >>>    
> >>> -static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >>> +static int __intel_engines_record_defaults(struct intel_gt *gt)
> >>>    {
> >>>        struct i915_request *requests[I915_NUM_ENGINES] = {};
> >>>        struct intel_engine_cs *engine;
> >>> @@ -1088,7 +1089,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >>>         * from the same default HW values.
> >>>         */
> >>>    
> >>> -     for_each_engine(engine, i915, id) {
> >>> +     for_each_engine(engine, gt, id) {
> >>>                struct intel_context *ce;
> >>>                struct i915_request *rq;
> >>>    
> >>> @@ -1096,7 +1097,8 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >>>                GEM_BUG_ON(!engine->kernel_context);
> >>>                engine->serial++; /* force the kernel context switch */
> >>>    
> >>> -             ce = intel_context_create(i915->kernel_context, engine);
> >>> +             ce = intel_context_create(engine->kernel_context->gem_context,
> >>> +                                       engine);
> >>>                if (IS_ERR(ce)) {
> >>>                        err = PTR_ERR(ce);
> >>>                        goto out;
> >>> @@ -1125,7 +1127,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >>>        }
> >>>    
> >>>        /* Flush the default context image to memory, and enable powersaving. */
> >>> -     if (!i915_gem_load_power_context(i915)) {
> >>> +     if (intel_gt_wait_for_idle(gt, I915_GEM_IDLE_TIMEOUT) == -ETIME) {
> >>
> >> What are the plans for i915_gem_load_power_context? It does a little bit
> >> extra. But also becomes confined to i915_gem_pm.c if not needed here any
> >> more so could be unexported.
> > 
> > It's to be subsumed entirely onto the gt. On resume, we simply call
> > intel_gt_resume() which does the power saving setup and ensures we have
> > a kernel_context primed. I'm still waiting on Andi's overhaul of GT
> > powersaving to land before pulling the plug.
> 
> Why it is bad, or not needed, to call intel_gt_pm_wait_for_idle at this 
> point?

It's just an extra delay that isn't required before bringing up
userspace. It should be a miniscule delay, but it's the principle of the
thing! The implicit dependency on the selftests of a completely idle
system has its own shortcomings.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Teach record_defaults to operate on the intel_gt

[Tvrtko Ursulin]

On 22/10/2019 15:19, Chris Wilson wrote:
> Again we wish to operate on the engines, which are owned by the
> intel_gt. As such it is easier, and much more consistent, to pass the
> intel_gt parameter.
> 
> v2: Unexport i915_gem_load_power_context()
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_pm.c         |  7 +------
>   drivers/gpu/drm/i915/gem/i915_gem_pm.h         |  1 -
>   drivers/gpu/drm/i915/i915_gem.c                | 14 ++++++++------
>   drivers/gpu/drm/i915/selftests/i915_selftest.c | 13 +++++++++++--
>   4 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> index ee3279c76566..9bc0cf3139e3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> @@ -37,11 +37,6 @@ static bool switch_to_kernel_context_sync(struct intel_gt *gt)
>   	return result;
>   }
>   
> -bool i915_gem_load_power_context(struct drm_i915_private *i915)
> -{
> -	return switch_to_kernel_context_sync(&i915->gt);
> -}
> -
>   static void user_forcewake(struct intel_gt *gt, bool suspend)
>   {
>   	int count = atomic_read(&gt->user_wakeref);
> @@ -171,7 +166,7 @@ void i915_gem_resume(struct drm_i915_private *i915)
>   	intel_uc_resume(&i915->gt.uc);
>   
>   	/* Always reload a context for powersaving. */
> -	if (!i915_gem_load_power_context(i915))
> +	if (!switch_to_kernel_context_sync(&i915->gt))
>   		goto err_wedged;
>   
>   	user_forcewake(&i915->gt, false);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.h b/drivers/gpu/drm/i915/gem/i915_gem_pm.h
> index a017572778d5..26b78dbdc225 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.h
> @@ -12,7 +12,6 @@
>   struct drm_i915_private;
>   struct work_struct;
>   
> -bool i915_gem_load_power_context(struct drm_i915_private *i915);
>   void i915_gem_resume(struct drm_i915_private *i915);
>   
>   void i915_gem_idle_work_handler(struct work_struct *work);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ca64a0c9b762..b882988056bd 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -48,6 +48,7 @@
>   #include "gt/intel_engine_user.h"
>   #include "gt/intel_gt.h"
>   #include "gt/intel_gt_pm.h"
> +#include "gt/intel_gt_requests.h"
>   #include "gt/intel_mocs.h"
>   #include "gt/intel_reset.h"
>   #include "gt/intel_renderstate.h"
> @@ -1072,7 +1073,7 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
>   	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
>   }
>   
> -static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> +static int __intel_engines_record_defaults(struct intel_gt *gt)
>   {
>   	struct i915_request *requests[I915_NUM_ENGINES] = {};
>   	struct intel_engine_cs *engine;
> @@ -1088,7 +1089,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   	 * from the same default HW values.
>   	 */
>   
> -	for_each_engine(engine, i915, id) {
> +	for_each_engine(engine, gt, id) {
>   		struct intel_context *ce;
>   		struct i915_request *rq;
>   
> @@ -1096,7 +1097,8 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   		GEM_BUG_ON(!engine->kernel_context);
>   		engine->serial++; /* force the kernel context switch */
>   
> -		ce = intel_context_create(i915->kernel_context, engine);
> +		ce = intel_context_create(engine->kernel_context->gem_context,
> +					  engine);
>   		if (IS_ERR(ce)) {
>   			err = PTR_ERR(ce);
>   			goto out;
> @@ -1125,7 +1127,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   	}
>   
>   	/* Flush the default context image to memory, and enable powersaving. */
> -	if (!i915_gem_load_power_context(i915)) {
> +	if (intel_gt_wait_for_idle(gt, I915_GEM_IDLE_TIMEOUT) == -ETIME) {
>   		err = -EIO;
>   		goto out;
>   	}
> @@ -1184,7 +1186,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   	 * this is by declaring ourselves wedged.
>   	 */
>   	if (err)
> -		intel_gt_set_wedged(&i915->gt);
> +		intel_gt_set_wedged(gt);
>   
>   	for (id = 0; id < ARRAY_SIZE(requests); id++) {
>   		struct intel_context *ce;
> @@ -1298,7 +1300,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
>   	if (ret)
>   		goto err_gt;
>   
> -	ret = __intel_engines_record_defaults(dev_priv);
> +	ret = __intel_engines_record_defaults(&dev_priv->gt);
>   	if (ret)
>   		goto err_gt;
>   
> diff --git a/drivers/gpu/drm/i915/selftests/i915_selftest.c b/drivers/gpu/drm/i915/selftests/i915_selftest.c
> index 825a8286cbe8..92c9193cdc85 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_selftest.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_selftest.c
> @@ -23,8 +23,9 @@
>   
>   #include <linux/random.h>
>   
> -#include "../i915_drv.h"
> -#include "../i915_selftest.h"
> +#include "gt/intel_gt_pm.h"
> +#include "i915_drv.h"
> +#include "i915_selftest.h"
>   
>   #include "igt_flush_test.h"
>   
> @@ -256,6 +257,10 @@ int __i915_live_setup(void *data)
>   {
>   	struct drm_i915_private *i915 = data;
>   
> +	/* The selftests expect an idle system */
> +	if (intel_gt_pm_wait_for_idle(&i915->gt))
> +		return -EIO;
> +
>   	return intel_gt_terminally_wedged(&i915->gt);
>   }
>   
> @@ -275,6 +280,10 @@ int __intel_gt_live_setup(void *data)
>   {
>   	struct intel_gt *gt = data;
>   
> +	/* The selftests expect an idle system */
> +	if (intel_gt_pm_wait_for_idle(gt))
> +		return -EIO;
> +
>   	return intel_gt_terminally_wedged(gt);
>   }
>   
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

✗ Fi.CI.BAT: failure for series starting with [v2] drm/i915: Teach record_defaults to operate on the intel_gt (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2] drm/i915: Teach record_defaults to operate on the intel_gt (rev2)
URL   : https://patchwork.freedesktop.org/series/68391/
State : failure

== Summary ==

Applying: drm/i915: Teach record_defaults to operate on the intel_gt
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/gem/i915_gem_pm.c
M	drivers/gpu/drm/i915/gem/i915_gem_pm.h
M	drivers/gpu/drm/i915/i915_gem.c
M	drivers/gpu/drm/i915/selftests/i915_selftest.c
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: drm/i915/selftests: Teach switch_to_context() to use the context
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/selftests/i915_gem.c
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.

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