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 > > 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 > > > Cc: Rodrigo Vivi > > > Signed-off-by: José Roberto de Souza > > > --- > > > 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))