* [PATCH 0/3] (Final) tidying up of GuC code
@ 2016-09-12 20:19 Dave Gordon
2016-09-12 20:19 ` [PATCH 1/3] drm/i915: clarify PMINTRMSK/pm_intr_keep usage Dave Gordon
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Dave Gordon @ 2016-09-12 20:19 UTC (permalink / raw)
To: intel-gfx
Mostly renaming the GuC functions to use a more consistent naming
scheme, along with updating comments to clarify some of the code.
Dave Gordon (3):
drm/i915: clarify PMINTRMSK/pm_intr_keep usage
drm/i915/guc: general tidying up (loader)
drm/i915/guc: general tidying up (submission)
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_guc_reg.h | 3 --
drivers/gpu/drm/i915/i915_guc_submission.c | 63 +++++++++++++++---------------
drivers/gpu/drm/i915/i915_irq.c | 4 +-
drivers/gpu/drm/i915/i915_reg.h | 2 +-
drivers/gpu/drm/i915/intel_guc.h | 2 +-
drivers/gpu/drm/i915/intel_guc_loader.c | 54 +++++++++++++++++--------
drivers/gpu/drm/i915/intel_lrc.c | 2 +-
8 files changed, 75 insertions(+), 56 deletions(-)
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] drm/i915: clarify PMINTRMSK/pm_intr_keep usage 2016-09-12 20:19 [PATCH 0/3] (Final) tidying up of GuC code Dave Gordon @ 2016-09-12 20:19 ` Dave Gordon 2016-09-14 15:06 ` Tvrtko Ursulin 2016-09-12 20:19 ` [PATCH 2/3] drm/i915/guc: general tidying up (loader) Dave Gordon ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Dave Gordon @ 2016-09-12 20:19 UTC (permalink / raw) To: intel-gfx No functional changes; just renaming a bit, tweaking a datatype, prettifying layout, and adding comments, in particular in the GuC setup code that touches this data. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_irq.c | 4 ++-- drivers/gpu/drm/i915/i915_reg.h | 2 +- drivers/gpu/drm/i915/intel_guc_loader.c | 27 +++++++++++++++++++++------ 4 files changed, 25 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1e2dda8..d01a50e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1184,6 +1184,7 @@ struct intel_gen6_power_mgmt { bool interrupts_enabled; u32 pm_iir; + /* PM interrupt bits that should never be masked */ u32 pm_intr_keep; /* Frequencies are stored in potentially platform dependent multiples. diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 8462817..c128fdb 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -371,7 +371,7 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv) spin_lock_irq(&dev_priv->irq_lock); dev_priv->rps.interrupts_enabled = false; - I915_WRITE(GEN6_PMINTRMSK, gen6_sanitize_rps_pm_mask(dev_priv, ~0)); + I915_WRITE(GEN6_PMINTRMSK, gen6_sanitize_rps_pm_mask(dev_priv, ~0u)); __gen6_disable_pm_irq(dev_priv, dev_priv->pm_rps_events); I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) & @@ -4500,7 +4500,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv) dev_priv->rps.pm_intr_keep |= GEN6_PM_RP_UP_EI_EXPIRED; if (INTEL_INFO(dev_priv)->gen >= 8) - dev_priv->rps.pm_intr_keep |= GEN8_PMINTR_REDIRECT_TO_NON_DISP; + dev_priv->rps.pm_intr_keep |= GEN8_PMINTR_REDIRECT_TO_GUC; INIT_DELAYED_WORK(&dev_priv->gpu_error.hangcheck_work, i915_hangcheck_elapsed); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index a29d707..70d9616 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7067,7 +7067,7 @@ enum { #define VLV_RCEDATA _MMIO(0xA0BC) #define GEN6_RC6pp_THRESHOLD _MMIO(0xA0C0) #define GEN6_PMINTRMSK _MMIO(0xA168) -#define GEN8_PMINTR_REDIRECT_TO_NON_DISP (1<<31) +#define GEN8_PMINTR_REDIRECT_TO_GUC (1<<31) #define GEN8_MISC_CTRL0 _MMIO(0xA180) #define VLV_PWRDWNUPCTL _MMIO(0xA294) #define GEN9_MEDIA_PG_IDLE_HYSTERESIS _MMIO(0xA0C4) diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 853928f..0021748 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -134,13 +134,28 @@ static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv) I915_WRITE(GUC_WD_VECS_IER, ~irqs); /* - * If GuC has routed PM interrupts to itself, don't keep it. - * and keep other interrupts those are unmasked by GuC. - */ + * The REDIRECT_TO_GUC bit of the PMINTRMSK register directs all + * (unmasked) PM interrupts to the GuC. All other bits of this + * register *disable* generation of a specific interrupt. + * + * 'pm_intr_keep' indicates bits that are NOT to be set when + * writing to the PM interrupt mask register, i.e. interrupts + * that must not be disabled. + * + * If the GuC is handling these interrupts, then we must not let + * the PM code disable ANY interrupt that the GuC is expecting. + * So for each ENABLED (0) bit in this register, we must SET the + * bit in pm_intr_keep so that it's left enabled for the GuC. + * + * OTOH the REDIRECT_TO_GUC bit is initially SET in pm_intr_keep + * (so interrupts go to the DISPLAY unit at first); but here we + * need to CLEAR that bit, which will result in the register bit + * being left SET! + */ tmp = I915_READ(GEN6_PMINTRMSK); - if (tmp & GEN8_PMINTR_REDIRECT_TO_NON_DISP) { - dev_priv->rps.pm_intr_keep |= ~(tmp & ~GEN8_PMINTR_REDIRECT_TO_NON_DISP); - dev_priv->rps.pm_intr_keep &= ~GEN8_PMINTR_REDIRECT_TO_NON_DISP; + if (tmp & GEN8_PMINTR_REDIRECT_TO_GUC) { + dev_priv->rps.pm_intr_keep |= ~tmp; + dev_priv->rps.pm_intr_keep &= ~GEN8_PMINTR_REDIRECT_TO_GUC; } } -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] drm/i915: clarify PMINTRMSK/pm_intr_keep usage 2016-09-12 20:19 ` [PATCH 1/3] drm/i915: clarify PMINTRMSK/pm_intr_keep usage Dave Gordon @ 2016-09-14 15:06 ` Tvrtko Ursulin 0 siblings, 0 replies; 11+ messages in thread From: Tvrtko Ursulin @ 2016-09-14 15:06 UTC (permalink / raw) To: Dave Gordon, intel-gfx On 12/09/2016 21:19, Dave Gordon wrote: > No functional changes; just renaming a bit, tweaking a datatype, > prettifying layout, and adding comments, in particular in the > GuC setup code that touches this data. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_irq.c | 4 ++-- > drivers/gpu/drm/i915/i915_reg.h | 2 +- > drivers/gpu/drm/i915/intel_guc_loader.c | 27 +++++++++++++++++++++------ > 4 files changed, 25 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 1e2dda8..d01a50e 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1184,6 +1184,7 @@ struct intel_gen6_power_mgmt { > bool interrupts_enabled; > u32 pm_iir; > > + /* PM interrupt bits that should never be masked */ > u32 pm_intr_keep; > > /* Frequencies are stored in potentially platform dependent multiples. > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 8462817..c128fdb 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -371,7 +371,7 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv) > spin_lock_irq(&dev_priv->irq_lock); > dev_priv->rps.interrupts_enabled = false; > > - I915_WRITE(GEN6_PMINTRMSK, gen6_sanitize_rps_pm_mask(dev_priv, ~0)); > + I915_WRITE(GEN6_PMINTRMSK, gen6_sanitize_rps_pm_mask(dev_priv, ~0u)); > > __gen6_disable_pm_irq(dev_priv, dev_priv->pm_rps_events); > I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) & > @@ -4500,7 +4500,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv) > dev_priv->rps.pm_intr_keep |= GEN6_PM_RP_UP_EI_EXPIRED; > > if (INTEL_INFO(dev_priv)->gen >= 8) > - dev_priv->rps.pm_intr_keep |= GEN8_PMINTR_REDIRECT_TO_NON_DISP; > + dev_priv->rps.pm_intr_keep |= GEN8_PMINTR_REDIRECT_TO_GUC; > > INIT_DELAYED_WORK(&dev_priv->gpu_error.hangcheck_work, > i915_hangcheck_elapsed); > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index a29d707..70d9616 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7067,7 +7067,7 @@ enum { > #define VLV_RCEDATA _MMIO(0xA0BC) > #define GEN6_RC6pp_THRESHOLD _MMIO(0xA0C0) > #define GEN6_PMINTRMSK _MMIO(0xA168) > -#define GEN8_PMINTR_REDIRECT_TO_NON_DISP (1<<31) > +#define GEN8_PMINTR_REDIRECT_TO_GUC (1<<31) > #define GEN8_MISC_CTRL0 _MMIO(0xA180) > #define VLV_PWRDWNUPCTL _MMIO(0xA294) > #define GEN9_MEDIA_PG_IDLE_HYSTERESIS _MMIO(0xA0C4) > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 853928f..0021748 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -134,13 +134,28 @@ static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv) > I915_WRITE(GUC_WD_VECS_IER, ~irqs); > > /* > - * If GuC has routed PM interrupts to itself, don't keep it. > - * and keep other interrupts those are unmasked by GuC. > - */ > + * The REDIRECT_TO_GUC bit of the PMINTRMSK register directs all > + * (unmasked) PM interrupts to the GuC. All other bits of this > + * register *disable* generation of a specific interrupt. > + * > + * 'pm_intr_keep' indicates bits that are NOT to be set when > + * writing to the PM interrupt mask register, i.e. interrupts > + * that must not be disabled. > + * > + * If the GuC is handling these interrupts, then we must not let > + * the PM code disable ANY interrupt that the GuC is expecting. > + * So for each ENABLED (0) bit in this register, we must SET the > + * bit in pm_intr_keep so that it's left enabled for the GuC. > + * > + * OTOH the REDIRECT_TO_GUC bit is initially SET in pm_intr_keep > + * (so interrupts go to the DISPLAY unit at first); but here we > + * need to CLEAR that bit, which will result in the register bit > + * being left SET! > + */ > tmp = I915_READ(GEN6_PMINTRMSK); > - if (tmp & GEN8_PMINTR_REDIRECT_TO_NON_DISP) { > - dev_priv->rps.pm_intr_keep |= ~(tmp & ~GEN8_PMINTR_REDIRECT_TO_NON_DISP); > - dev_priv->rps.pm_intr_keep &= ~GEN8_PMINTR_REDIRECT_TO_NON_DISP; > + if (tmp & GEN8_PMINTR_REDIRECT_TO_GUC) { > + dev_priv->rps.pm_intr_keep |= ~tmp; > + dev_priv->rps.pm_intr_keep &= ~GEN8_PMINTR_REDIRECT_TO_GUC; > } > } > More comments is always good and the cleanup just above obviously good as well. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] drm/i915/guc: general tidying up (loader) 2016-09-12 20:19 [PATCH 0/3] (Final) tidying up of GuC code Dave Gordon 2016-09-12 20:19 ` [PATCH 1/3] drm/i915: clarify PMINTRMSK/pm_intr_keep usage Dave Gordon @ 2016-09-12 20:19 ` Dave Gordon 2016-09-14 15:12 ` Tvrtko Ursulin 2016-09-12 20:19 ` [PATCH 3/3] drm/i915/guc: general tidying up (submission) Dave Gordon 2016-09-12 21:20 ` ✓ Fi.CI.BAT: success for (Final) tidying up of GuC code Patchwork 3 siblings, 1 reply; 11+ messages in thread From: Dave Gordon @ 2016-09-12 20:19 UTC (permalink / raw) To: intel-gfx Renaming to more consistent scheme, delete unused definitions Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_guc_reg.h | 3 --- drivers/gpu/drm/i915/intel_guc_loader.c | 27 ++++++++++++++++----------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h index cf5a65b..a47e1e4 100644 --- a/drivers/gpu/drm/i915/i915_guc_reg.h +++ b/drivers/gpu/drm/i915/i915_guc_reg.h @@ -103,9 +103,6 @@ #define HOST2GUC_INTERRUPT _MMIO(0xc4c8) #define HOST2GUC_TRIGGER (1<<0) -#define DRBMISC1 0x1984 -#define DOORBELL_ENABLE (1<<0) - #define GEN8_DRBREGL(x) _MMIO(0x1000 + (x) * 8) #define GEN8_DRB_VALID (1<<0) #define GEN8_DRBREGU(x) _MMIO(0x1000 + (x) * 8 + 4) diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 0021748..6fd39ef 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status) } }; -static void direct_interrupts_to_host(struct drm_i915_private *dev_priv) +static void guc_interrupts_release(struct drm_i915_private *dev_priv) { struct intel_engine_cs *engine; int irqs; @@ -114,7 +114,7 @@ static void direct_interrupts_to_host(struct drm_i915_private *dev_priv) I915_WRITE(GUC_WD_VECS_IER, 0); } -static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv) +static void guc_interrupts_capture(struct drm_i915_private *dev_priv) { struct intel_engine_cs *engine; int irqs; @@ -179,7 +179,12 @@ static u32 get_core_family(struct drm_i915_private *dev_priv) } } -static void set_guc_init_params(struct drm_i915_private *dev_priv) +/* + * Initialise the GuC parameter block before starting the firmware + * transfer. These parameters are read by the firmware on startup + * and cannot be changed thereafter. + */ +static void guc_params_init(struct drm_i915_private *dev_priv) { struct intel_guc *guc = &dev_priv->guc; u32 params[GUC_CTL_MAX_DWORDS]; @@ -392,11 +397,11 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv) I915_WRITE(GEN7_MISCCPCTL, (GEN8_DOP_CLOCK_GATE_GUC_ENABLE | I915_READ(GEN7_MISCCPCTL))); - /* allows for 5us before GT can go to RC6 */ + /* allows for 5us (in 10ns units) before GT can go to RC6 */ I915_WRITE(GUC_ARAT_C6DIS, 0x1FF); } - set_guc_init_params(dev_priv); + guc_params_init(dev_priv); ret = guc_ucode_xfer_dma(dev_priv, vma); @@ -411,7 +416,7 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv) return ret; } -static int i915_reset_guc(struct drm_i915_private *dev_priv) +static int guc_hw_reset(struct drm_i915_private *dev_priv) { int ret; u32 guc_status; @@ -478,7 +483,7 @@ int intel_guc_setup(struct drm_device *dev) goto fail; } - direct_interrupts_to_host(dev_priv); + guc_interrupts_release(dev_priv); guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; @@ -501,7 +506,7 @@ int intel_guc_setup(struct drm_device *dev) * Always reset the GuC just before (re)loading, so * that the state and timing are fairly predictable */ - err = i915_reset_guc(dev_priv); + err = guc_hw_reset(dev_priv); if (err) goto fail; @@ -526,7 +531,7 @@ int intel_guc_setup(struct drm_device *dev) err = i915_guc_submission_enable(dev_priv); if (err) goto fail; - direct_interrupts_to_guc(dev_priv); + guc_interrupts_capture(dev_priv); } return 0; @@ -535,7 +540,7 @@ int intel_guc_setup(struct drm_device *dev) if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; - direct_interrupts_to_host(dev_priv); + guc_interrupts_release(dev_priv); i915_guc_submission_disable(dev_priv); i915_guc_submission_fini(dev_priv); @@ -768,7 +773,7 @@ void intel_guc_fini(struct drm_device *dev) struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; mutex_lock(&dev->struct_mutex); - direct_interrupts_to_host(dev_priv); + guc_interrupts_release(dev_priv); i915_guc_submission_disable(dev_priv); i915_guc_submission_fini(dev_priv); -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] drm/i915/guc: general tidying up (loader) 2016-09-12 20:19 ` [PATCH 2/3] drm/i915/guc: general tidying up (loader) Dave Gordon @ 2016-09-14 15:12 ` Tvrtko Ursulin 0 siblings, 0 replies; 11+ messages in thread From: Tvrtko Ursulin @ 2016-09-14 15:12 UTC (permalink / raw) To: Dave Gordon, intel-gfx On 12/09/2016 21:19, Dave Gordon wrote: > Renaming to more consistent scheme, delete unused definitions > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_reg.h | 3 --- > drivers/gpu/drm/i915/intel_guc_loader.c | 27 ++++++++++++++++----------- > 2 files changed, 16 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h > index cf5a65b..a47e1e4 100644 > --- a/drivers/gpu/drm/i915/i915_guc_reg.h > +++ b/drivers/gpu/drm/i915/i915_guc_reg.h > @@ -103,9 +103,6 @@ > #define HOST2GUC_INTERRUPT _MMIO(0xc4c8) > #define HOST2GUC_TRIGGER (1<<0) > > -#define DRBMISC1 0x1984 > -#define DOORBELL_ENABLE (1<<0) > - > #define GEN8_DRBREGL(x) _MMIO(0x1000 + (x) * 8) > #define GEN8_DRB_VALID (1<<0) > #define GEN8_DRBREGU(x) _MMIO(0x1000 + (x) * 8 + 4) > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 0021748..6fd39ef 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status) > } > }; > > -static void direct_interrupts_to_host(struct drm_i915_private *dev_priv) > +static void guc_interrupts_release(struct drm_i915_private *dev_priv) > { > struct intel_engine_cs *engine; > int irqs; > @@ -114,7 +114,7 @@ static void direct_interrupts_to_host(struct drm_i915_private *dev_priv) > I915_WRITE(GUC_WD_VECS_IER, 0); > } > > -static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv) > +static void guc_interrupts_capture(struct drm_i915_private *dev_priv) > { > struct intel_engine_cs *engine; > int irqs; > @@ -179,7 +179,12 @@ static u32 get_core_family(struct drm_i915_private *dev_priv) > } > } > > -static void set_guc_init_params(struct drm_i915_private *dev_priv) > +/* > + * Initialise the GuC parameter block before starting the firmware > + * transfer. These parameters are read by the firmware on startup > + * and cannot be changed thereafter. > + */ > +static void guc_params_init(struct drm_i915_private *dev_priv) > { > struct intel_guc *guc = &dev_priv->guc; > u32 params[GUC_CTL_MAX_DWORDS]; > @@ -392,11 +397,11 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv) > I915_WRITE(GEN7_MISCCPCTL, (GEN8_DOP_CLOCK_GATE_GUC_ENABLE | > I915_READ(GEN7_MISCCPCTL))); > > - /* allows for 5us before GT can go to RC6 */ > + /* allows for 5us (in 10ns units) before GT can go to RC6 */ > I915_WRITE(GUC_ARAT_C6DIS, 0x1FF); > } > > - set_guc_init_params(dev_priv); > + guc_params_init(dev_priv); > > ret = guc_ucode_xfer_dma(dev_priv, vma); > > @@ -411,7 +416,7 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv) > return ret; > } > > -static int i915_reset_guc(struct drm_i915_private *dev_priv) > +static int guc_hw_reset(struct drm_i915_private *dev_priv) > { > int ret; > u32 guc_status; > @@ -478,7 +483,7 @@ int intel_guc_setup(struct drm_device *dev) > goto fail; > } > > - direct_interrupts_to_host(dev_priv); > + guc_interrupts_release(dev_priv); > > guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; > > @@ -501,7 +506,7 @@ int intel_guc_setup(struct drm_device *dev) > * Always reset the GuC just before (re)loading, so > * that the state and timing are fairly predictable > */ > - err = i915_reset_guc(dev_priv); > + err = guc_hw_reset(dev_priv); > if (err) > goto fail; > > @@ -526,7 +531,7 @@ int intel_guc_setup(struct drm_device *dev) > err = i915_guc_submission_enable(dev_priv); > if (err) > goto fail; > - direct_interrupts_to_guc(dev_priv); > + guc_interrupts_capture(dev_priv); > } > > return 0; > @@ -535,7 +540,7 @@ int intel_guc_setup(struct drm_device *dev) > if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) > guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; > > - direct_interrupts_to_host(dev_priv); > + guc_interrupts_release(dev_priv); > i915_guc_submission_disable(dev_priv); > i915_guc_submission_fini(dev_priv); > > @@ -768,7 +773,7 @@ void intel_guc_fini(struct drm_device *dev) > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > > mutex_lock(&dev->struct_mutex); > - direct_interrupts_to_host(dev_priv); > + guc_interrupts_release(dev_priv); > i915_guc_submission_disable(dev_priv); > i915_guc_submission_fini(dev_priv); > I liked the direct interrupts naming more, but it is not that critical. The rest is OK. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] drm/i915/guc: general tidying up (submission) 2016-09-12 20:19 [PATCH 0/3] (Final) tidying up of GuC code Dave Gordon 2016-09-12 20:19 ` [PATCH 1/3] drm/i915: clarify PMINTRMSK/pm_intr_keep usage Dave Gordon 2016-09-12 20:19 ` [PATCH 2/3] drm/i915/guc: general tidying up (loader) Dave Gordon @ 2016-09-12 20:19 ` Dave Gordon 2016-09-14 15:22 ` Tvrtko Ursulin 2016-09-12 21:20 ` ✓ Fi.CI.BAT: success for (Final) tidying up of GuC code Patchwork 3 siblings, 1 reply; 11+ messages in thread From: Dave Gordon @ 2016-09-12 20:19 UTC (permalink / raw) To: intel-gfx Renaming to more consistent scheme, and updating comments, mostly about i915_guc_wq_reserve(), aka i915_guc_wq_check_space(). Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 63 +++++++++++++++--------------- drivers/gpu/drm/i915/intel_guc.h | 2 +- drivers/gpu/drm/i915/intel_lrc.c | 2 +- 3 files changed, 34 insertions(+), 33 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 279a4d0..43358e1 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -59,7 +59,7 @@ * WQ_TYPE_INORDER is needed to support legacy submission via GuC, which * represents in-order queue. The kernel driver packs ring tail pointer and an * ELSP context descriptor dword into Work Item. - * See guc_add_workqueue_item() + * See guc_wq_item_append() * */ @@ -288,7 +288,7 @@ static uint32_t select_doorbell_cacheline(struct intel_guc *guc) /* * Initialise the process descriptor shared with the GuC firmware. */ -static void guc_init_proc_desc(struct intel_guc *guc, +static void guc_proc_desc_init(struct intel_guc *guc, struct i915_guc_client *client) { struct guc_process_desc *desc; @@ -320,7 +320,7 @@ static void guc_init_proc_desc(struct intel_guc *guc, * write queue, etc). */ -static void guc_init_ctx_desc(struct intel_guc *guc, +static void guc_ctx_desc_init(struct intel_guc *guc, struct i915_guc_client *client) { struct drm_i915_private *dev_priv = guc_to_i915(guc); @@ -399,7 +399,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc, sizeof(desc) * client->ctx_index); } -static void guc_fini_ctx_desc(struct intel_guc *guc, +static void guc_ctx_desc_fini(struct intel_guc *guc, struct i915_guc_client *client) { struct guc_context_desc desc; @@ -413,7 +413,7 @@ static void guc_fini_ctx_desc(struct intel_guc *guc, } /** - * i915_guc_wq_check_space() - check that the GuC can accept a request + * i915_guc_wq_reserve() - reserve space in the GuC's workqueue * @request: request associated with the commands * * Return: 0 if space is available @@ -421,14 +421,14 @@ static void guc_fini_ctx_desc(struct intel_guc *guc, * * This function must be called (and must return 0) before a request * is submitted to the GuC via i915_guc_submit() below. Once a result - * of 0 has been returned, it remains valid until (but only until) - * the next call to submit(). + * of 0 has been returned, it must be balanced by a corresponding + * call to submit(). * - * This precheck allows the caller to determine in advance that space + * Reservation allows the caller to determine in advance that space * will be available for the next submission before committing resources * to it, and helps avoid late failures with complicated recovery paths. */ -int i915_guc_wq_check_space(struct drm_i915_gem_request *request) +int i915_guc_wq_reserve(struct drm_i915_gem_request *request) { const size_t wqi_size = sizeof(struct guc_wq_item); struct i915_guc_client *gc = request->i915->guc.execbuf_client; @@ -451,8 +451,9 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request *request) return ret; } -static void guc_add_workqueue_item(struct i915_guc_client *gc, - struct drm_i915_gem_request *rq) +/* Construct a Work Item and append it to the GuC's Work Queue */ +static void guc_wq_item_append(struct i915_guc_client *gc, + struct drm_i915_gem_request *rq) { /* wqi_len is in DWords, and does not include the one-word header */ const size_t wqi_size = sizeof(struct guc_wq_item); @@ -465,7 +466,7 @@ static void guc_add_workqueue_item(struct i915_guc_client *gc, desc = gc->client_base + gc->proc_desc_offset; - /* Free space is guaranteed, see i915_guc_wq_check_space() above */ + /* Free space is guaranteed, see i915_guc_wq_reserve() above */ freespace = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size); GEM_BUG_ON(freespace < wqi_size); @@ -575,14 +576,13 @@ static int guc_ring_doorbell(struct i915_guc_client *gc) * Return: 0 on success, otherwise an errno. * (Note: nonzero really shouldn't happen!) * - * The caller must have already called i915_guc_wq_check_space() above - * with a result of 0 (success) since the last request submission. This - * guarantees that there is space in the work queue for the new request, - * so enqueuing the item cannot fail. + * The caller must have already called i915_guc_wq_reserve() above with + * a result of 0 (success), guaranteeing that there is space in the work + * queue for the new request, so enqueuing the item cannot fail. * * Bad Things Will Happen if the caller violates this protocol e.g. calls - * submit() when check() says there's no space, or calls submit() multiple - * times with no intervening check(). + * submit() when _reserve() says there's no space, or calls _submit() + * a different number of times from (successful) calls to _reserve(). * * The only error here arises if the doorbell hardware isn't functioning * as expected, which really shouln't happen. @@ -595,7 +595,7 @@ static void i915_guc_submit(struct drm_i915_gem_request *rq) int b_ret; spin_lock(&client->wq_lock); - guc_add_workqueue_item(client, rq); + guc_wq_item_append(client, rq); b_ret = guc_ring_doorbell(client); client->submissions[engine_id] += 1; @@ -686,7 +686,7 @@ static struct i915_vma *guc_allocate_vma(struct intel_guc *guc, u32 size) i915_vma_unpin_and_release(&client->vma); if (client->ctx_index != GUC_INVALID_CTX_ID) { - guc_fini_ctx_desc(guc, client); + guc_ctx_desc_fini(guc, client); ida_simple_remove(&guc->ctx_ids, client->ctx_index); } @@ -818,8 +818,8 @@ static void guc_init_doorbell_hw(struct intel_guc *guc) else client->proc_desc_offset = (GUC_DB_SIZE / 2); - guc_init_proc_desc(guc, client); - guc_init_ctx_desc(guc, client); + guc_proc_desc_init(guc, client); + guc_ctx_desc_init(guc, client); if (guc_init_doorbell(guc, client, db_id)) goto err; @@ -835,7 +835,7 @@ static void guc_init_doorbell_hw(struct intel_guc *guc) return NULL; } -static void guc_create_log(struct intel_guc *guc) +static void guc_log_create(struct intel_guc *guc) { struct i915_vma *vma; unsigned long offset; @@ -875,7 +875,7 @@ static void guc_create_log(struct intel_guc *guc) guc->log_flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags; } -static void init_guc_policies(struct guc_policies *policies) +static void guc_policies_init(struct guc_policies *policies) { struct guc_policy *policy; u32 p, i; @@ -897,7 +897,7 @@ static void init_guc_policies(struct guc_policies *policies) policies->is_valid = 1; } -static void guc_create_ads(struct intel_guc *guc) +static void guc_addon_create(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); struct i915_vma *vma; @@ -940,7 +940,7 @@ static void guc_create_ads(struct intel_guc *guc) /* GuC scheduling policies */ policies = (void *)ads + sizeof(struct guc_ads); - init_guc_policies(policies); + guc_policies_init(policies); ads->scheduler_policies = i915_ggtt_offset(vma) + sizeof(struct guc_ads); @@ -971,9 +971,11 @@ static void guc_create_ads(struct intel_guc *guc) */ int i915_guc_submission_init(struct drm_i915_private *dev_priv) { + const size_t ctxsize = sizeof(struct guc_context_desc); + const size_t poolsize = GUC_MAX_GPU_CONTEXTS * ctxsize; + const size_t gemsize = round_up(poolsize, PAGE_SIZE); struct intel_guc *guc = &dev_priv->guc; struct i915_vma *vma; - u32 size; /* Wipe bitmap & delete client in case of reinitialisation */ bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS); @@ -985,15 +987,14 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv) if (guc->ctx_pool_vma) return 0; /* already allocated */ - size = PAGE_ALIGN(GUC_MAX_GPU_CONTEXTS*sizeof(struct guc_context_desc)); - vma = guc_allocate_vma(guc, size); + vma = guc_allocate_vma(guc, gemsize); if (IS_ERR(vma)) return PTR_ERR(vma); guc->ctx_pool_vma = vma; ida_init(&guc->ctx_ids); - guc_create_log(guc); - guc_create_ads(guc); + guc_log_create(guc); + guc_addon_create(guc); return 0; } diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index 4678459..b1ba869 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -159,7 +159,7 @@ extern int intel_guc_resume(struct drm_device *dev); /* i915_guc_submission.c */ int i915_guc_submission_init(struct drm_i915_private *dev_priv); int i915_guc_submission_enable(struct drm_i915_private *dev_priv); -int i915_guc_wq_check_space(struct drm_i915_gem_request *rq); +int i915_guc_wq_reserve(struct drm_i915_gem_request *rq); void i915_guc_submission_disable(struct drm_i915_private *dev_priv); void i915_guc_submission_fini(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 16d7cdd..25114336 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -627,7 +627,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request * going any further, as the i915_add_request() call * later on mustn't fail ... */ - ret = i915_guc_wq_check_space(request); + ret = i915_guc_wq_reserve(request); if (ret) return ret; } -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] drm/i915/guc: general tidying up (submission) 2016-09-12 20:19 ` [PATCH 3/3] drm/i915/guc: general tidying up (submission) Dave Gordon @ 2016-09-14 15:22 ` Tvrtko Ursulin 2016-09-14 17:00 ` Dave Gordon 0 siblings, 1 reply; 11+ messages in thread From: Tvrtko Ursulin @ 2016-09-14 15:22 UTC (permalink / raw) To: Dave Gordon, intel-gfx On 12/09/2016 21:19, Dave Gordon wrote: > Renaming to more consistent scheme, and updating comments, mostly > about i915_guc_wq_reserve(), aka i915_guc_wq_check_space(). > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 63 +++++++++++++++--------------- > drivers/gpu/drm/i915/intel_guc.h | 2 +- > drivers/gpu/drm/i915/intel_lrc.c | 2 +- > 3 files changed, 34 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 279a4d0..43358e1 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -59,7 +59,7 @@ > * WQ_TYPE_INORDER is needed to support legacy submission via GuC, which > * represents in-order queue. The kernel driver packs ring tail pointer and an > * ELSP context descriptor dword into Work Item. > - * See guc_add_workqueue_item() > + * See guc_wq_item_append() > * > */ > > @@ -288,7 +288,7 @@ static uint32_t select_doorbell_cacheline(struct intel_guc *guc) > /* > * Initialise the process descriptor shared with the GuC firmware. > */ > -static void guc_init_proc_desc(struct intel_guc *guc, > +static void guc_proc_desc_init(struct intel_guc *guc, > struct i915_guc_client *client) > { > struct guc_process_desc *desc; > @@ -320,7 +320,7 @@ static void guc_init_proc_desc(struct intel_guc *guc, > * write queue, etc). > */ > > -static void guc_init_ctx_desc(struct intel_guc *guc, > +static void guc_ctx_desc_init(struct intel_guc *guc, > struct i915_guc_client *client) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > @@ -399,7 +399,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc, > sizeof(desc) * client->ctx_index); > } > > -static void guc_fini_ctx_desc(struct intel_guc *guc, > +static void guc_ctx_desc_fini(struct intel_guc *guc, > struct i915_guc_client *client) > { > struct guc_context_desc desc; > @@ -413,7 +413,7 @@ static void guc_fini_ctx_desc(struct intel_guc *guc, > } > > /** > - * i915_guc_wq_check_space() - check that the GuC can accept a request > + * i915_guc_wq_reserve() - reserve space in the GuC's workqueue > * @request: request associated with the commands > * > * Return: 0 if space is available > @@ -421,14 +421,14 @@ static void guc_fini_ctx_desc(struct intel_guc *guc, > * > * This function must be called (and must return 0) before a request > * is submitted to the GuC via i915_guc_submit() below. Once a result > - * of 0 has been returned, it remains valid until (but only until) > - * the next call to submit(). > + * of 0 has been returned, it must be balanced by a corresponding > + * call to submit(). > * > - * This precheck allows the caller to determine in advance that space > + * Reservation allows the caller to determine in advance that space > * will be available for the next submission before committing resources > * to it, and helps avoid late failures with complicated recovery paths. > */ > -int i915_guc_wq_check_space(struct drm_i915_gem_request *request) > +int i915_guc_wq_reserve(struct drm_i915_gem_request *request) > { > const size_t wqi_size = sizeof(struct guc_wq_item); > struct i915_guc_client *gc = request->i915->guc.execbuf_client; > @@ -451,8 +451,9 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request *request) > return ret; > } > > -static void guc_add_workqueue_item(struct i915_guc_client *gc, > - struct drm_i915_gem_request *rq) > +/* Construct a Work Item and append it to the GuC's Work Queue */ > +static void guc_wq_item_append(struct i915_guc_client *gc, > + struct drm_i915_gem_request *rq) > { > /* wqi_len is in DWords, and does not include the one-word header */ > const size_t wqi_size = sizeof(struct guc_wq_item); > @@ -465,7 +466,7 @@ static void guc_add_workqueue_item(struct i915_guc_client *gc, > > desc = gc->client_base + gc->proc_desc_offset; > > - /* Free space is guaranteed, see i915_guc_wq_check_space() above */ > + /* Free space is guaranteed, see i915_guc_wq_reserve() above */ > freespace = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size); > GEM_BUG_ON(freespace < wqi_size); > > @@ -575,14 +576,13 @@ static int guc_ring_doorbell(struct i915_guc_client *gc) > * Return: 0 on success, otherwise an errno. > * (Note: nonzero really shouldn't happen!) > * > - * The caller must have already called i915_guc_wq_check_space() above > - * with a result of 0 (success) since the last request submission. This > - * guarantees that there is space in the work queue for the new request, > - * so enqueuing the item cannot fail. > + * The caller must have already called i915_guc_wq_reserve() above with > + * a result of 0 (success), guaranteeing that there is space in the work > + * queue for the new request, so enqueuing the item cannot fail. > * > * Bad Things Will Happen if the caller violates this protocol e.g. calls > - * submit() when check() says there's no space, or calls submit() multiple > - * times with no intervening check(). > + * submit() when _reserve() says there's no space, or calls _submit() > + * a different number of times from (successful) calls to _reserve(). > * > * The only error here arises if the doorbell hardware isn't functioning > * as expected, which really shouln't happen. > @@ -595,7 +595,7 @@ static void i915_guc_submit(struct drm_i915_gem_request *rq) > int b_ret; > > spin_lock(&client->wq_lock); > - guc_add_workqueue_item(client, rq); > + guc_wq_item_append(client, rq); > b_ret = guc_ring_doorbell(client); > > client->submissions[engine_id] += 1; > @@ -686,7 +686,7 @@ static struct i915_vma *guc_allocate_vma(struct intel_guc *guc, u32 size) > i915_vma_unpin_and_release(&client->vma); > > if (client->ctx_index != GUC_INVALID_CTX_ID) { > - guc_fini_ctx_desc(guc, client); > + guc_ctx_desc_fini(guc, client); > ida_simple_remove(&guc->ctx_ids, client->ctx_index); > } > > @@ -818,8 +818,8 @@ static void guc_init_doorbell_hw(struct intel_guc *guc) > else > client->proc_desc_offset = (GUC_DB_SIZE / 2); > > - guc_init_proc_desc(guc, client); > - guc_init_ctx_desc(guc, client); > + guc_proc_desc_init(guc, client); > + guc_ctx_desc_init(guc, client); > if (guc_init_doorbell(guc, client, db_id)) > goto err; > > @@ -835,7 +835,7 @@ static void guc_init_doorbell_hw(struct intel_guc *guc) > return NULL; > } > > -static void guc_create_log(struct intel_guc *guc) > +static void guc_log_create(struct intel_guc *guc) > { > struct i915_vma *vma; > unsigned long offset; > @@ -875,7 +875,7 @@ static void guc_create_log(struct intel_guc *guc) > guc->log_flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags; > } > > -static void init_guc_policies(struct guc_policies *policies) > +static void guc_policies_init(struct guc_policies *policies) > { > struct guc_policy *policy; > u32 p, i; > @@ -897,7 +897,7 @@ static void init_guc_policies(struct guc_policies *policies) > policies->is_valid = 1; > } > > -static void guc_create_ads(struct intel_guc *guc) > +static void guc_addon_create(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > struct i915_vma *vma; > @@ -940,7 +940,7 @@ static void guc_create_ads(struct intel_guc *guc) > > /* GuC scheduling policies */ > policies = (void *)ads + sizeof(struct guc_ads); > - init_guc_policies(policies); > + guc_policies_init(policies); > > ads->scheduler_policies = > i915_ggtt_offset(vma) + sizeof(struct guc_ads); > @@ -971,9 +971,11 @@ static void guc_create_ads(struct intel_guc *guc) > */ > int i915_guc_submission_init(struct drm_i915_private *dev_priv) > { > + const size_t ctxsize = sizeof(struct guc_context_desc); > + const size_t poolsize = GUC_MAX_GPU_CONTEXTS * ctxsize; > + const size_t gemsize = round_up(poolsize, PAGE_SIZE); > struct intel_guc *guc = &dev_priv->guc; > struct i915_vma *vma; > - u32 size; > > /* Wipe bitmap & delete client in case of reinitialisation */ > bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS); > @@ -985,15 +987,14 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv) > if (guc->ctx_pool_vma) > return 0; /* already allocated */ > > - size = PAGE_ALIGN(GUC_MAX_GPU_CONTEXTS*sizeof(struct guc_context_desc)); > - vma = guc_allocate_vma(guc, size); > + vma = guc_allocate_vma(guc, gemsize); PAGE_ALIGN lost - lower layers do that for us? I don't have easy access to the tree at the moment to check and I kind of can't remember right now. > if (IS_ERR(vma)) > return PTR_ERR(vma); > > guc->ctx_pool_vma = vma; > ida_init(&guc->ctx_ids); > - guc_create_log(guc); > - guc_create_ads(guc); > + guc_log_create(guc); > + guc_addon_create(guc); > > return 0; > } > diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h > index 4678459..b1ba869 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -159,7 +159,7 @@ extern int intel_guc_resume(struct drm_device *dev); > /* i915_guc_submission.c */ > int i915_guc_submission_init(struct drm_i915_private *dev_priv); > int i915_guc_submission_enable(struct drm_i915_private *dev_priv); > -int i915_guc_wq_check_space(struct drm_i915_gem_request *rq); > +int i915_guc_wq_reserve(struct drm_i915_gem_request *rq); > void i915_guc_submission_disable(struct drm_i915_private *dev_priv); > void i915_guc_submission_fini(struct drm_i915_private *dev_priv); > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 16d7cdd..25114336 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -627,7 +627,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request > * going any further, as the i915_add_request() call > * later on mustn't fail ... > */ > - ret = i915_guc_wq_check_space(request); > + ret = i915_guc_wq_reserve(request); > if (ret) > return ret; > } Name changes make sense. Just the PAGE_ALIGN question above. Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] drm/i915/guc: general tidying up (submission) 2016-09-14 15:22 ` Tvrtko Ursulin @ 2016-09-14 17:00 ` Dave Gordon 2016-09-15 8:57 ` Tvrtko Ursulin 0 siblings, 1 reply; 11+ messages in thread From: Dave Gordon @ 2016-09-14 17:00 UTC (permalink / raw) To: Tvrtko Ursulin, intel-gfx On 14/09/16 16:22, Tvrtko Ursulin wrote: > > On 12/09/2016 21:19, Dave Gordon wrote: >> Renaming to more consistent scheme, and updating comments, mostly >> about i915_guc_wq_reserve(), aka i915_guc_wq_check_space(). >> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >> --- >> drivers/gpu/drm/i915/i915_guc_submission.c | 63 >> +++++++++++++++--------------- >> drivers/gpu/drm/i915/intel_guc.h | 2 +- >> drivers/gpu/drm/i915/intel_lrc.c | 2 +- >> 3 files changed, 34 insertions(+), 33 deletions(-) >> [snip] >> int i915_guc_submission_init(struct drm_i915_private *dev_priv) >> { >> + const size_t ctxsize = sizeof(struct guc_context_desc); >> + const size_t poolsize = GUC_MAX_GPU_CONTEXTS * ctxsize; >> + const size_t gemsize = round_up(poolsize, PAGE_SIZE); >> struct intel_guc *guc = &dev_priv->guc; >> struct i915_vma *vma; >> - u32 size; >> /* Wipe bitmap & delete client in case of reinitialisation */ >> bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS); >> @@ -985,15 +987,14 @@ int i915_guc_submission_init(struct >> drm_i915_private *dev_priv) >> if (guc->ctx_pool_vma) >> return 0; /* already allocated */ >> - size = PAGE_ALIGN(GUC_MAX_GPU_CONTEXTS*sizeof(struct >> guc_context_desc)); >> - vma = guc_allocate_vma(guc, size); >> + vma = guc_allocate_vma(guc, gemsize); > > PAGE_ALIGN lost - lower layers do that for us? I don't have easy access > to the tree at the moment to check and I kind of can't remember right now. PAGE_ALIGN here is replaced by using round_up(..., PAGE_SIZE) at the point where the constant is defined a few lines above. I think round_up() is clearer, because "align" could equally well mean round down. Anyway "align" (up or down) is something you do to addresses or offsets, not sizes. .Dave. >> if (IS_ERR(vma)) >> return PTR_ERR(vma); >> guc->ctx_pool_vma = vma; >> ida_init(&guc->ctx_ids); >> - guc_create_log(guc); >> - guc_create_ads(guc); >> + guc_log_create(guc); >> + guc_addon_create(guc); >> return 0; >> } >> diff --git a/drivers/gpu/drm/i915/intel_guc.h >> b/drivers/gpu/drm/i915/intel_guc.h >> index 4678459..b1ba869 100644 >> --- a/drivers/gpu/drm/i915/intel_guc.h >> +++ b/drivers/gpu/drm/i915/intel_guc.h >> @@ -159,7 +159,7 @@ extern int intel_guc_resume(struct drm_device *dev); >> /* i915_guc_submission.c */ >> int i915_guc_submission_init(struct drm_i915_private *dev_priv); >> int i915_guc_submission_enable(struct drm_i915_private *dev_priv); >> -int i915_guc_wq_check_space(struct drm_i915_gem_request *rq); >> +int i915_guc_wq_reserve(struct drm_i915_gem_request *rq); >> void i915_guc_submission_disable(struct drm_i915_private *dev_priv); >> void i915_guc_submission_fini(struct drm_i915_private *dev_priv); >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c >> b/drivers/gpu/drm/i915/intel_lrc.c >> index 16d7cdd..25114336 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -627,7 +627,7 @@ int intel_logical_ring_alloc_request_extras(struct >> drm_i915_gem_request *request >> * going any further, as the i915_add_request() call >> * later on mustn't fail ... >> */ >> - ret = i915_guc_wq_check_space(request); >> + ret = i915_guc_wq_reserve(request); >> if (ret) >> return ret; >> } > > Name changes make sense. Just the PAGE_ALIGN question above. > > Regards, > > Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] drm/i915/guc: general tidying up (submission) 2016-09-14 17:00 ` Dave Gordon @ 2016-09-15 8:57 ` Tvrtko Ursulin 2016-09-15 10:04 ` Chris Wilson 0 siblings, 1 reply; 11+ messages in thread From: Tvrtko Ursulin @ 2016-09-15 8:57 UTC (permalink / raw) To: Dave Gordon, intel-gfx On 14/09/2016 18:00, Dave Gordon wrote: > On 14/09/16 16:22, Tvrtko Ursulin wrote: >> >> On 12/09/2016 21:19, Dave Gordon wrote: >>> Renaming to more consistent scheme, and updating comments, mostly >>> about i915_guc_wq_reserve(), aka i915_guc_wq_check_space(). >>> >>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_guc_submission.c | 63 >>> +++++++++++++++--------------- >>> drivers/gpu/drm/i915/intel_guc.h | 2 +- >>> drivers/gpu/drm/i915/intel_lrc.c | 2 +- >>> 3 files changed, 34 insertions(+), 33 deletions(-) >>> > > [snip] > >>> int i915_guc_submission_init(struct drm_i915_private *dev_priv) >>> { >>> + const size_t ctxsize = sizeof(struct guc_context_desc); >>> + const size_t poolsize = GUC_MAX_GPU_CONTEXTS * ctxsize; >>> + const size_t gemsize = round_up(poolsize, PAGE_SIZE); >>> struct intel_guc *guc = &dev_priv->guc; >>> struct i915_vma *vma; >>> - u32 size; >>> /* Wipe bitmap & delete client in case of reinitialisation */ >>> bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS); >>> @@ -985,15 +987,14 @@ int i915_guc_submission_init(struct >>> drm_i915_private *dev_priv) >>> if (guc->ctx_pool_vma) >>> return 0; /* already allocated */ >>> - size = PAGE_ALIGN(GUC_MAX_GPU_CONTEXTS*sizeof(struct >>> guc_context_desc)); >>> - vma = guc_allocate_vma(guc, size); >>> + vma = guc_allocate_vma(guc, gemsize); >> >> PAGE_ALIGN lost - lower layers do that for us? I don't have easy access >> to the tree at the moment to check and I kind of can't remember right >> now. > > PAGE_ALIGN here is replaced by using round_up(..., PAGE_SIZE) at the > point where the constant is defined a few lines above. I think > round_up() is clearer, because "align" could equally well mean round > down. Anyway "align" (up or down) is something you do to addresses or > offsets, not sizes. I failed to spot that line. :( Should really take a long holiday. :) Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] drm/i915/guc: general tidying up (submission) 2016-09-15 8:57 ` Tvrtko Ursulin @ 2016-09-15 10:04 ` Chris Wilson 0 siblings, 0 replies; 11+ messages in thread From: Chris Wilson @ 2016-09-15 10:04 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: intel-gfx On Thu, Sep 15, 2016 at 09:57:18AM +0100, Tvrtko Ursulin wrote: > On 14/09/2016 18:00, Dave Gordon wrote: > >On 14/09/16 16:22, Tvrtko Ursulin wrote: > >> > >>On 12/09/2016 21:19, Dave Gordon wrote: > >>>Renaming to more consistent scheme, and updating comments, mostly > >>>about i915_guc_wq_reserve(), aka i915_guc_wq_check_space(). > >>> > >>>Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> And pushed. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* ✓ Fi.CI.BAT: success for (Final) tidying up of GuC code 2016-09-12 20:19 [PATCH 0/3] (Final) tidying up of GuC code Dave Gordon ` (2 preceding siblings ...) 2016-09-12 20:19 ` [PATCH 3/3] drm/i915/guc: general tidying up (submission) Dave Gordon @ 2016-09-12 21:20 ` Patchwork 3 siblings, 0 replies; 11+ messages in thread From: Patchwork @ 2016-09-12 21:20 UTC (permalink / raw) To: Dave Gordon; +Cc: intel-gfx == Series Details == Series: (Final) tidying up of GuC code URL : https://patchwork.freedesktop.org/series/12357/ State : success == Summary == Series 12357v1 (Final) tidying up of GuC code https://patchwork.freedesktop.org/api/1.0/series/12357/revisions/1/mbox/ Test kms_cursor_legacy: Subgroup basic-cursor-vs-flip-legacy: fail -> PASS (fi-bsw-n3050) fi-bdw-5557u total:254 pass:239 dwarn:0 dfail:0 fail:0 skip:15 fi-bsw-n3050 total:254 pass:208 dwarn:0 dfail:0 fail:0 skip:46 fi-byt-n2820 total:254 pass:212 dwarn:0 dfail:0 fail:1 skip:41 fi-hsw-4770k total:254 pass:232 dwarn:0 dfail:0 fail:0 skip:22 fi-hsw-4770r total:254 pass:228 dwarn:0 dfail:0 fail:0 skip:26 fi-ilk-650 total:254 pass:184 dwarn:0 dfail:0 fail:2 skip:68 fi-ivb-3520m total:254 pass:223 dwarn:0 dfail:0 fail:0 skip:31 fi-ivb-3770 total:254 pass:223 dwarn:0 dfail:0 fail:0 skip:31 fi-skl-6260u total:254 pass:240 dwarn:0 dfail:0 fail:0 skip:14 fi-skl-6700hq total:254 pass:227 dwarn:0 dfail:0 fail:1 skip:26 fi-skl-6700k total:254 pass:225 dwarn:1 dfail:0 fail:0 skip:28 fi-snb-2520m total:254 pass:209 dwarn:0 dfail:0 fail:0 skip:45 fi-snb-2600 total:254 pass:209 dwarn:0 dfail:0 fail:0 skip:45 Results at /archive/results/CI_IGT_test/Patchwork_2515/ 93d9d9dd030c6eb360b500bf9872663b30b5ebe5 drm-intel-nightly: 2016y-09m-12d-14h-34m-11s UTC integration manifest 9f221491 drm/i915/guc: general tidying up (submission) 6e8d886 drm/i915/guc: general tidying up (loader) 26c088f drm/i915: clarify PMINTRMSK/pm_intr_keep usage _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-09-15 10:04 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-12 20:19 [PATCH 0/3] (Final) tidying up of GuC code Dave Gordon 2016-09-12 20:19 ` [PATCH 1/3] drm/i915: clarify PMINTRMSK/pm_intr_keep usage Dave Gordon 2016-09-14 15:06 ` Tvrtko Ursulin 2016-09-12 20:19 ` [PATCH 2/3] drm/i915/guc: general tidying up (loader) Dave Gordon 2016-09-14 15:12 ` Tvrtko Ursulin 2016-09-12 20:19 ` [PATCH 3/3] drm/i915/guc: general tidying up (submission) Dave Gordon 2016-09-14 15:22 ` Tvrtko Ursulin 2016-09-14 17:00 ` Dave Gordon 2016-09-15 8:57 ` Tvrtko Ursulin 2016-09-15 10:04 ` Chris Wilson 2016-09-12 21:20 ` ✓ Fi.CI.BAT: success for (Final) tidying up of GuC code Patchwork
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).