On Thu, 15 Aug 2019, Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> wrote: > This is only required for a single platform so no need to reserve the > memory on all of them. > > This removes the last direct dependency of i915_drv.h on i915_reg.h > (apart from the i915_reg_t definition). > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> > Cc: Imre Deak <imre.deak@xxxxxxxxx> Heh, I've already sent a version of this [1], but I don't mind you finishing the job. Especially because I think it's better to handle the alloc/free explicitly instead of the way I do it. I do have some nitpicks on this one though, inline. [1] http://patchwork.freedesktop.org/patch/msgid/20190807144939.32123-1-jani.nikula@xxxxxxxxx > --- > drivers/gpu/drm/i915/i915_drv.c | 107 +++++++++++++++++++++++++++++--- > drivers/gpu/drm/i915/i915_drv.h | 64 +------------------ > 2 files changed, 100 insertions(+), 71 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 2541a3a1c229..1723b2ddfccd 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -80,6 +80,68 @@ > > static struct drm_driver driver; > > +struct vlv_s0ix_state { > + /* GAM */ > + u32 wr_watermark; > + u32 gfx_prio_ctrl; > + u32 arb_mode; > + u32 gfx_pend_tlb0; > + u32 gfx_pend_tlb1; > + u32 lra_limits[GEN7_LRA_LIMITS_REG_NUM]; > + u32 media_max_req_count; > + u32 gfx_max_req_count; > + u32 render_hwsp; > + u32 ecochk; > + u32 bsd_hwsp; > + u32 blt_hwsp; > + u32 tlb_rd_addr; > + > + /* MBC */ > + u32 g3dctl; > + u32 gsckgctl; > + u32 mbctl; > + > + /* GCP */ > + u32 ucgctl1; > + u32 ucgctl3; > + u32 rcgctl1; > + u32 rcgctl2; > + u32 rstctl; > + u32 misccpctl; > + > + /* GPM */ > + u32 gfxpause; > + u32 rpdeuhwtc; > + u32 rpdeuc; > + u32 ecobus; > + u32 pwrdwnupctl; > + u32 rp_down_timeout; > + u32 rp_deucsw; > + u32 rcubmabdtmr; > + u32 rcedata; > + u32 spare2gh; > + > + /* Display 1 CZ domain */ > + u32 gt_imr; > + u32 gt_ier; > + u32 pm_imr; > + u32 pm_ier; > + u32 gt_scratch[GEN7_GT_SCRATCH_REG_NUM]; > + > + /* GT SA CZ domain */ > + u32 tilectl; > + u32 gt_fifoctl; > + u32 gtlc_wake_ctrl; > + u32 gtlc_survive; > + u32 pmwgicz; > + > + /* Display 2 CZ domain */ > + u32 gu_ctl0; > + u32 gu_ctl1; > + u32 pcbr; > + u32 clock_gate_dis2; > +}; > + > static int i915_get_bridge_dev(struct drm_i915_private *dev_priv) > { > int domain = pci_domain_nr(dev_priv->drm.pdev->bus); > @@ -466,6 +528,28 @@ static void intel_detect_preproduction_hw(struct drm_i915_private *dev_priv) > } > } > > +static int vlv_alloc_s0ix_state(struct drm_i915_private *i915) > +{ > + if (!IS_VALLEYVIEW(i915)) > + return 0; > + > + /* we write all the values in the structure, so no need to zero it out */ > + i915->s0ix_state = kmalloc(sizeof(struct vlv_s0ix_state), GFP_KERNEL); > + if (!i915->s0ix_state) > + return -ENOMEM; > + > + return 0; > +} > + > +static void vlv_free_s0ix_state(struct drm_i915_private *i915) > +{ > + if (!i915->s0ix_state) > + return; > + > + kfree(i915->s0ix_state); > + i915->s0ix_state = NULL; > +} > + > /** > * i915_driver_early_probe - setup state not requiring device access > * @dev_priv: device private > @@ -508,13 +592,17 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv) > if (ret < 0) > return ret; > > + ret = vlv_alloc_s0ix_state(dev_priv); > + if (ret < 0) > + goto err_workqueues; > + > intel_wopcm_init_early(&dev_priv->wopcm); > > intel_gt_init_early(&dev_priv->gt, dev_priv); > > ret = i915_gem_init_early(dev_priv); > if (ret < 0) > - goto err_workqueues; > + goto err_gt; > > /* This must be called before any calls to HAS_PCH_* */ > intel_detect_pch(dev_priv); > @@ -536,8 +624,10 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv) > > err_gem: > i915_gem_cleanup_early(dev_priv); > -err_workqueues: > +err_gt: > intel_gt_driver_late_release(&dev_priv->gt); > + vlv_free_s0ix_state(dev_priv); > +err_workqueues: > i915_workqueues_cleanup(dev_priv); > return ret; > } > @@ -553,6 +643,7 @@ static void i915_driver_late_release(struct drm_i915_private *dev_priv) > intel_power_domains_cleanup(dev_priv); > i915_gem_cleanup_early(dev_priv); > intel_gt_driver_late_release(&dev_priv->gt); > + vlv_free_s0ix_state(dev_priv); > i915_workqueues_cleanup(dev_priv); > > pm_qos_remove_request(&dev_priv->sb_qos); > @@ -2137,7 +2228,7 @@ static int i915_pm_restore(struct device *kdev) > */ > static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv) > { > - struct vlv_s0ix_state *s = &dev_priv->vlv_s0ix_state; > + struct vlv_s0ix_state *s = dev_priv->s0ix_state; I think I'd now call this function unconditionally, and return early if (!s). This puts the decision to do this or not in one place only, in vlv_alloc_s0ix_state(), instead of duplicating the conditions. > int i; > > /* GAM 0x4000-0x4770 */ > @@ -2147,7 +2238,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv) > s->gfx_pend_tlb0 = I915_READ(GEN7_GFX_PEND_TLB0); > s->gfx_pend_tlb1 = I915_READ(GEN7_GFX_PEND_TLB1); > > - for (i = 0; i < ARRAY_SIZE(s->lra_limits); i++) > + for (i = 0; i < GEN7_LRA_LIMITS_REG_NUM; i++) > s->lra_limits[i] = I915_READ(GEN7_LRA_LIMITS(i)); > > s->media_max_req_count = I915_READ(GEN7_MEDIA_MAX_REQ_COUNT); > @@ -2191,7 +2282,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv) > s->pm_imr = I915_READ(GEN6_PMIMR); > s->pm_ier = I915_READ(GEN6_PMIER); > > - for (i = 0; i < ARRAY_SIZE(s->gt_scratch); i++) > + for (i = 0; i < GEN7_GT_SCRATCH_REG_NUM; i++) > s->gt_scratch[i] = I915_READ(GEN7_GT_SCRATCH(i)); The above two hunks are in the wrong patch. > > /* GT SA CZ domain, 0x100000-0x138124 */ > @@ -2218,7 +2309,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv) > > static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv) > { > - struct vlv_s0ix_state *s = &dev_priv->vlv_s0ix_state; > + struct vlv_s0ix_state *s = dev_priv->s0ix_state; Early return on !s here as well, and call the function unconditionally. > u32 val; > int i; > > @@ -2229,7 +2320,7 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv) > I915_WRITE(GEN7_GFX_PEND_TLB0, s->gfx_pend_tlb0); > I915_WRITE(GEN7_GFX_PEND_TLB1, s->gfx_pend_tlb1); > > - for (i = 0; i < ARRAY_SIZE(s->lra_limits); i++) > + for (i = 0; i < GEN7_LRA_LIMITS_REG_NUM; i++) > I915_WRITE(GEN7_LRA_LIMITS(i), s->lra_limits[i]); > > I915_WRITE(GEN7_MEDIA_MAX_REQ_COUNT, s->media_max_req_count); > @@ -2273,7 +2364,7 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv) > I915_WRITE(GEN6_PMIMR, s->pm_imr); > I915_WRITE(GEN6_PMIER, s->pm_ier); > > - for (i = 0; i < ARRAY_SIZE(s->gt_scratch); i++) > + for (i = 0; i < GEN7_GT_SCRATCH_REG_NUM; i++) > I915_WRITE(GEN7_GT_SCRATCH(i), s->gt_scratch[i]); The above two hunks are in the wrong patch. > > /* GT SA CZ domain, 0x100000-0x138124 */ > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c6722d54ccd5..9b41f2209b69 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -527,68 +527,6 @@ struct i915_suspend_saved_registers { > u16 saveGCDGMBUS; > }; > > -struct vlv_s0ix_state { > - /* GAM */ > - u32 wr_watermark; > - u32 gfx_prio_ctrl; > - u32 arb_mode; > - u32 gfx_pend_tlb0; > - u32 gfx_pend_tlb1; > - u32 lra_limits[GEN7_LRA_LIMITS_REG_NUM]; > - u32 media_max_req_count; > - u32 gfx_max_req_count; > - u32 render_hwsp; > - u32 ecochk; > - u32 bsd_hwsp; > - u32 blt_hwsp; > - u32 tlb_rd_addr; > - > - /* MBC */ > - u32 g3dctl; > - u32 gsckgctl; > - u32 mbctl; > - > - /* GCP */ > - u32 ucgctl1; > - u32 ucgctl3; > - u32 rcgctl1; > - u32 rcgctl2; > - u32 rstctl; > - u32 misccpctl; > - > - /* GPM */ > - u32 gfxpause; > - u32 rpdeuhwtc; > - u32 rpdeuc; > - u32 ecobus; > - u32 pwrdwnupctl; > - u32 rp_down_timeout; > - u32 rp_deucsw; > - u32 rcubmabdtmr; > - u32 rcedata; > - u32 spare2gh; > - > - /* Display 1 CZ domain */ > - u32 gt_imr; > - u32 gt_ier; > - u32 pm_imr; > - u32 pm_ier; > - u32 gt_scratch[GEN7_GT_SCRATCH_REG_NUM]; > - > - /* GT SA CZ domain */ > - u32 tilectl; > - u32 gt_fifoctl; > - u32 gtlc_wake_ctrl; > - u32 gtlc_survive; > - u32 pmwgicz; > - > - /* Display 2 CZ domain */ > - u32 gu_ctl0; > - u32 gu_ctl1; > - u32 pcbr; > - u32 clock_gate_dis2; > -}; > - > struct intel_rps_ei { > ktime_t ktime; > u32 render_c0; > @@ -1622,7 +1560,7 @@ struct drm_i915_private { > u32 suspend_count; > bool power_domains_suspended; > struct i915_suspend_saved_registers regfile; > - struct vlv_s0ix_state vlv_s0ix_state; > + void *s0ix_state; I'd keep the vlv_ prefix in the member name. BR, Jani. > > enum { > I915_SAGV_UNKNOWN = 0, -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
