| Message ID | 1510562894-1561-2-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 13 Nov 2017 09:48:11 +0100, Sagar Arun Kamble <sagar.a.kamble@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@chris-wilson.co.uk> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michal Winiarski <michal.winiarski@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Acked-by: Chris Wilson <chris@chris-wilson.co.uk> > Acked-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Acked-by: Oscar Mateo <oscar.mateo@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
Quoting Michal Wajdeczko (2017-11-14 12:23:18) > On Mon, 13 Nov 2017 09:48:11 +0100, Sagar Arun Kamble > > -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. Hooks are an interesting one, because they are exported via the vfuncs even though they are static. Here, the export is onto to other i915 functions so it is reasonably clear, but if we export a vfunc further afield having the intel_ prefix is useful to mark the boundary into our module. -Chris
On Tue, 14 Nov 2017 13:31:44 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2017-11-14 12:23:18) >> On Mon, 13 Nov 2017 09:48:11 +0100, Sagar Arun Kamble >> > -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. > > Hooks are an interesting one, because they are exported via the vfuncs > even though they are static. Here, the export is onto to other i915 > functions so it is reasonably clear, but if we export a vfunc further > afield having the intel_ prefix is useful to mark the boundary into our > module. Note that our boundary is already visible/available when using %pF format. See some examples below: [ 98.279612] i915_init+0x6b/0x6e [i915] ^^^^ [ 67.109688] [drm:intel_device_info_dump [i915]] ... ^^^^ -Michal
Quoting Michal Wajdeczko (2017-11-14 14:54:09) > On Tue, 14 Nov 2017 13:31:44 +0100, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > Quoting Michal Wajdeczko (2017-11-14 12:23:18) > >> On Mon, 13 Nov 2017 09:48:11 +0100, Sagar Arun Kamble > >> > -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. > > > > Hooks are an interesting one, because they are exported via the vfuncs > > even though they are static. Here, the export is onto to other i915 > > functions so it is reasonably clear, but if we export a vfunc further > > afield having the intel_ prefix is useful to mark the boundary into our > > module. > > Note that our boundary is already visible/available when using %pF format. > See some examples below: > > [ 98.279612] i915_init+0x6b/0x6e [i915] > ^^^^ > [ 67.109688] [drm:intel_device_info_dump [i915]] ... > ^^^^ True. I shall keep my comments to userspace then ;) -Chris
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@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@chris-wilson.co.uk> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Michal Winiarski <michal.winiarski@intel.com> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Acked-by: Chris Wilson <chris@chris-wilson.co.uk> >> Acked-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Acked-by: Oscar Mateo <oscar.mateo@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
Quoting Sagar Arun Kamble (2017-11-14 18:19:01) > > > 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@intel.com> wrote: > >> -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. Ok. Do that as a separate (initial) step. -Chris
On Tue, 14 Nov 2017 19:27:26 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Sagar Arun Kamble (2017-11-14 18:19:01) >> >> >> 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@intel.com> wrote: >> >> -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. > > Ok. Do that as a separate (initial) step. Hmm. By "tasklet" I usually think of "tasklet_struct". Then "guc_submission_tasklet" suggests that this is another kind or customized "tasklet" struct. So maybe use full name: s/i915_guc_irq_handler/guc_submission_tasklet_func ?
Quoting Michal Wajdeczko (2017-11-14 19:23:24) > On Tue, 14 Nov 2017 19:27:26 +0100, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > Quoting Sagar Arun Kamble (2017-11-14 18:19:01) > >> > >> > >> 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@intel.com> wrote: > >> >> -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. > > > > Ok. Do that as a separate (initial) step. > > Hmm. By "tasklet" I usually think of "tasklet_struct". Then > "guc_submission_tasklet" suggests that this is another kind > or customized "tasklet" struct. So maybe use full name: > > s/i915_guc_irq_handler/guc_submission_tasklet_func ? Please no. You'll grow to dislike the tautology immensely! struct tasklet tasklet; execlists->tasklet = execlists_submission_tasklet; execlists->tasklet = guc_submission_tasklet; tasklet_schedule(engine->execlists.tasklet) etc is clear to me. -Chris
On 11/15/2017 1:01 AM, Chris Wilson wrote: > Quoting Michal Wajdeczko (2017-11-14 19:23:24) >> On Tue, 14 Nov 2017 19:27:26 +0100, Chris Wilson >> <chris@chris-wilson.co.uk> wrote: >> >>> Quoting Sagar Arun Kamble (2017-11-14 18:19:01) >>>> >>>> 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@intel.com> wrote: >>>>>> -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. >>> Ok. Do that as a separate (initial) step. >> Hmm. By "tasklet" I usually think of "tasklet_struct". Then >> "guc_submission_tasklet" suggests that this is another kind >> or customized "tasklet" struct. So maybe use full name: >> >> s/i915_guc_irq_handler/guc_submission_tasklet_func ? > Please no. You'll grow to dislike the tautology immensely! > > struct tasklet tasklet; > > execlists->tasklet = execlists_submission_tasklet; You meant "execlists->tasklet.func =" here right? > execlists->tasklet = guc_submission_tasklet; > > tasklet_schedule(engine->execlists.tasklet) etc > > is clear to me. > -Chris Michal wanted to distinguish tasklet func from tasklet. I thought of naming vfunc as submission_tasklet and component functions as execlists/guc_submission_tasklet_func (with michal suggestion). But that is looking little big i guess. With Michal's initial suggestion how about below change? (irq_tasklet was little misleading for me) struct tasklet tasklet; execlists->tasklet.func = execlists_submission_handler; execlists->tasklet.func = guc_submission_handler; tasklet_schedule(engine->execlists.tasklet)
Quoting Sagar Arun Kamble (2017-11-14 19:47:05) > > > On 11/15/2017 1:01 AM, Chris Wilson wrote: > > Quoting Michal Wajdeczko (2017-11-14 19:23:24) > >> On Tue, 14 Nov 2017 19:27:26 +0100, Chris Wilson > >> <chris@chris-wilson.co.uk> wrote: > >> > >>> Quoting Sagar Arun Kamble (2017-11-14 18:19:01) > >>>> > >>>> 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@intel.com> wrote: > >>>>>> -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. > >>> Ok. Do that as a separate (initial) step. > >> Hmm. By "tasklet" I usually think of "tasklet_struct". Then > >> "guc_submission_tasklet" suggests that this is another kind > >> or customized "tasklet" struct. So maybe use full name: > >> > >> s/i915_guc_irq_handler/guc_submission_tasklet_func ? > > Please no. You'll grow to dislike the tautology immensely! > > > > struct tasklet tasklet; > > > > execlists->tasklet = execlists_submission_tasklet; > You meant "execlists->tasklet.func =" here right? > > execlists->tasklet = guc_submission_tasklet; > > > > tasklet_schedule(engine->execlists.tasklet) etc > > > > is clear to me. > > -Chris > Michal wanted to distinguish tasklet func from tasklet. I don't see the point as I don't find any confusion between a struct and a function. The tasklet is the function; struct tasklet is merely its integration to softirq. -Chris
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. */ -static void i915_guc_submit(struct intel_engine_cs *engine) +static void intel_guc_submit(struct intel_engine_cs *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) { 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) { 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) { 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; } 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); } i915_ggtt_disable_guc(dev_priv);