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

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 8/16/19 2:35 AM, Jani Nikula wrote:
> On Thu, 15 Aug 2019, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> 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@intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
> 
> 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@intel.com
> 
> 

<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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx