Re: [PATCH 7/7] drm/i915: Make PSR registers relative to transcoders

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

 



On Fri, 2019-04-05 at 17:55 -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2019-04-03 at 16:35 -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
> > from BSpec to search the register name in i915 as also the BSpec
> > name
> > don't match with the name in i915.
> > 
> > The other option would be use the whole hard-coded address but this
> > is
> > not future proof, so here going in the middle ground by making
> > every
> > PSR register relative to transcoder(that is EDP transcoder), the
> > only
> > exception is PSR_IMR/IIR that is not relative to nothing.
> > For the _TRANS2() macros to work it needs the address of the
> > register
> > from the TRANSCODER_A, so adding it to every register together with
> > the register address from the EDP transcoder so it will make easy
> > for
> > people searching with BSpec address also adding those with the
> > BSpec
> > name.
> > 
> > For Haswell all the PSR register are relative to 0x64000, so
> > mmio_base_adjust was added and used to take care of that.
> > 
> > Also removing BDW_EDP_PSR_BASE from GVT because it is not used as
> > the only PSR register that GVT have is this one(SRD/PSR_CTL).
> > 
> > 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>
> > 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     | 59 ++++++++++++++++++++-----
> > ----
> >  drivers/gpu/drm/i915/intel_psr.c    | 11 ++++--
> >  4 files changed, 52 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
> > b/drivers/gpu/drm/i915/gvt/handlers.c
> > index 86761b1def1e..d09b798e93cb 100644
> > --- a/drivers/gpu/drm/i915/gvt/handlers.c
> > +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> > @@ -2739,7 +2739,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 8f38d03b1c4e..9ce46a7dabfd 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -501,6 +501,8 @@ struct i915_drrs {
> >  };
> >  
> >  struct i915_psr {
> > +	/* different than zero only on HSW see _TRANS2_PSR() for more
> > info */
> > +	u32 mmio_base_adjust;
> >  	struct mutex lock;
> >  
> >  #define I915_PSR_DEBUG_MODE_MASK	0x0f
> > @@ -515,6 +517,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;
> > @@ -1541,8 +1544,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 18e2991b376d..4df56c118cd2 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -250,9 +250,10 @@ static inline bool
> > i915_mmio_reg_valid(i915_reg_t reg)
> >  #define _MMIO_PIPE2(pipe, reg)		_MMIO(INTEL_INFO(dev_pr
> > iv)-
> > > pipe_offsets[pipe] - \
> >  					      INTEL_INFO(dev_priv)-
> > > pipe_offsets[PIPE_A] + (reg) + \
> >  					      DISPLAY_MMIO_BASE(dev_pri
> > v))
> > -#define _MMIO_TRANS2(pipe, reg)		_MMIO(INTEL_INFO(dev_pr
> > iv)-
> > > trans_offsets[(pipe)] - \
> > -					      INTEL_INFO(dev_priv)-
> > > trans_offsets[TRANSCODER_A] + (reg) + \
> > -					      DISPLAY_MMIO_BASE(dev_pri
> > v))
> > +#define _TRANS2(trans, reg)		(INTEL_INFO(dev_priv)-
> > > trans_offsets[(trans)] - \
> > +					 INTEL_INFO(dev_priv)-
> > > trans_offsets[TRANSCODER_A] + (reg) + \
> > +					 DISPLAY_MMIO_BASE(dev_priv))
> > +#define _MMIO_TRANS2(trans, reg)	_MMIO(_TRANS2(trans, reg))
> >  #define _CURSOR2(pipe, reg)		_MMIO(INTEL_INFO(dev_pr
> > iv)-
> > > cursor_offsets[(pipe)] - \
> >  					      INTEL_INFO(dev_priv)-
> > > cursor_offsets[PIPE_A] + (reg) + \
> >  					      DISPLAY_MMIO_BASE(dev_pri
> > v))
> > @@ -4210,9 +4211,15 @@ 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
> > +
> > +/* 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)
> 
> The error interrupt registers are still hardcoded for eDP, aren't
> they?

Yes, the PSR_IMR and PSR_IIR have the same hardcoded address in HSW and
other platforms.

> 
> > +#define _MMIO_TRANS2_PSR(reg)	_MMIO(_TRANS2_PSR(reg))
> > +
> > +#define _SRD_CTL_A				0x60800
> > +#define _SRD_CTL_EDP				0x6F800
> > +#define EDP_PSR_CTL				_MMIO_TRANS2_PS
> > R(_SRD_CTL_A)
> Why isn't this accepting transcoder/pipe as an argument? dev_priv-
> > psr.transcoder is hidden two levels below than it should be.
> 

Mostly to avoid more changes, as we only have one psr struct now.
But if in the future we have a platform that can have more than one eDP
panel we will need for sure add a transcoder parameter.

> 
> 
> >  #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 */
> > @@ -4245,16 +4252,22 @@ enum {
> >  #define   EDP_PSR_POST_EXIT			(1 << 1)
> >  #define   EDP_PSR_PRE_ENTRY			(1 << 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_PS
> > R(_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_P
> > SR(_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_PS
> > R(_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)
> > @@ -4279,10 +4292,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_PS
> > R(_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)
> > @@ -4290,7 +4308,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_PSR(_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+ */
> > @@ -4308,7 +4328,9 @@ enum {
> >  #define   EDP_PSR2_IDLE_FRAME_MASK	0xf
> >  #define   EDP_PSR2_IDLE_FRAME_SHIFT	0
> >  
> > -#define PSR_EVENT				_MMIO(0x6F848)
> > +#define _PSR_EVENT_A				0x60848
> > +#define _PSR_EVENT_EDP				0x6F848
> > +#define PSR_EVENT				_MMIO_TRANS2_PSR(_PSR_E
> > VENT_A)
> >  #define  PSR_EVENT_PSR2_WD_TIMER_EXPIRE		(1 << 17)
> >  #define  PSR_EVENT_PSR2_DISABLED		(1 << 16)
> >  #define  PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN	(1 << 15)
> > @@ -4326,14 +4348,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
> > +#define _PSR2_STATUS_EDP		0x6F940
> > +#define EDP_PSR2_STATUS			_MMIO_TRANS2_PSR(_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_PSR(_PSR2
> > _SU_STATU
> > S_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 b984e005b72e..ba88464cbbe8 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -668,6 +668,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;
> > +	}
> >  
> >  	DRM_DEBUG_KMS("Enabling PSR%s\n",
> >  		      dev_priv->psr.psr2_enabled ? "2" : "1");
> > @@ -1132,9 +1140,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 (!dev_priv->psr.sink_support)
> >  		return;
> >  

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
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