[Intel-gfx] [PATCH 1/4] drm/i915/guc: Update name and prototype of GuC submission related functions
Michal Wajdeczko
michal.wajdeczko at intel.com
Tue Nov 14 12:23:18 UTC 2017
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.
> */
> -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)" ?
> {
> 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()" ?
> {
> 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.
> {
> 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.
> }
> i915_ggtt_disable_guc(dev_priv);
Michal
More information about the Intel-gfx
mailing list