Re: [Intel-gfx] [PATCH 8/8] drm/i915: Move submission tasklet to i915_sched_engine
On 6/15/2021 3:43 PM, Matthew Brost wrote:
The submission tasklet operates on i915_sched_engine, thus it is the
correct place for it.
v3:
(Jason Ekstrand)
Change sched_engine->engine to a void* private data pointer
Add kernel doc
v4:
(Daniele)
Update private_data comment
Set queue_priority_hint in kick_execlists
v5:
(CI)
Rebase and fix build error
Signed-off-by: Matthew Brost
Reviewed-by: Daniele Ceraolo Spurio
Daniele
---
drivers/gpu/drm/i915/gt/intel_engine.h| 14
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 12 +--
drivers/gpu/drm/i915/gt/intel_engine_types.h | 5 --
.../drm/i915/gt/intel_execlists_submission.c | 84 ++-
drivers/gpu/drm/i915/gt/mock_engine.c | 1 +
drivers/gpu/drm/i915/gt/selftest_execlists.c | 14 ++--
drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 2 +-
drivers/gpu/drm/i915/gt/selftest_lrc.c| 6 +-
drivers/gpu/drm/i915/gt/selftest_reset.c | 2 +-
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 25 +++---
drivers/gpu/drm/i915/i915_scheduler.c | 1 +
drivers/gpu/drm/i915/i915_scheduler.h | 14
drivers/gpu/drm/i915/i915_scheduler_types.h | 10 +++
13 files changed, 100 insertions(+), 90 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
b/drivers/gpu/drm/i915/gt/intel_engine.h
index a8b2174b4395..988d9688ae4d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -123,20 +123,6 @@ execlists_active(const struct intel_engine_execlists
*execlists)
return active;
}
-static inline void
-execlists_active_lock_bh(struct intel_engine_execlists *execlists)
-{
- local_bh_disable(); /* prevent local softirq and lock recursion */
- tasklet_lock(>tasklet);
-}
-
-static inline void
-execlists_active_unlock_bh(struct intel_engine_execlists *execlists)
-{
- tasklet_unlock(>tasklet);
- local_bh_enable(); /* restore softirq, and kick ksoftirqd! */
-}
-
struct i915_request *
execlists_unwind_incomplete_requests(struct intel_engine_execlists
*execlists);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 7ff2640aa74a..67939ee0d68f 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -713,6 +713,7 @@ static int engine_setup_common(struct intel_engine_cs
*engine)
err = -ENOMEM;
goto err_sched_engine;
}
+ engine->sched_engine->private_data = engine;
err = intel_engine_init_cmd_parser(engine);
if (err)
@@ -937,7 +938,6 @@ int intel_engines_init(struct intel_gt *gt)
void intel_engine_cleanup_common(struct intel_engine_cs *engine)
{
GEM_BUG_ON(!list_empty(>sched_engine->requests));
- tasklet_kill(>execlists.tasklet); /* flush the callback */
i915_sched_engine_put(engine->sched_engine);
intel_breadcrumbs_free(engine->breadcrumbs);
@@ -1223,7 +1223,7 @@ static bool ring_is_idle(struct intel_engine_cs *engine)
void __intel_engine_flush_submission(struct intel_engine_cs *engine, bool sync)
{
- struct tasklet_struct *t = >execlists.tasklet;
+ struct tasklet_struct *t = >sched_engine->tasklet;
if (!t->callback)
return;
@@ -1484,8 +1484,8 @@ static void intel_engine_print_registers(struct
intel_engine_cs *engine,
drm_printf(m, "\tExeclist tasklet queued? %s (%s), preempt? %s, timeslice? %s\n",
yesno(test_bit(TASKLET_STATE_SCHED,
- >execlists.tasklet.state)),
-
enableddisabled(!atomic_read(>execlists.tasklet.count)),
+
>sched_engine->tasklet.state)),
+
enableddisabled(!atomic_read(>sched_engine->tasklet.count)),
repr_timer(>execlists.preempt),
repr_timer(>execlists.timer));
@@ -1509,7 +1509,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine,
idx, hws[idx * 2], hws[idx * 2 + 1]);
}
- execlists_active_lock_bh(execlists);
+ i915_sched_engine_active_lock_bh(engine->sched_engine);
rcu_read_lock();
for (port = execlists->active; (rq = *port); port++) {
char hdr[160];
@@ -1540,7 +1540,7 @@ static void intel_engine_print_registers(struct
intel_engine_cs *engine,
i915_request_show(m, rq, hdr, 0);
}
rcu_read_unlock();
- execlists_active_unlock_bh(execlists);
+ i915_sched_engine_active_unlock_bh(engine->sched_engine);
} else if (GRAPHICS_VER(dev_priv) > 6) {
drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n",
ENGINE_READ(engine, RING_PP_DIR_BASE));
diff
Re: [Intel-gfx] [PATCH 8/8] drm/i915: Move submission tasklet to i915_sched_engine
On 6/15/2021 1:59 PM, Matthew Brost wrote:
The submission tasklet operates on i915_sched_engine, thus it is the
correct place for it.
v3:
(Jason Ekstrand)
Change sched_engine->engine to a void* private data pointer
Add kernel doc
v4:
(Daniele)
Update private_data comment
Set queue_priority_hint in kick_execlists
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gt/intel_engine.h| 14
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 12 +--
drivers/gpu/drm/i915/gt/intel_engine_types.h | 5 --
.../drm/i915/gt/intel_execlists_submission.c | 84 ++-
drivers/gpu/drm/i915/gt/mock_engine.c | 1 +
drivers/gpu/drm/i915/gt/selftest_execlists.c | 16 ++--
drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 2 +-
drivers/gpu/drm/i915/gt/selftest_lrc.c| 6 +-
drivers/gpu/drm/i915/gt/selftest_reset.c | 2 +-
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 25 +++---
drivers/gpu/drm/i915/i915_scheduler.c | 1 +
drivers/gpu/drm/i915/i915_scheduler.h | 14
drivers/gpu/drm/i915/i915_scheduler_types.h | 10 +++
13 files changed, 101 insertions(+), 91 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
b/drivers/gpu/drm/i915/gt/intel_engine.h
index a8b2174b4395..988d9688ae4d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -123,20 +123,6 @@ execlists_active(const struct intel_engine_execlists
*execlists)
return active;
}
-static inline void
-execlists_active_lock_bh(struct intel_engine_execlists *execlists)
-{
- local_bh_disable(); /* prevent local softirq and lock recursion */
- tasklet_lock(>tasklet);
-}
-
-static inline void
-execlists_active_unlock_bh(struct intel_engine_execlists *execlists)
-{
- tasklet_unlock(>tasklet);
- local_bh_enable(); /* restore softirq, and kick ksoftirqd! */
-}
-
struct i915_request *
execlists_unwind_incomplete_requests(struct intel_engine_execlists
*execlists);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 7ff2640aa74a..67939ee0d68f 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -713,6 +713,7 @@ static int engine_setup_common(struct intel_engine_cs
*engine)
err = -ENOMEM;
goto err_sched_engine;
}
+ engine->sched_engine->private_data = engine;
I see you didn't move this to the backend in this rev. Not a blocker now
since it doesn't matter functionally, but please move it as a follow up.
Reviewed-by: Daniele Ceraolo Spurio
Daniele
||
err = intel_engine_init_cmd_parser(engine);
if (err)
@@ -937,7 +938,6 @@ int intel_engines_init(struct intel_gt *gt)
void intel_engine_cleanup_common(struct intel_engine_cs *engine)
{
GEM_BUG_ON(!list_empty(>sched_engine->requests));
- tasklet_kill(>execlists.tasklet); /* flush the callback */
i915_sched_engine_put(engine->sched_engine);
intel_breadcrumbs_free(engine->breadcrumbs);
@@ -1223,7 +1223,7 @@ static bool ring_is_idle(struct intel_engine_cs *engine)
void __intel_engine_flush_submission(struct intel_engine_cs *engine, bool sync)
{
- struct tasklet_struct *t = >execlists.tasklet;
+ struct tasklet_struct *t = >sched_engine->tasklet;
if (!t->callback)
return;
@@ -1484,8 +1484,8 @@ static void intel_engine_print_registers(struct
intel_engine_cs *engine,
drm_printf(m, "\tExeclist tasklet queued? %s (%s), preempt? %s, timeslice? %s\n",
yesno(test_bit(TASKLET_STATE_SCHED,
- >execlists.tasklet.state)),
-
enableddisabled(!atomic_read(>execlists.tasklet.count)),
+
>sched_engine->tasklet.state)),
+
enableddisabled(!atomic_read(>sched_engine->tasklet.count)),
repr_timer(>execlists.preempt),
repr_timer(>execlists.timer));
@@ -1509,7 +1509,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine,
idx, hws[idx * 2], hws[idx * 2 + 1]);
}
- execlists_active_lock_bh(execlists);
+ i915_sched_engine_active_lock_bh(engine->sched_engine);
rcu_read_lock();
for (port = execlists->active; (rq = *port); port++) {
char hdr[160];
@@ -1540,7 +1540,7 @@ static void intel_engine_print_registers(struct
intel_engine_cs *engine,
i915_request_show(m, rq, hdr, 0);
}
rcu_read_unlock();
- execlists_active_unlock_bh(execlists);
+ i915_sched_engine_active_unlock_bh(engine->sched_engine);
} else if (GRAPHICS_VER(dev_priv) > 6) {
Re: [Intel-gfx] [PATCH 8/8] drm/i915: Move submission tasklet to i915_sched_engine
On Mon, Jun 14, 2021 at 06:05:19PM -0700, Daniele Ceraolo Spurio wrote:
>
>
> On 6/8/2021 12:17 PM, Matthew Brost wrote:
> > The submission tasklet operates on i915_sched_engine, thus it is the
> > correct place for it.
> >
> > v3:
> > (Jason Ekstrand)
> >Change sched_engine->engine to a void* private data pointer
> >Add kernel doc
> >
> > Signed-off-by: Matthew Brost
> > ---
> > drivers/gpu/drm/i915/gt/intel_engine.h| 14 ---
> > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 12 +--
> > drivers/gpu/drm/i915/gt/intel_engine_types.h | 5 --
> > .../drm/i915/gt/intel_execlists_submission.c | 86 ++-
> > drivers/gpu/drm/i915/gt/mock_engine.c | 1 +
> > drivers/gpu/drm/i915/gt/selftest_execlists.c | 16 ++--
> > drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 2 +-
> > drivers/gpu/drm/i915/gt/selftest_lrc.c| 6 +-
> > drivers/gpu/drm/i915/gt/selftest_reset.c | 2 +-
> > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 25 +++---
> > drivers/gpu/drm/i915/i915_scheduler.c | 1 +
> > drivers/gpu/drm/i915/i915_scheduler.h | 14 +++
> > drivers/gpu/drm/i915/i915_scheduler_types.h | 10 +++
> > 13 files changed, 101 insertions(+), 93 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
> > b/drivers/gpu/drm/i915/gt/intel_engine.h
> > index a8b2174b4395..988d9688ae4d 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> > @@ -123,20 +123,6 @@ execlists_active(const struct intel_engine_execlists
> > *execlists)
> > return active;
> > }
> > -static inline void
> > -execlists_active_lock_bh(struct intel_engine_execlists *execlists)
> > -{
> > - local_bh_disable(); /* prevent local softirq and lock recursion */
> > - tasklet_lock(>tasklet);
> > -}
> > -
> > -static inline void
> > -execlists_active_unlock_bh(struct intel_engine_execlists *execlists)
> > -{
> > - tasklet_unlock(>tasklet);
> > - local_bh_enable(); /* restore softirq, and kick ksoftirqd! */
> > -}
> > -
> > struct i915_request *
> > execlists_unwind_incomplete_requests(struct intel_engine_execlists
> > *execlists);
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > index 7ff2640aa74a..67939ee0d68f 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > @@ -713,6 +713,7 @@ static int engine_setup_common(struct intel_engine_cs
> > *engine)
> > err = -ENOMEM;
> > goto err_sched_engine;
> > }
> > + engine->sched_engine->private_data = engine;
>
> Given that the private_data is back-end specific, IMO it should be set in
> one of the back-end functions, like set_default_submission().
>
That likely works.
> > err = intel_engine_init_cmd_parser(engine);
> > if (err)
> > @@ -937,7 +938,6 @@ int intel_engines_init(struct intel_gt *gt)
> > void intel_engine_cleanup_common(struct intel_engine_cs *engine)
> > {
> > GEM_BUG_ON(!list_empty(>sched_engine->requests));
> > - tasklet_kill(>execlists.tasklet); /* flush the callback */
> > i915_sched_engine_put(engine->sched_engine);
> > intel_breadcrumbs_free(engine->breadcrumbs);
> > @@ -1223,7 +1223,7 @@ static bool ring_is_idle(struct intel_engine_cs
> > *engine)
> > void __intel_engine_flush_submission(struct intel_engine_cs *engine, bool
> > sync)
> > {
> > - struct tasklet_struct *t = >execlists.tasklet;
> > + struct tasklet_struct *t = >sched_engine->tasklet;
> > if (!t->callback)
> > return;
> > @@ -1484,8 +1484,8 @@ static void intel_engine_print_registers(struct
> > intel_engine_cs *engine,
> > drm_printf(m, "\tExeclist tasklet queued? %s (%s), preempt? %s,
> > timeslice? %s\n",
> >yesno(test_bit(TASKLET_STATE_SCHED,
> > - >execlists.tasklet.state)),
> > -
> > enableddisabled(!atomic_read(>execlists.tasklet.count)),
> > +
> > >sched_engine->tasklet.state)),
> > +
> > enableddisabled(!atomic_read(>sched_engine->tasklet.count)),
> >repr_timer(>execlists.preempt),
> >repr_timer(>execlists.timer));
> > @@ -1509,7 +1509,7 @@ static void intel_engine_print_registers(struct
> > intel_engine_cs *engine,
> >idx, hws[idx * 2], hws[idx * 2 + 1]);
> > }
> > - execlists_active_lock_bh(execlists);
> > + i915_sched_engine_active_lock_bh(engine->sched_engine);
> > rcu_read_lock();
> > for (port = execlists->active; (rq = *port); port++) {
> > char hdr[160];
> > @@ -1540,7 +1540,7 @@ static void intel_engine_print_registers(struct
> > intel_engine_cs *engine,
> > i915_request_show(m, rq, hdr, 0);
> >
Re: [Intel-gfx] [PATCH 8/8] drm/i915: Move submission tasklet to i915_sched_engine
On 6/8/2021 12:17 PM, Matthew Brost wrote:
The submission tasklet operates on i915_sched_engine, thus it is the
correct place for it.
v3:
(Jason Ekstrand)
Change sched_engine->engine to a void* private data pointer
Add kernel doc
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gt/intel_engine.h| 14 ---
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 12 +--
drivers/gpu/drm/i915/gt/intel_engine_types.h | 5 --
.../drm/i915/gt/intel_execlists_submission.c | 86 ++-
drivers/gpu/drm/i915/gt/mock_engine.c | 1 +
drivers/gpu/drm/i915/gt/selftest_execlists.c | 16 ++--
drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 2 +-
drivers/gpu/drm/i915/gt/selftest_lrc.c| 6 +-
drivers/gpu/drm/i915/gt/selftest_reset.c | 2 +-
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 25 +++---
drivers/gpu/drm/i915/i915_scheduler.c | 1 +
drivers/gpu/drm/i915/i915_scheduler.h | 14 +++
drivers/gpu/drm/i915/i915_scheduler_types.h | 10 +++
13 files changed, 101 insertions(+), 93 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
b/drivers/gpu/drm/i915/gt/intel_engine.h
index a8b2174b4395..988d9688ae4d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -123,20 +123,6 @@ execlists_active(const struct intel_engine_execlists
*execlists)
return active;
}
-static inline void
-execlists_active_lock_bh(struct intel_engine_execlists *execlists)
-{
- local_bh_disable(); /* prevent local softirq and lock recursion */
- tasklet_lock(>tasklet);
-}
-
-static inline void
-execlists_active_unlock_bh(struct intel_engine_execlists *execlists)
-{
- tasklet_unlock(>tasklet);
- local_bh_enable(); /* restore softirq, and kick ksoftirqd! */
-}
-
struct i915_request *
execlists_unwind_incomplete_requests(struct intel_engine_execlists
*execlists);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 7ff2640aa74a..67939ee0d68f 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -713,6 +713,7 @@ static int engine_setup_common(struct intel_engine_cs
*engine)
err = -ENOMEM;
goto err_sched_engine;
}
+ engine->sched_engine->private_data = engine;
Given that the private_data is back-end specific, IMO it should be set
in one of the back-end functions, like set_default_submission().
err = intel_engine_init_cmd_parser(engine);
if (err)
@@ -937,7 +938,6 @@ int intel_engines_init(struct intel_gt *gt)
void intel_engine_cleanup_common(struct intel_engine_cs *engine)
{
GEM_BUG_ON(!list_empty(>sched_engine->requests));
- tasklet_kill(>execlists.tasklet); /* flush the callback */
i915_sched_engine_put(engine->sched_engine);
intel_breadcrumbs_free(engine->breadcrumbs);
@@ -1223,7 +1223,7 @@ static bool ring_is_idle(struct intel_engine_cs *engine)
void __intel_engine_flush_submission(struct intel_engine_cs *engine, bool sync)
{
- struct tasklet_struct *t = >execlists.tasklet;
+ struct tasklet_struct *t = >sched_engine->tasklet;
if (!t->callback)
return;
@@ -1484,8 +1484,8 @@ static void intel_engine_print_registers(struct
intel_engine_cs *engine,
drm_printf(m, "\tExeclist tasklet queued? %s (%s), preempt? %s, timeslice? %s\n",
yesno(test_bit(TASKLET_STATE_SCHED,
- >execlists.tasklet.state)),
-
enableddisabled(!atomic_read(>execlists.tasklet.count)),
+
>sched_engine->tasklet.state)),
+
enableddisabled(!atomic_read(>sched_engine->tasklet.count)),
repr_timer(>execlists.preempt),
repr_timer(>execlists.timer));
@@ -1509,7 +1509,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine,
idx, hws[idx * 2], hws[idx * 2 + 1]);
}
- execlists_active_lock_bh(execlists);
+ i915_sched_engine_active_lock_bh(engine->sched_engine);
rcu_read_lock();
for (port = execlists->active; (rq = *port); port++) {
char hdr[160];
@@ -1540,7 +1540,7 @@ static void intel_engine_print_registers(struct
intel_engine_cs *engine,
i915_request_show(m, rq, hdr, 0);
}
rcu_read_unlock();
- execlists_active_unlock_bh(execlists);
+ i915_sched_engine_active_unlock_bh(engine->sched_engine);
} else if (GRAPHICS_VER(dev_priv) > 6) {
drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n",
ENGINE_READ(engine, RING_PP_DIR_BASE));
diff --git
