[Intel-gfx] [PATCH 3/8] drm/i915/psr: Make all PSR register relative to mmio base

[Souza, Jose]

On Fri, 2019-03-22 at 10:27 -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2019-03-22 at 11:15 +0200, Jani Nikula wrote:
> > On Thu, 21 Mar 2019, José Roberto de Souza <jose.souza at intel.com>
> > wrote:
> > > Right now it have a mix of PSR registers that are relative to PSR
> > > mmio base and other register with a hardcoded address, lets keep
> > > it
> > > consistented and have it all relative to mmio base.
> > 
> > This is not strictly limited to this patch, but an overall trend.
> > The
> > thing that really bugs me with this is losing more of the actual
> > absolute mmio addresses from the file. When you're seeking to add a
> > new
> > register, you can't trivially grep for it in the file anymore. Not
> > all
> > of our register names match the spec (and the spec occasionally
> > also
> > changes register names) so being able to find the offset is
> > important.

I understand but for new gens BSpec is using relative address see BSpec
50583 and 50577 for example.

> 
> Fully agreed.
> 
> I think we can do something along the lines of  
> 
> #define _HSW_PSR_OFFSET BDW_EDP_PSR_BASE - HSW_PSR_PSR_BASE
> #define _BDW_PSR_CTL 0x6f800
> 
> _MMIO_HSW_ADJUST(pipe, reg)  IS_HASWELL(dev_priv) ?
> 				MMIO_TRANS2((pipe), reg -
> _HSW_PSR_OFFSET) : 					MMIO_TRANS2((pi
> pe), reg)

To MMIO_TRANS2() work we need to give the reg based on the first
transcoder, what you think about this?


#define _HSW_EDP_PSR_BASE	0x64000

/* _PSR_CTL_A to follow BSpec naming or we could keep _PSR_CTL_A */
#define _SRD_CTL_A		0x60800
#define _SRD_CTL_EDP		0x6F800
#define EDP_PSR_CTL				(_MMIO_TRANS2(dev_priv-
>psr.transcoder, _SRD_CTL_A) - dev_priv->psr.mmio_base_adjust)

intel_psr_enable_locked()
	if (IS_HASWELL(dev_priv))
		dev_priv->psr.mmio_base_adjust = _SRD_CTL_EDP
- _HSW_EDP_PSR_BASE;

The only concern here it that _SRD_CTL_A(and the other registers
conterparts) could give the understand that PSR could be enabled in
regular transcoders what it not the case in current gens.


> 
> #define EDP_PSR_CTL(pipe) _MMIO_HSW_ADJUST((pipe), _BDW_PSR_CTL)
> 
> 
> I'd like at least BDW+ addresses to be in the code.
> 
> -DK
> 
> > When we added the macros that use ->pipe_offsets and
> > ->trans_offsets, we
> > took care to have at least one of the offsets in the file. I'm
> > wondering
> > if we could do something like that here as well.
> > 
> > BR,
> > Jani.
> > 
> > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h | 11 ++++-------
> > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 28728399e607..e1ed2ba1c315 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4326,7 +4326,7 @@ 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 EDP_PSR2_CTL			_MMIO(dev_priv-
> > > >psr.mmio_base +
> > > 0x100)
> > >  #define   EDP_PSR2_ENABLE		(1 << 31)
> > >  #define   EDP_SU_TRACK_ENABLE		(1 << 30)
> > >  #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
> > > @@ -4344,7 +4344,7 @@ enum {
> > >  #define   EDP_PSR2_IDLE_FRAME_MASK	0xf
> > >  #define   EDP_PSR2_IDLE_FRAME_SHIFT	0
> > >  
> > > -#define PSR_EVENT				_MMIO(0x6F848)
> > > +#define PSR_EVENT				_MMIO(dev_priv-
> > > >psr.mmio_base +
> > > 0x48)
> > >  #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)
> > > @@ -4362,14 +4362,11 @@ enum {
> > >  #define  PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
> > >  #define  PSR_EVENT_PSR_DISABLE			(1 << 0)
> > >  
> > > -#define EDP_PSR2_STATUS			_MMIO(0x6f940)
> > > +#define EDP_PSR2_STATUS			_MMIO(dev_priv-
> > > >psr.mmio_base +
> > > 0x140)
> > >  #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(index)		_MMIO(dev_priv-
> > > >psr.mmio_base +
> > > 0x114 + (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))
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20190326/f76bce43/attachment.sig>