| Message ID | 20190703091726.11690-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/15] drm/i915/selftests: Common live setup/teardown | expand |
On 03/07/2019 10:17, Chris Wilson wrote: > During post-reset resume, we call intel_mocs_init_engine to reinitialise > the MOCS registers. Suprisingly, especially when enhanced by lockdep, > the acquisition of the forcewake lock around each register write takes a > substantial portion of the reset time. We don't need to use the > individual forcewake here as we can assume that the caller is holding a > blanket forcewake for the reset&resume and the resume is serialised. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/gt/intel_mocs.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c > index ae6cbf0d517c..290a5e9b90b9 100644 > --- a/drivers/gpu/drm/i915/gt/intel_mocs.c > +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c > @@ -346,6 +346,9 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine) > unsigned int index; > u32 unused_value; > > + /* Called under a blanket forcewake */ > + assert_forcewakes_active(uncore, FORCEWAKE_ALL); > + Assert is strictly speaking a bit weak since forcewake status can theoretically change until the actual access below. But in our current code we indeed hold it for the whole reset. I don't have any actual ideas on how to fundamentally improve the assert. Thought to have it after the writes is the only thing which came to mind. Thoughts? Regards, Tvrtko > if (!get_mocs_settings(gt, &table)) > return; > > @@ -355,16 +358,16 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine) > for (index = 0; index < table.size; index++) { > u32 value = get_entry_control(&table, index); > > - intel_uncore_write(uncore, > - mocs_register(engine->id, index), > - value); > + intel_uncore_write_fw(uncore, > + mocs_register(engine->id, index), > + value); > } > > /* All remaining entries are also unused */ > for (; index < table.n_entries; index++) > - intel_uncore_write(uncore, > - mocs_register(engine->id, index), > - unused_value); > + intel_uncore_write_fw(uncore, > + mocs_register(engine->id, index), > + unused_value); > } > > /** >
Quoting Tvrtko Ursulin (2019-07-03 12:23:52) > > On 03/07/2019 10:17, Chris Wilson wrote: > > During post-reset resume, we call intel_mocs_init_engine to reinitialise > > the MOCS registers. Suprisingly, especially when enhanced by lockdep, > > the acquisition of the forcewake lock around each register write takes a > > substantial portion of the reset time. We don't need to use the > > individual forcewake here as we can assume that the caller is holding a > > blanket forcewake for the reset&resume and the resume is serialised. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/gt/intel_mocs.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c > > index ae6cbf0d517c..290a5e9b90b9 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_mocs.c > > +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c > > @@ -346,6 +346,9 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine) > > unsigned int index; > > u32 unused_value; > > > > + /* Called under a blanket forcewake */ > > + assert_forcewakes_active(uncore, FORCEWAKE_ALL); > > + > > Assert is strictly speaking a bit weak since forcewake status can > theoretically change until the actual access below. But in our current > code we indeed hold it for the whole reset. You want to distinguish between an explicit hold by the caller and the auto. > I don't have any actual ideas on how to fundamentally improve the > assert. Thought to have it after the writes is the only thing which came > to mind. Thoughts? I definitely prefer having it upfront to document the function preconditions, and so would prefer if the assert actually did assert an explicit forcewake as it is meant to do :) -Chris
On 03/07/2019 12:47, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-07-03 12:23:52) >> >> On 03/07/2019 10:17, Chris Wilson wrote: >>> During post-reset resume, we call intel_mocs_init_engine to reinitialise >>> the MOCS registers. Suprisingly, especially when enhanced by lockdep, >>> the acquisition of the forcewake lock around each register write takes a >>> substantial portion of the reset time. We don't need to use the >>> individual forcewake here as we can assume that the caller is holding a >>> blanket forcewake for the reset&resume and the resume is serialised. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> --- >>> drivers/gpu/drm/i915/gt/intel_mocs.c | 15 +++++++++------ >>> 1 file changed, 9 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c >>> index ae6cbf0d517c..290a5e9b90b9 100644 >>> --- a/drivers/gpu/drm/i915/gt/intel_mocs.c >>> +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c >>> @@ -346,6 +346,9 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine) >>> unsigned int index; >>> u32 unused_value; >>> >>> + /* Called under a blanket forcewake */ >>> + assert_forcewakes_active(uncore, FORCEWAKE_ALL); >>> + >> >> Assert is strictly speaking a bit weak since forcewake status can >> theoretically change until the actual access below. But in our current >> code we indeed hold it for the whole reset. > > You want to distinguish between an explicit hold by the caller and the > auto. Yes, we just don't have a way of distinguishing if the caller is holding or they were held and about to be released. >> I don't have any actual ideas on how to fundamentally improve the >> assert. Thought to have it after the writes is the only thing which came >> to mind. Thoughts? > > I definitely prefer having it upfront to document the function > preconditions, and so would prefer if the assert actually did assert an > explicit forcewake as it is meant to do :) Probably better yes. I was just thinking out loud. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c index ae6cbf0d517c..290a5e9b90b9 100644 --- a/drivers/gpu/drm/i915/gt/intel_mocs.c +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c @@ -346,6 +346,9 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine) unsigned int index; u32 unused_value; + /* Called under a blanket forcewake */ + assert_forcewakes_active(uncore, FORCEWAKE_ALL); + if (!get_mocs_settings(gt, &table)) return; @@ -355,16 +358,16 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine) for (index = 0; index < table.size; index++) { u32 value = get_entry_control(&table, index); - intel_uncore_write(uncore, - mocs_register(engine->id, index), - value); + intel_uncore_write_fw(uncore, + mocs_register(engine->id, index), + value); } /* All remaining entries are also unused */ for (; index < table.n_entries; index++) - intel_uncore_write(uncore, - mocs_register(engine->id, index), - unused_value); + intel_uncore_write_fw(uncore, + mocs_register(engine->id, index), + unused_value); } /**
During post-reset resume, we call intel_mocs_init_engine to reinitialise the MOCS registers. Suprisingly, especially when enhanced by lockdep, the acquisition of the forcewake lock around each register write takes a substantial portion of the reset time. We don't need to use the individual forcewake here as we can assume that the caller is holding a blanket forcewake for the reset&resume and the resume is serialised. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/gt/intel_mocs.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)