All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jani Nikula <jani.nikula@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915/display: use intel_de_*() functions for register access
Date: Tue, 18 Feb 2020 11:31:05 +0200	[thread overview]
Message-ID: <87eeusz1gm.fsf@intel.com> (raw)
In-Reply-To: <20200214142414.GJ13686@intel.com>

On Fri, 14 Feb 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Feb 14, 2020 at 04:09:10PM +0200, Jani Nikula wrote:
>> The implicit "dev_priv" local variable use has been a long-standing pain
>> point in the register access macros I915_READ(), I915_WRITE(),
>> POSTING_READ(), I915_READ_FW(), and I915_WRITE_FW().
>> 
>> Replace them with the corresponding new display engine register
>> accessors intel_de_read(), intel_de_write(), intel_de_posting_read(),
>> intel_de_read_fw(), and intel_de_write_fw().
>> 
>> No functional changes.
>> 
>> Generated using the following semantic patch:
>> 
>> @@
>> expression REG, OFFSET;
>> @@
>> - I915_READ(REG)
>> + intel_de_read(dev_priv, REG)
>> 
>> @@
>> expression REG, OFFSET;
>> @@
>> - POSTING_READ(REG)
>> + intel_de_posting_read(dev_priv, REG)
>> 
>> @@
>> expression REG, OFFSET;
>> @@
>> - I915_WRITE(REG, OFFSET)
>> + intel_de_write(dev_priv, REG, OFFSET)
>> 
>> @@
>> expression REG;
>> @@
>> - I915_READ_FW(REG)
>> + intel_de_read_fw(dev_priv, REG)
>> 
>> @@
>> expression REG, OFFSET;
>> @@
>> - I915_WRITE_FW(REG, OFFSET)
>> + intel_de_write_fw(dev_priv, REG, OFFSET)
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  .../drm/i915/display/intel_display_debugfs.c  | 65 +++++++++++--------
>>  1 file changed, 37 insertions(+), 28 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> index 50baac726e70..287442bbcb46 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> @@ -57,15 +57,15 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>>  		u32 mask;
>>  
>>  		if (INTEL_GEN(dev_priv) >= 8)
>> -			mask = I915_READ(IVB_FBC_STATUS2) & BDW_FBC_COMP_SEG_MASK;
>> +			mask = intel_de_read(dev_priv, IVB_FBC_STATUS2) & BDW_FBC_COMP_SEG_MASK;
>>  		else if (INTEL_GEN(dev_priv) >= 7)
>> -			mask = I915_READ(IVB_FBC_STATUS2) & IVB_FBC_COMP_SEG_MASK;
>> +			mask = intel_de_read(dev_priv, IVB_FBC_STATUS2) & IVB_FBC_COMP_SEG_MASK;
>>  		else if (INTEL_GEN(dev_priv) >= 5)
>> -			mask = I915_READ(ILK_DPFC_STATUS) & ILK_DPFC_COMP_SEG_MASK;
>> +			mask = intel_de_read(dev_priv, ILK_DPFC_STATUS) & ILK_DPFC_COMP_SEG_MASK;
>>  		else if (IS_G4X(dev_priv))
>> -			mask = I915_READ(DPFC_STATUS) & DPFC_COMP_SEG_MASK;
>> +			mask = intel_de_read(dev_priv, DPFC_STATUS) & DPFC_COMP_SEG_MASK;
>>  		else
>> -			mask = I915_READ(FBC_STATUS) & (FBC_STAT_COMPRESSING |
>> +			mask = intel_de_read(dev_priv, FBC_STATUS) & (FBC_STAT_COMPRESSING |
>>  							FBC_STAT_COMPRESSED);
>
> Seesm to want some manual work to fix the alignment afterwards.
> Probably should wrap after & actually.

Thanks for the review, pushed with this whitespace change applied.

BR,
Jani.

>
>>  
>>  		seq_printf(m, "Compressing: %s\n", yesno(mask));
>> @@ -99,12 +99,11 @@ static int i915_fbc_false_color_set(void *data, u64 val)
>>  
>>  	mutex_lock(&dev_priv->fbc.lock);
>>  
>> -	reg = I915_READ(ILK_DPFC_CONTROL);
>> +	reg = intel_de_read(dev_priv, ILK_DPFC_CONTROL);
>>  	dev_priv->fbc.false_color = val;
>>  
>> -	I915_WRITE(ILK_DPFC_CONTROL, val ?
>> -		   (reg | FBC_CTL_FALSE_COLOR) :
>> -		   (reg & ~FBC_CTL_FALSE_COLOR));
>> +	intel_de_write(dev_priv, ILK_DPFC_CONTROL,
>> +		       val ? (reg | FBC_CTL_FALSE_COLOR) : (reg & ~FBC_CTL_FALSE_COLOR));
>
> Did we introduce some kind of rmw() variant? Could use it here to get
> rid of this rather ugly construct.
>
> For the series
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>>  
>>  	mutex_unlock(&dev_priv->fbc.lock);
>>  	return 0;
>> @@ -130,7 +129,7 @@ static int i915_ips_status(struct seq_file *m, void *unused)
>>  	if (INTEL_GEN(dev_priv) >= 8) {
>>  		seq_puts(m, "Currently: unknown\n");
>>  	} else {
>> -		if (I915_READ(IPS_CTL) & IPS_ENABLE)
>> +		if (intel_de_read(dev_priv, IPS_CTL) & IPS_ENABLE)
>>  			seq_puts(m, "Currently: enabled\n");
>>  		else
>>  			seq_puts(m, "Currently: disabled\n");
>> @@ -152,16 +151,16 @@ static int i915_sr_status(struct seq_file *m, void *unused)
>>  	if (INTEL_GEN(dev_priv) >= 9)
>>  		/* no global SR status; inspect per-plane WM */;
>>  	else if (HAS_PCH_SPLIT(dev_priv))
>> -		sr_enabled = I915_READ(WM1_LP_ILK) & WM1_LP_SR_EN;
>> +		sr_enabled = intel_de_read(dev_priv, WM1_LP_ILK) & WM1_LP_SR_EN;
>>  	else if (IS_I965GM(dev_priv) || IS_G4X(dev_priv) ||
>>  		 IS_I945G(dev_priv) || IS_I945GM(dev_priv))
>> -		sr_enabled = I915_READ(FW_BLC_SELF) & FW_BLC_SELF_EN;
>> +		sr_enabled = intel_de_read(dev_priv, FW_BLC_SELF) & FW_BLC_SELF_EN;
>>  	else if (IS_I915GM(dev_priv))
>> -		sr_enabled = I915_READ(INSTPM) & INSTPM_SELF_EN;
>> +		sr_enabled = intel_de_read(dev_priv, INSTPM) & INSTPM_SELF_EN;
>>  	else if (IS_PINEVIEW(dev_priv))
>> -		sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN;
>> +		sr_enabled = intel_de_read(dev_priv, DSPFW3) & PINEVIEW_SELF_REFRESH_EN;
>>  	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>> -		sr_enabled = I915_READ(FW_BLC_SELF_VLV) & FW_CSPWRDWNEN;
>> +		sr_enabled = intel_de_read(dev_priv, FW_BLC_SELF_VLV) & FW_CSPWRDWNEN;
>>  
>>  	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT, wakeref);
>>  
>> @@ -298,7 +297,8 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m)
>>  			"BUF_ON",
>>  			"TG_ON"
>>  		};
>> -		val = I915_READ(EDP_PSR2_STATUS(dev_priv->psr.transcoder));
>> +		val = intel_de_read(dev_priv,
>> +				    EDP_PSR2_STATUS(dev_priv->psr.transcoder));
>>  		status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
>>  			      EDP_PSR2_STATUS_STATE_SHIFT;
>>  		if (status_val < ARRAY_SIZE(live_status))
>> @@ -314,7 +314,8 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m)
>>  			"SRDOFFACK",
>>  			"SRDENT_ON",
>>  		};
>> -		val = I915_READ(EDP_PSR_STATUS(dev_priv->psr.transcoder));
>> +		val = intel_de_read(dev_priv,
>> +				    EDP_PSR_STATUS(dev_priv->psr.transcoder));
>>  		status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
>>  			      EDP_PSR_STATUS_STATE_SHIFT;
>>  		if (status_val < ARRAY_SIZE(live_status))
>> @@ -361,10 +362,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>>  	}
>>  
>>  	if (psr->psr2_enabled) {
>> -		val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
>> +		val = intel_de_read(dev_priv,
>> +				    EDP_PSR2_CTL(dev_priv->psr.transcoder));
>>  		enabled = val & EDP_PSR2_ENABLE;
>>  	} else {
>> -		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
>> +		val = intel_de_read(dev_priv,
>> +				    EDP_PSR_CTL(dev_priv->psr.transcoder));
>>  		enabled = val & EDP_PSR_ENABLE;
>>  	}
>>  	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
>> @@ -377,7 +380,8 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>>  	 * SKL+ Perf counter is reset to 0 everytime DC state is entered
>>  	 */
>>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>> -		val = I915_READ(EDP_PSR_PERF_CNT(dev_priv->psr.transcoder));
>> +		val = intel_de_read(dev_priv,
>> +				    EDP_PSR_PERF_CNT(dev_priv->psr.transcoder));
>>  		val &= EDP_PSR_PERF_CNT_MASK;
>>  		seq_printf(m, "Performance counter: %u\n", val);
>>  	}
>> @@ -397,8 +401,8 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>>  		 * frame boundary between register reads
>>  		 */
>>  		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame += 3) {
>> -			val = I915_READ(PSR2_SU_STATUS(dev_priv->psr.transcoder,
>> -						       frame));
>> +			val = intel_de_read(dev_priv,
>> +					    PSR2_SU_STATUS(dev_priv->psr.transcoder, frame));
>>  			su_frames_val[frame / 3] = val;
>>  		}
>>  
>> @@ -518,7 +522,8 @@ static int i915_dmc_info(struct seq_file *m, void *unused)
>>  		 * reg for DC3CO debugging and validation,
>>  		 * but TGL DMC f/w is using DMC_DEBUG3 reg for DC3CO counter.
>>  		 */
>> -		seq_printf(m, "DC3CO count: %d\n", I915_READ(DMC_DEBUG3));
>> +		seq_printf(m, "DC3CO count: %d\n",
>> +			   intel_de_read(dev_priv, DMC_DEBUG3));
>>  	} else {
>>  		dc5_reg = IS_BROXTON(dev_priv) ? BXT_CSR_DC3_DC5_COUNT :
>>  						 SKL_CSR_DC3_DC5_COUNT;
>> @@ -526,14 +531,18 @@ static int i915_dmc_info(struct seq_file *m, void *unused)
>>  			dc6_reg = SKL_CSR_DC5_DC6_COUNT;
>>  	}
>>  
>> -	seq_printf(m, "DC3 -> DC5 count: %d\n", I915_READ(dc5_reg));
>> +	seq_printf(m, "DC3 -> DC5 count: %d\n",
>> +		   intel_de_read(dev_priv, dc5_reg));
>>  	if (dc6_reg.reg)
>> -		seq_printf(m, "DC5 -> DC6 count: %d\n", I915_READ(dc6_reg));
>> +		seq_printf(m, "DC5 -> DC6 count: %d\n",
>> +			   intel_de_read(dev_priv, dc6_reg));
>>  
>>  out:
>> -	seq_printf(m, "program base: 0x%08x\n", I915_READ(CSR_PROGRAM(0)));
>> -	seq_printf(m, "ssp base: 0x%08x\n", I915_READ(CSR_SSP_BASE));
>> -	seq_printf(m, "htp: 0x%08x\n", I915_READ(CSR_HTP_SKL));
>> +	seq_printf(m, "program base: 0x%08x\n",
>> +		   intel_de_read(dev_priv, CSR_PROGRAM(0)));
>> +	seq_printf(m, "ssp base: 0x%08x\n",
>> +		   intel_de_read(dev_priv, CSR_SSP_BASE));
>> +	seq_printf(m, "htp: 0x%08x\n", intel_de_read(dev_priv, CSR_HTP_SKL));
>>  
>>  	intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref);
>>  
>> -- 
>> 2.20.1
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  parent reply	other threads:[~2020-02-18  9:31 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-14 14:09 [Intel-gfx] [PATCH 1/2] drm/i915/csr: use intel_de_*() functions for register access Jani Nikula
2020-02-14 14:09 ` [Intel-gfx] [PATCH 2/2] drm/i915/display: " Jani Nikula
2020-02-14 14:24   ` Ville Syrjälä
2020-02-14 14:27     ` Jani Nikula
2020-02-18  9:31     ` Jani Nikula [this message]
2020-02-14 20:54 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/csr: " Patchwork
2020-02-18  2:09 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87eeusz1gm.fsf@intel.com \
    --to=jani.nikula@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=ville.syrjala@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.