[Intel-gfx] [PATCH 1/4] drm/i915/guc: Update name and prototype of GuC submission related functions
Sagar Arun Kamble
sagar.a.kamble at intel.com
Tue Nov 14 18:19:01 UTC 2017
On 11/14/2017 5:53 PM, Michal Wajdeczko wrote:
> On Mon, 13 Nov 2017 09:48:11 +0100, Sagar Arun Kamble
> <sagar.a.kamble at intel.com> wrote:
>
>> i915 GuC submission is hardware interface and GuC APIs that are not user
>> facing should be named intel_guc* hence we change GuC submission related
>> functions name prefix to intel_guc. Also changed the parameter to these
>> functions to intel_guc struct.
>>
>> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Michal Winiarski <michal.winiarski at intel.com>
>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Acked-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Acked-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Acked-by: Oscar Mateo <oscar.mateo at intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_guc_submission.c | 39
>> ++++++++++++++----------------
>> drivers/gpu/drm/i915/i915_guc_submission.h | 8 +++---
>> drivers/gpu/drm/i915/intel_uc.c | 10 ++++----
>> 3 files changed, 27 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 0ba2fc0..42dc5b4 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -683,13 +683,13 @@ static void wait_for_guc_preempt_report(struct
>> intel_engine_cs *engine)
>> }
>> /**
>> - * i915_guc_submit() - Submit commands through GuC
>> + * intel_guc_submit() - Submit commands through GuC
>> * @engine: engine associated with the commands
>> *
>> * The only error here arises if the doorbell hardware isn't
>> functioning
>> * as expected, which really shouln't happen.
> ^
> While here, please fix this old typo.
Yes.
>
>> */
>> -static void i915_guc_submit(struct intel_engine_cs *engine)
>> +static void intel_guc_submit(struct intel_engine_cs *engine)
>
> Hmm, this is static function and neither old or new name looks good
> or correct as function takes engine as parameter not guc.
>
> What about plain and simple "submit_through_guc(engine)" ?
> Or more formal "engine_submit_through_guc(engine)" ?
>
How about "guc_submit(engine)" & "guc_dequeue(engine)" vis-a-vis
"execlists_submit_ports" & "execlists_dequeue"
I think ports are not visible with GuC hence we may not say
guc_submit_ports. agree?
>> {
>> struct intel_guc *guc = &engine->i915->guc;
>> struct intel_engine_execlists * const execlists =
>> &engine->execlists;
>> @@ -722,7 +722,7 @@ static void port_assign(struct execlist_port *port,
>> port_set(port, port_pack(i915_gem_request_get(rq),
>> port_count(port)));
>> }
>> -static void i915_guc_dequeue(struct intel_engine_cs *engine)
>> +static void intel_guc_dequeue(struct intel_engine_cs *engine)
>
> and then matching "dequeue_and_submit(engine)"
>
>> {
>> struct intel_engine_execlists * const execlists =
>> &engine->execlists;
>> struct execlist_port *port = execlists->port;
>> @@ -793,13 +793,13 @@ static void i915_guc_dequeue(struct
>> intel_engine_cs *engine)
>> if (submit) {
>> port_assign(port, last);
>> execlists_set_active(execlists, EXECLISTS_ACTIVE_USER);
>> - i915_guc_submit(engine);
>> + intel_guc_submit(engine);
>> }
>> unlock:
>> spin_unlock_irq(&engine->timeline->lock);
>> }
>> -static void i915_guc_irq_handler(unsigned long data)
>> +static void intel_guc_irq_handler(unsigned long data)
>
> and verbose "guc_submission_handler()" ?
>
Yes. Should we rename irq_tasklet to submission_tasklet?
then we can s/intel_lrc_irq_handler/execlists_submission_tasklet and
s/i915_guc_irq_handler/guc_submission_tasklet.
Again trying to maintain the nomenclature consistency for Execlists and GuC.
>> {
>> struct intel_engine_cs * const engine = (struct intel_engine_cs
>> *)data;
>> struct intel_engine_execlists * const execlists =
>> &engine->execlists;
>> @@ -831,13 +831,13 @@ static void i915_guc_irq_handler(unsigned long
>> data)
>> }
>> if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT))
>> - i915_guc_dequeue(engine);
>> + intel_guc_dequeue(engine);
>> }
>> /*
>> * Everything below here is concerned with setup & teardown, and is
>> * therefore not part of the somewhat time-critical batch-submission
>> - * path of i915_guc_submit() above.
>> + * path of intel_guc_submit() above.
>> */
>> /* Check that a doorbell register is in the expected state */
>> @@ -1253,9 +1253,8 @@ static void guc_preempt_work_destroy(struct
>> intel_guc *guc)
>> * Set up the memory resources to be shared with the GuC (via the GGTT)
>> * at firmware loading time.
>> */
>> -int i915_guc_submission_init(struct drm_i915_private *dev_priv)
>> +int intel_guc_submission_init(struct intel_guc *guc)
>> {
>> - struct intel_guc *guc = &dev_priv->guc;
>> int ret;
>> if (guc->stage_desc_pool)
>> @@ -1302,10 +1301,8 @@ int i915_guc_submission_init(struct
>> drm_i915_private *dev_priv)
>> return ret;
>> }
>> -void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>> +void intel_guc_submission_fini(struct intel_guc *guc)
>> {
>> - struct intel_guc *guc = &dev_priv->guc;
>> -
>> guc_ads_destroy(guc);
>> guc_preempt_work_destroy(guc);
>> intel_guc_log_destroy(guc);
>> @@ -1381,19 +1378,19 @@ static void guc_interrupts_release(struct
>> drm_i915_private *dev_priv)
>> rps->pm_intrmsk_mbz &= ~ARAT_EXPIRED_INTRMSK;
>> }
>> -static void i915_guc_submission_park(struct intel_engine_cs *engine)
>> +static void intel_guc_submission_park(struct intel_engine_cs *engine)
>> {
>> intel_engine_unpin_breadcrumbs_irq(engine);
>> }
>> -static void i915_guc_submission_unpark(struct intel_engine_cs *engine)
>> +static void intel_guc_submission_unpark(struct intel_engine_cs *engine)
>
> Both park/unpark are also static and do not require "intel" prefix.
>
Yes.
>> {
>> intel_engine_pin_breadcrumbs_irq(engine);
>> }
>> -int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
>> +int intel_guc_submission_enable(struct intel_guc *guc)
>> {
>> - struct intel_guc *guc = &dev_priv->guc;
>> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> struct intel_engine_cs *engine;
>> enum intel_engine_id id;
>> int err;
>> @@ -1439,9 +1436,9 @@ int i915_guc_submission_enable(struct
>> drm_i915_private *dev_priv)
>> for_each_engine(engine, dev_priv, id) {
>> struct intel_engine_execlists * const execlists =
>> &engine->execlists;
>> - execlists->irq_tasklet.func = i915_guc_irq_handler;
>> - engine->park = i915_guc_submission_park;
>> - engine->unpark = i915_guc_submission_unpark;
>> + execlists->irq_tasklet.func = intel_guc_irq_handler;
>> + engine->park = intel_guc_submission_park;
>> + engine->unpark = intel_guc_submission_unpark;
>
> Then we'll have:
>
> execlists->irq_tasklet.func = guc_submission_handler;
> engine->park = guc_submission_park;
> engine->unpark = guc_submission_unpark;
>
>> }
>> return 0;
>> @@ -1451,9 +1448,9 @@ int i915_guc_submission_enable(struct
>> drm_i915_private *dev_priv)
>> return err;
>> }
>> -void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
>> +void intel_guc_submission_disable(struct intel_guc *guc)
>> {
>> - struct intel_guc *guc = &dev_priv->guc;
>> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> GEM_BUG_ON(dev_priv->gt.awake); /* GT should be parked first */
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.h
>> b/drivers/gpu/drm/i915/i915_guc_submission.h
>> index cb4353b..6bdb8fc 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.h
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.h
>> @@ -72,9 +72,9 @@ struct i915_guc_client {
>> u64 submissions[I915_NUM_ENGINES];
>> };
>> -int i915_guc_submission_init(struct drm_i915_private *dev_priv);
>> -int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
>> -void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
>> -void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
>> +int intel_guc_submission_init(struct intel_guc *guc);
>> +int intel_guc_submission_enable(struct intel_guc *guc);
>> +void intel_guc_submission_disable(struct intel_guc *guc);
>> +void intel_guc_submission_fini(struct intel_guc *guc);
>> #endif
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index aec2954..775db48 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -168,7 +168,7 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> * This is stuff we need to have available at fw load time
>> * if we are planning to enable submission later
>> */
>> - ret = i915_guc_submission_init(dev_priv);
>> + ret = intel_guc_submission_init(guc);
>> if (ret)
>> goto err_guc;
>> }
>> @@ -217,7 +217,7 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> if (i915_modparams.guc_log_level >= 0)
>> gen9_enable_guc_interrupts(dev_priv);
>> - ret = i915_guc_submission_enable(dev_priv);
>> + ret = intel_guc_submission_enable(guc);
>> if (ret)
>> goto err_interrupts;
>> }
>> @@ -246,7 +246,7 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> guc_capture_load_err_log(guc);
>> err_submission:
>> if (i915_modparams.enable_guc_submission)
>> - i915_guc_submission_fini(dev_priv);
>> + intel_guc_submission_fini(guc);
>> err_guc:
>> i915_ggtt_disable_guc(dev_priv);
>> @@ -277,13 +277,13 @@ void intel_uc_fini_hw(struct drm_i915_private
>> *dev_priv)
>> return;
>> if (i915_modparams.enable_guc_submission)
>> - i915_guc_submission_disable(dev_priv);
>> + intel_guc_submission_disable(&dev_priv->guc);
>> guc_disable_communication(&dev_priv->guc);
>> if (i915_modparams.enable_guc_submission) {
>> gen9_disable_guc_interrupts(dev_priv);
>> - i915_guc_submission_fini(dev_priv);
>> + intel_guc_submission_fini(&dev_priv->guc);
>
> Better to declare local variable guc and use them in all places.
Yes.
>
>> }
>> i915_ggtt_disable_guc(dev_priv);
>
> Michal
More information about the Intel-gfx
mailing list