Re: [PATCH 4/6] drm/i915: Dynamically allocate s0ix struct for VLV

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 8/16/19 2:35 AM, Jani Nikula wrote:
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.

D'oh! I completely missed that


I do have some nitpicks on this one though, inline.


[1] http://patchwork.freedesktop.org/patch/msgid/20190807144939.32123-1-jani.nikula@xxxxxxxxx



<snip>


@@ -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.

ack



  	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.


Yup, leftovers from a previous version.

/* 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.


ok

  	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.

ok (including the fwd decl mentioned in the other reply)

Daniele


BR,
Jani.


enum {
  		I915_SAGV_UNKNOWN = 0,

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux