On Fri, 2019-03-22 at 11:15 +0200, Jani Nikula wrote:
> On Thu, 21 Mar 2019, José Roberto de Souza <jose.souza@xxxxxxxxx> 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.
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((pipe), reg)
#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@xxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > ---
> > 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))
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
