Re: [PATCH v4 3/4] drm/i915: Make PSR registers relative to transcoders

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

 



On Wed, 2019-04-17 at 15:37 -0700, José Roberto de Souza wrote:
> PSR registers are a mess, some have the full address while others just
> have the additional offset from psr_mmio_base.
> 
> psr_mmio_base is nothing more than TRANSCODER_EDP_OFFSET + 0x800 and
> using it makes more difficult for people with an PSR register address
> or PSR register name from from BSpec as i915 also don't match the
> name.
I'd like this to be rewritten for clarity.

> 
> Other reason to make relative to transcoder is that since BDW every
> transcoder have PSR registers, so in theory it should be possible to
> have PSR enabled in a non-eDP transcoder.
> 
> For Haswell PSR registers are relative to 0x64000, so
> mmio_base_adjust and _TRANS2_PSR() was added so we can overcome
> this limitation in a single place.
> PSR2 registers and PSR_EVENT was added after Haswell so that is why
> _TRANS2_PSR() is not used in some macros.
> 
> The only registers that can not be relative to transcoder are
> PSR_IMR and PSR_IIR that are not relative to anything, so keeping it
> hardcoded.
> 
> Also removing BDW_EDP_PSR_BASE from GVT because it is not used as it
> is the only PSR register that GVT have.
> 
> v4:
> - Moved definition of _TRANS2_PSR() and _MMIO_TRANS2_PSR() to the
> beginning of i915_reg.h (Jani)
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Zhi Wang <zhi.a.wang@xxxxxxxxx>
> Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/gvt/handlers.c |  1 -
>  drivers/gpu/drm/i915/i915_drv.h     |  5 +--
>  drivers/gpu/drm/i915/i915_reg.h     | 48 ++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_psr.c    | 11 +++++--
>  4 files changed, 45 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
> b/drivers/gpu/drm/i915/gvt/handlers.c
> index 18f01eeb2510..749e3e4204f2 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -2776,7 +2776,6 @@ static int init_broadwell_mmio_info(struct intel_gvt
> *gvt)
>  	MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
>  
>  	MMIO_D(WM_MISC, D_BDW);
> -	MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
>  
>  	MMIO_D(_MMIO(0x6671c), D_BDW_PLUS);
>  	MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 066fd2a12851..58748f23511f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -494,6 +494,8 @@ struct i915_drrs {
>  };
>  
>  struct i915_psr {
> +	/* different than zero only on HSW see _TRANS2_PSR() for more info */
> +	u32 mmio_base_adjust;

If you rename this to hsw_base_adjust, the name becomes self-documenting and we
can get rid of the comment.

>  	struct mutex lock;
>  
>  #define I915_PSR_DEBUG_MODE_MASK	0x0f
> @@ -508,6 +510,7 @@ struct i915_psr {
>  	bool enabled;
>  	struct intel_dp *dp;
>  	enum pipe pipe;
> +	enum transcoder transcoder;
>  	bool active;
>  	struct work_struct work;
>  	unsigned busy_frontbuffer_bits;
> @@ -1534,8 +1537,6 @@ struct drm_i915_private {
>  	/* MMIO base address for MIPI regs */
>  	u32 mipi_mmio_base;
>  
> -	u32 psr_mmio_base;
> -
>  	u32 pps_mmio_base;
>  
>  	wait_queue_head_t gmbus_wait_queue;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9ef306b79e0d..987006946802 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -258,6 +258,10 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  					      INTEL_INFO(dev_priv)-
> >cursor_offsets[PIPE_A] + (reg) + \
>  					      DISPLAY_MMIO_BASE(dev_priv))
>  
> +/* PSR registers on HSW is not relative to eDP transcoder */
> +#define _TRANS2_PSR(reg)	(_TRANS2(dev_priv->psr.transcoder, (reg)) -
> dev_priv->psr.mmio_base_adjust)
> +#define _MMIO_TRANS2_PSR(reg)	_MMIO(_TRANS2_PSR(reg))
> +

I think the macro should be defined in it's final form in this commit. While I
understand you want to split the patch for making review easy, not convinced we
it's a good idea to add macros that change in the next patch.

We can define the macro to accept (tran, reg) as arguments in this patch but
have intel_psr_compute_config() reject non-eDP transcoders.

intel_psr_compute_config(intel_dp, crtc_state)
{
	if (crtc_state->cpu_transcoder
!= TRANSCODER_EDP)
		return;
	...
}
This allows you to add the macro without changing any functionality.


If you insist on not making changes to intel_psr.c, how about introducing the
full macro in this patch but hard-coding TRANCODER_EDP

#define PSR_CTL(tran)	_MMIO_TRANS2_PSR(TRANSCODER_EDP, _SRD_CTL_A)
#define PSR_STATUS(tran) _MMIO_TRANS2_PSR(TRANSCODER_EDP, _SRD_STATUS_A)

Comments about _MMIO_TRANS2_PSR below.


>  #define __MASKED_FIELD(mask, value) ((mask) << 16 | (value))
>  #define _MASKED_FIELD(mask, value) ({					
>    \
>  	if (__builtin_constant_p(mask))					   \
> @@ -4213,9 +4217,11 @@ enum {
>  #define PIPE_MULT(trans)	_MMIO_TRANS2(trans, _PIPE_MULT_A)
>  
>  /* HSW+ eDP PSR registers */
> -#define HSW_EDP_PSR_BASE	0x64800
> -#define BDW_EDP_PSR_BASE	0x6f800
> -#define EDP_PSR_CTL				_MMIO(dev_priv->psr_mmio_base +
> 0)
> +#define HSW_EDP_PSR_BASE	0x64000
> +
> +#define _SRD_CTL_A				0x60800
> +#define _SRD_CTL_EDP				0x6F800
						   ^f (use lower case for hex digits) here and elsewhere in this patch.

> +#define EDP_PSR_CTL				_MMIO_TRANS2_PSR(_SRD_CTL_A)
>  #define   EDP_PSR_ENABLE			(1 << 31)
>  #define   BDW_PSR_SINGLE_FRAME			(1 << 30)
>  #define   EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK	(1 << 29) /* SW can't
> modify */
> @@ -4252,16 +4258,22 @@ enum {
>  #define   EDP_PSR_TRANSCODER_A_SHIFT		8
>  #define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
>  
> -#define EDP_PSR_AUX_CTL				_MMIO(dev_priv-
> >psr_mmio_base + 0x10)
> +#define _SRD_AUX_CTL_A				0x60810
> +#define _SRD_AUX_CTL_EDP			0x6F810
> +#define EDP_PSR_AUX_CTL				_MMIO_TRANS2_PSR(_SRD_AU
> X_CTL_A)
>  #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 << 26)
>  #define   EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK	(0x1f << 20)
>  #define   EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK	(0xf << 16)
>  #define   EDP_PSR_AUX_CTL_ERROR_INTERRUPT	(1 << 11)
>  #define   EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK	(0x7ff)
>  
> -#define EDP_PSR_AUX_DATA(i)			_MMIO(dev_priv->psr_mmio_base +
> 0x14 + (i) * 4) /* 5 registers */
> +#define _SRD_AUX_DATA_A				0x60814
> +#define _SRD_AUX_DATA_EDP			0x6F814
> +#define EDP_PSR_AUX_DATA(i)			_MMIO(_TRANS2_PSR(_SRD_AUX_DATA_
> A) + (i) + 4) /* 5 registers */
>  
> -#define EDP_PSR_STATUS				_MMIO(dev_priv-
> >psr_mmio_base + 0x40)
> +#define _SRD_STATUS_A				0x60840
> +#define _SRD_STATUS_EDP				0x6F840
> +#define EDP_PSR_STATUS				_MMIO_TRANS2_PSR(_SRD_ST
> ATUS_A)
>  #define   EDP_PSR_STATUS_STATE_MASK		(7 << 29)
>  #define   EDP_PSR_STATUS_STATE_SHIFT		29
>  #define   EDP_PSR_STATUS_STATE_IDLE		(0 << 29)
> @@ -4286,10 +4298,15 @@ enum {
>  #define   EDP_PSR_STATUS_SENDING_TP1		(1 << 4)
>  #define   EDP_PSR_STATUS_IDLE_MASK		0xf
>  
> -#define EDP_PSR_PERF_CNT		_MMIO(dev_priv->psr_mmio_base + 0x44)
> +#define _SRD_PERF_CNT_A			0x60844
> +#define _SRD_PERF_CNT_EDP		0x6F844
> +#define EDP_PSR_PERF_CNT		_MMIO_TRANS2_PSR(_SRD_PERF_CNT_A)
>  #define   EDP_PSR_PERF_CNT_MASK		0xffffff
>  
> -#define EDP_PSR_DEBUG				_MMIO(dev_priv-
> >psr_mmio_base + 0x60) /* PSR_MASK on SKL+ */
> +/* PSR_MASK on SKL+ */
> +#define _SRD_DEBUG_A				0x60860
> +#define _SRD_DEBUG_EDP				0x6F860
> +#define EDP_PSR_DEBUG				_MMIO_TRANS2_PSR(_SRD_DE
> BUG_A)
>  #define   EDP_PSR_DEBUG_MASK_MAX_SLEEP         (1 << 28)
>  #define   EDP_PSR_DEBUG_MASK_LPSP              (1 << 27)
>  #define   EDP_PSR_DEBUG_MASK_MEMUP             (1 << 26)
> @@ -4297,7 +4314,9 @@ enum {
>  #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /* Reserved in ICL+
> */
>  #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+ */
>  
> -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> +#define _PSR2_CTL_A			0x60900
> +#define _PSR2_CTL_EDP			0x6F900
> +#define EDP_PSR2_CTL			_MMIO_TRANS2(dev_priv->psr.transcoder,
> _PSR2_CTL_A)
>  #define   EDP_PSR2_ENABLE		(1 << 31)
>  #define   EDP_SU_TRACK_ENABLE		(1 << 30)
>  #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
> @@ -4338,14 +4357,15 @@ enum {
>  #define  PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
>  #define  PSR_EVENT_PSR_DISABLE			(1 << 0)
>  
> -#define EDP_PSR2_STATUS			_MMIO(0x6f940)
> +#define _PSR2_STATUS_A			0x60940
This is a completely made up register, isn't it? 

> +#define _PSR2_STATUS_EDP		0x6F940
> +#define EDP_PSR2_STATUS			_MMIO_TRANS2(dev_priv-
> >psr.transcoder, _PSR2_STATUS_A)
>  #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
>  #define EDP_PSR2_STATUS_STATE_SHIFT    28
>  
> -#define _PSR2_SU_STATUS_0		0x6F914
> -#define _PSR2_SU_STATUS_1		0x6F918
> -#define _PSR2_SU_STATUS_2		0x6F91C
> -#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index),
> _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
> +#define _PSR2_SU_STATUS_A		0x60914
> +#define _PSR2_SU_STATUS_EDP		0x6F914
> +#define _PSR2_SU_STATUS(index)		_MMIO(_TRANS2(dev_priv-
> >psr.transcoder, _PSR2_SU_STATUS_A) + (index) * 4)
>  #define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame) / 3))
>  #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
>  #define PSR2_SU_STATUS_MASK(frame)	(0x3ff << PSR2_SU_STATUS_SHIFT(frame))
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 963663ba0edf..da351c8f86d7 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -742,6 +742,14 @@ static void intel_psr_enable_locked(struct
> drm_i915_private *dev_priv,
>  	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
>  	dev_priv->psr.busy_frontbuffer_bits = 0;
>  	dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)->pipe;
> +	dev_priv->psr.transcoder = crtc_state->cpu_transcoder;
> +
> +	if (IS_HASWELL(dev_priv)) {
> +		u32 trans_offset = INTEL_INFO(dev_priv)->trans_offsets[dev_priv-
> >psr.transcoder];
> +
> +		WARN_ON(trans_offset < HSW_EDP_PSR_BASE);
> +		dev_priv->psr.mmio_base_adjust = trans_offset -
> HSW_EDP_PSR_BASE;

The HSW PSR registers are not per-transcoder registers and as such it just feels
wrong to compute .mmio_base_adjust based on the transcoder. Here's what I
propose


#define HSW_EDP_PSR_BASE		0x64800
#define _SRD_CTL_A			0x60800
#define _MMIO_HSW_PSR_ADJ(reg)		_MMIO(reg - _SRD_CTL_A + HSW_EDP_PSR_BASE)
#define _MMIO_PSR_ADJ(tran, reg)	IS_HASWELL(dev_priv) ? 	\								_MMIO_HS
W_PSR_ADJ(reg) :\
 	                                       _MMIO_TRANS2(tran, reg)
#define EDP_PSR_CTL(tran)       _MMIO_PSR_ADJ(tran, _SRD_CTL_A)
#define EDP_PSR_STATUS(tran)	_MMIO_PSR_ADJ(tran, _SRD_STATUS_A)
...

Or 

#define HSW_EDP_PSR_BASE                        0x64800
#define BDW_EDP_PSR_BASE                        0x6f800
#define _MMIO_PSR_ADJ(tran, reg)		_MMIO_TRANS2(tran, reg - dev_priv-
>psr.hsw_base_adj)
#define EDP_PSR_CTL(tran)                       _MMIO_PSR_ADJ(tran, _SRD_CTL_A)

with

> +	}
>  
>  	DRM_DEBUG_KMS("Enabling PSR%s\n",
>  		      dev_priv->psr.psr2_enabled ? "2" : "1");
> @@ -1206,9 +1214,6 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
>  	if (!HAS_PSR(dev_priv))
>  		return;
>  
> -	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
> -		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
> -
	if (IS_HASWELL(dev_priv))
		dev_priv->hsw_base_adj =
BDW_EDP_PSR_BASE - HSW_EDP_PSR_BASE;

	

>  	if (!dev_priv->psr.sink_support)
>  		return;
>  

_______________________________________________
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