[Intel-gfx] [PATCH 2/3] drm/i915/gt: create workqueue dedicated to wake references

Fri May 12 09:32:19 UTC 2023

On 12/05/2023 10:10, Coelho, Luciano wrote:
> On Fri, 2023-05-12 at 10:04 +0100, Tvrtko Ursulin wrote:
>> On 11/05/2023 09:20, Luca Coelho wrote:
>>> Add a work queue in the intel_wakeref structure to be used exclusively
>>> by the wake reference mechanism.  This is needed in order to avoid
>>> using the system workqueue and relying on flush_scheduled_work().
>>>
>>> Cc: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> Cc: Jani Nikula <jani.nikula at intel.com>
>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> Signed-off-by: Luca Coelho <luciano.coelho at intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_engine_cs.c |  7 ++++++-
>>>    drivers/gpu/drm/i915/gt/intel_engine_pm.c | 15 ++++++++++++--
>>>    drivers/gpu/drm/i915/gt/intel_engine_pm.h |  3 ++-
>>>    drivers/gpu/drm/i915/gt/mock_engine.c     |  8 +++++++-
>>>    drivers/gpu/drm/i915/intel_wakeref.c      | 21 ++++++++++++++-----
>>>    drivers/gpu/drm/i915/intel_wakeref.h      | 25 +++++++++++++++--------
>>>    6 files changed, 60 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>> index 0aff5bb13c53..6505bfa70cd0 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>> @@ -1290,7 +1290,11 @@ static int engine_setup_common(struct intel_engine_cs *engine)
>>>    		goto err_cmd_parser;
>>>    
>>>    	intel_engine_init_execlists(engine);
>>> -	intel_engine_init__pm(engine);
>>> +
>>> +	err = intel_engine_init__pm(engine);
>>> +	if (err)
>>> +		goto err_cmd_parser;
>>> +
>>>    	intel_engine_init_retire(engine);
>>>    
>>>    	/* Use the whole device by default */
>>> @@ -1525,6 +1529,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
>>>    {
>>>    	GEM_BUG_ON(!list_empty(&engine->sched_engine->requests));
>>>    
>>> +	intel_engine_destroy__pm(engine);
>>>    	i915_sched_engine_put(engine->sched_engine);
>>>    	intel_breadcrumbs_put(engine->breadcrumbs);
>>>    
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
>>> index ee531a5c142c..859b62cf660f 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
>>> @@ -294,14 +294,25 @@ static const struct intel_wakeref_ops wf_ops = {
>>>    	.put = __engine_park,
>>>    };
>>>    
>>> -void intel_engine_init__pm(struct intel_engine_cs *engine)
>>> +int intel_engine_init__pm(struct intel_engine_cs *engine)
>>>    {
>>>    	struct intel_runtime_pm *rpm = engine->uncore->rpm;
>>> +	int err;
>>> +
>>> +	err = intel_wakeref_init(&engine->wakeref, rpm, &wf_ops);
>>> +	if (err)
>>> +		return err;
>>>    
>>> -	intel_wakeref_init(&engine->wakeref, rpm, &wf_ops);
>>>    	intel_engine_init_heartbeat(engine);
>>>    
>>>    	intel_gsc_idle_msg_enable(engine);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +void intel_engine_destroy__pm(struct intel_engine_cs *engine)
>>> +{
>>> +	intel_wakeref_destroy(&engine->wakeref);
>>>    }
>>>    
>>>    /**
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.h b/drivers/gpu/drm/i915/gt/intel_engine_pm.h
>>> index d68675925b79..e8568f7d10c6 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.h
>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.h
>>> @@ -104,7 +104,8 @@ intel_engine_create_kernel_request(struct intel_engine_cs *engine)
>>>    	return rq;
>>>    }
>>>    
>>> -void intel_engine_init__pm(struct intel_engine_cs *engine);
>>> +int intel_engine_init__pm(struct intel_engine_cs *engine);
>>> +void intel_engine_destroy__pm(struct intel_engine_cs *engine);
>>>    
>>>    void intel_engine_reset_pinned_contexts(struct intel_engine_cs *engine);
>>>    
>>> diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c b/drivers/gpu/drm/i915/gt/mock_engine.c
>>> index c0637bf799a3..0a3c702c21e2 100644
>>> --- a/drivers/gpu/drm/i915/gt/mock_engine.c
>>> +++ b/drivers/gpu/drm/i915/gt/mock_engine.c
>>> @@ -336,6 +336,7 @@ static void mock_engine_release(struct intel_engine_cs *engine)
>>>    	intel_context_put(engine->kernel_context);
>>>    
>>>    	intel_engine_fini_retire(engine);
>>> +	intel_engine_destroy__pm(engine);
>>>    }
>>>    
>>>    struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
>>> @@ -393,6 +394,7 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
>>>    int mock_engine_init(struct intel_engine_cs *engine)
>>>    {
>>>    	struct intel_context *ce;
>>> +	int err;
>>>    
>>>    	INIT_LIST_HEAD(&engine->pinned_contexts_list);
>>>    
>>> @@ -402,7 +404,11 @@ int mock_engine_init(struct intel_engine_cs *engine)
>>>    	engine->sched_engine->private_data = engine;
>>>    
>>>    	intel_engine_init_execlists(engine);
>>> -	intel_engine_init__pm(engine);
>>> +
>>> +	err = intel_engine_init__pm(engine);
>>> +	if (err)
>>> +		return err;
>>> +
>>>    	intel_engine_init_retire(engine);
>>>    
>>>    	engine->breadcrumbs = intel_breadcrumbs_create(NULL);
>>> diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c
>>> index dfd87d082218..6bae609e1312 100644
>>> --- a/drivers/gpu/drm/i915/intel_wakeref.c
>>> +++ b/drivers/gpu/drm/i915/intel_wakeref.c
>>> @@ -74,7 +74,7 @@ void __intel_wakeref_put_last(struct intel_wakeref *wf, unsigned long flags)
>>>    
>>>    	/* Assume we are not in process context and so cannot sleep. */
>>>    	if (flags & INTEL_WAKEREF_PUT_ASYNC || !mutex_trylock(&wf->mutex)) {
>>> -		mod_delayed_work(system_wq, &wf->work,
>>> +		mod_delayed_work(wf->wq, &wf->work,
>>>    				 FIELD_GET(INTEL_WAKEREF_PUT_DELAY, flags));
>>>    		return;
>>>    	}
>>> @@ -93,10 +93,10 @@ static void __intel_wakeref_put_work(struct work_struct *wrk)
>>>    	____intel_wakeref_put_last(wf);
>>>    }
>>>    
>>> -void __intel_wakeref_init(struct intel_wakeref *wf,
>>> -			  struct intel_runtime_pm *rpm,
>>> -			  const struct intel_wakeref_ops *ops,
>>> -			  struct intel_wakeref_lockclass *key)
>>> +int __intel_wakeref_init(struct intel_wakeref *wf,
>>> +			 struct intel_runtime_pm *rpm,
>>> +			 const struct intel_wakeref_ops *ops,
>>> +			 struct intel_wakeref_lockclass *key)
>>>    {
>>>    	wf->rpm = rpm;
>>>    	wf->ops = ops;
>>> @@ -105,9 +105,20 @@ void __intel_wakeref_init(struct intel_wakeref *wf,
>>>    	atomic_set(&wf->count, 0);
>>>    	wf->wakeref = 0;
>>>    
>>> +	wf->wq = alloc_workqueue("i1915-wakeref", 0, 0);
>>
>> Typo here -
> 
> Oh, good catch! This is one of my "favorite" typos, for some reason.

Yes, I had the same one. :) Patch 3/3 too.

>>   I wanted to ask however - why does this particular wq
>> "deserves" to be dedicated and can't just use one of the
>> drm_i915_private ones?
> 
> It's because there's no easy way to get access to the drm_i915_private
> structure from here.  And I don't think this work needs to be in sync
> with the rest of the works in i915.

Yeah I don't think it needs to be synchronised either. Was just thinking 
if we really need to be creating a bunch of separate workqueues (one per 
engine) for not much use, or instead could just add a backpointer to 
either intel_wakeref or intel_runtime_pm. Latter already has rpm->kdev 
so could plausably be replaced with rpm->i915.

Actually, looking at intel_runtime_pm_init_early, you could get to i915 
via wf->rpm and container_of.

Regards,

Tvrtko