[Intel-gfx] [PATCH 4/6] drm/i915: Add PSR2 software tracking registers
Souza, Jose
jose.souza at intel.com
Mon Jun 15 19:23:37 UTC 2020
On Mon, 2020-06-15 at 19:37 +0100, Mun, Gwan-gyeong wrote:
> On Fri, 2020-06-12 at 21:49 +0000, Mun, Gwan-gyeong wrote:
> > On Fri, 2020-06-12 at 14:18 -0700, Souza, Jose wrote:
> > > On Fri, 2020-06-12 at 21:57 +0100, Mun, Gwan-gyeong wrote:
> > > > On Tue, 2020-05-26 at 15:14 -0700, José Roberto de Souza wrote:
> > > > > This registers will be used to implement PSR2 software
> > > > > tracking.
> > > > >
> > > > > BSpec: 55229
> > > > > BSpec: 50424
> > > > > BSpec: 50420
> > > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/i915_reg.h | 68
> > > > > ++++++++++++++++++++++++++++++-
> > > > > --
> > > > > 1 file changed, 63 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index e9d50fe0f375..6f547e459d30 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -4566,6 +4566,18 @@ enum {
> > > > > #define PSR2_SU_STATUS_MASK(frame) (0x3ff <<
> > > > > PSR2_SU_STATUS_SHIFT(frame))
> > > > > #define PSR2_SU_STATUS_FRAMES 8
> > > > >
> > > > > +#define _PSR2_MAN_TRK_CTL_A 0x60910
> > > > > +#define _PSR2_MAN_TRK_CTL_EDP 0x6f910
> > > > > +#define PSR2_MAN_TRK_CTL(tran) _MMIO_T
> > > > > RANS2(tran, _PSR2_MAN_TRK_CTL_A)
> > > > > +#define PSR2_MAN_TRK_CTL_ENABLE REG_BIT
> > > > > (31)
> > > > > +#define PSR2_MAN_TRK_CTL_REGION_START_ADDR_MASK REG_GEN
> > > > > MASK(30,
> > > > > 21)
> > > > > +#define PSR2_MAN_TRK_CTL_REGION_START_ADDR(val) REG_FIE
> > > > > LD_PREP(
> > > > > PSR2_MAN_TRK_CTL_REGION_START_ADDR_MASK, val)
> > > > > +#define PSR2_MAN_TRK_CTL_REGION_END_ADDR_MASK REG_GEN
> > > > > MASK(20, 11)
> > > > > +#define PSR2_MAN_TRK_CTL_REGION_END_ADDR(val) REG_FIE
> > > > > LD_PREP(PSR2_MAN_TRK_CTL_REGION_END_ADDR_MASK, val)
> > > > > +#define PSR2_MAN_TRK_CTL_SINGLE_FULL_FRAME REG_BIT
> > > > > (3)
> > > > > +#define PSR2_MAN_TRK_CTL_CONTINUOS_FULL_FRAME REG_BIT
> > > > > (2)
> > > > > +#define PSR2_MAN_TRK_CTL_PARTIAL_FRAME_UPDATE REG_BIT
> > > > > (1)
> > > > > +
> > > > As per Bspec, it would be better that the names of bit as below.
> > > >
> > > > PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME
> > > > PSR2_MAN_TRK_CTL_SF_CONTINUOS_FULL_FRAME
> > > > PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_ENABLE
> > >
> > > No problem in naming like this but MAN_TRK and SF is kind of
> > > redundant and the name was already big.
> > > Your call.
> > >
> > > > > /* VGA port control */
> > > > > #define ADPA _MMIO(0x61100)
> > > > > #define PCH_ADPA _MMIO(0xe1100)
> > > > > @@ -7129,7 +7141,52 @@ enum {
> > > > > #define PLANE_COLOR_CTL(pipe, plane) \
> > > > > _MMIO_PLANE(plane, _PLANE_COLOR_CTL_1(pipe),
> > > > > _PLANE_COLOR_CTL_2(pipe))
> > > > >
> > > > > -#/* SKL new cursor registers */
> > > > > +#define _PLANE_SEL_FETCH_BASE_1_A 0x70890
> > > > > +#define _PLANE_SEL_FETCH_BASE_2_A 0x708B0
> > > > > +#define _PLANE_SEL_FETCH_BASE_3_A 0x708D0
> > > > > +#define _PLANE_SEL_FETCH_BASE_4_A 0x708F0
> > > > > +#define _PLANE_SEL_FETCH_BASE_5_A 0x70920
> > > > > +#define _PLANE_SEL_FETCH_BASE_6_A 0x70940
> > > > > +#define _PLANE_SEL_FETCH_BASE_7_A 0x70960
> > > > > +#define _PLANE_SEL_FETCH_BASE_CUR_A 0x70880
> > > > > +#define _PLANE_SEL_FETCH_BASE_1_B 0x70990
> > > > > +
> > > > And as per Bspec, the prefix _SEL_FETCH_PLANE_ is better than
> > > > _PLANE_SEL_FETCH_ .
> > > You mean just for the "internal" ones? For PLANE_SEL_FETCH_CTL,
> > > PLANE_SEL_FETCH_SIZE... would be better keep like this to match
> > > other
> > > plane register
> > > names.
> > Internals and externals. I also noticed your intention (match other
> > plane related registers), but when I checked other plane related
> > resiters, they followed bspec names. (But I am not confident on
> > register naming policy; we always have to follow documented register
> > names or not. )
> > > > > +#define _PLANE_SEL_FETCH_BASE_A(plane) _PICK(plane, \
> > > > > + _PLANE_SEL_FETCH_B
> > > > > ASE_1_A,
> > > > > \
> > > > > + _PLANE_SEL_FETCH_B
> > > > > ASE_2_A,
> > > > > \
> > > > > + _PLANE_SEL_FETCH_B
> > > > > ASE_3_A,
> > > > > \
> > > > > + _PLANE_SEL_FETCH_B
> > > > > ASE_4_A,
> > > > > \
> > > > > + _PLANE_SEL_FETCH_B
> > > > > ASE_5_A,
> > > > > \
> > > > > + _PLANE_SEL_FETCH_B
> > > > > ASE_6_A,
> > > > > \
> > > > > + _PLANE_SEL_FETCH_B
> > > > > ASE_7_A,
> > > > > \
> > > > > + _PLANE_SEL_FETCH_B
> > > > > ASE_CUR_
> > > > > A)
> > > > > +#define _PLANE_SEL_FETCH_BASE_1(pipe) _PIPE(pipe,
> > > > > _PLANE_SEL_FETCH_BASE_1_A, _PLANE_SEL_FETCH_BASE_1_A)
>
> It seems that indicates an wrong register name.
> IMHO, is it your intention like this? " #define
> _PLANE_SEL_FETCH_BASE_1(pipe) _PIPE(pipe, _PLANE_SEL_FETCH_BASE_1_A,
> _PLANE_SEL_FETCH_BASE_1_B) "?
Yes, it should be _PLANE_SEL_FETCH_BASE_1_B, thanks for catching this.
Will send this 4 patches in a few days with the requested fixes.
>
> > > > > +#define PLANE_SEL_FETCH_BASE(pipe, plane)
> > > > > (_PLANE_SEL_FETCH_BASE_1(pipe) - \
> > > > > + _PLANE_SEL_FETCH_BAS
> > > > > E_1_A +
> > > > > \
> > > > > + _PLANE_SEL_FETCH_BAS
> > > > > E_A(plan
> > > > > e))
> > > > > +
> > > > > +#define _PLANE_SEL_FETCH_CTL_1_A 0x70890
> > > > > +#define PLANE_SEL_FETCH_CTL(pipe, plane)
> > > > > _MMIO(PLANE_SEL_FETCH_BASE(pipe, plane) + \
> > > > > + _PLANE_SEL_FETCH
> > > > > _CTL_1_A
> > > > > - \
> > > > > + _PLANE_SEL_FETCH
> > > > > _BASE_1_
> > > > > A)
> > > > > +#define PLANE_SET_FETCH_CTL_ENABLE REG_BIT(31)
> > > > > +
> > > > > +#define _PLANE_SEL_FETCH_POS_1_A 0x70894
> > > > > +#define PLANE_SEL_FETCH_POS(pipe, plane)
> > > > > _MMIO(PLANE_SEL_FETCH_BASE(pipe, plane) + \
> > > > > + _PLANE_SEL_FETCH
> > > > > _POS_1_A
> > > > > - \
> > > > > + _PLANE_SEL_FETCH
> > > > > _BASE_1_
> > > > > A)
> > > > > +
> > > > > +#define _PLANE_SEL_FETCH_SIZE_1_A 0x70898
> > > > > +#define PLANE_SEL_FETCH_SIZE(pipe, plane)
> > > > > _MMIO(PLANE_SEL_FETCH_BASE(pipe, plane) + \
> > > > > + _PLANE_SEL_FETC
> > > > > H_SIZE_1
> > > > > _A - \
> > > > > + _PLANE_SEL_FETC
> > > > > H_BASE_1
> > > > > _A)
> > > > > +
> > > > > +#define _PLANE_SEL_FETCH_OFFSET_1_A 0x7089C
> > > > > +#define PLANE_SEL_FETCH_OFFSET(pipe, plane)
> > > > > _MMIO(PLANE_SEL_FETCH_BASE(pipe, plane) + \
> > > > > + _PLANE_SEL_FE
> > > > > TCH_OFFS
> > > > > ET_1_A - \
> > > > > + _PLANE_SEL_FE
> > > > > TCH_BASE
> > > > > _1_A)
> > > > > +
> > > > > +/* SKL new cursor registers */
> > > > > #define _CUR_BUF_CFG_A 0x7017c
> > > > > #define _CUR_BUF_CFG_B 0x7117c
> > > > > #define CUR_BUF_CFG(pipe) _MMIO_PIPE(pipe,
> > > > > _CUR_BUF_CFG_A,
> > > > > _CUR_BUF_CFG_B)
> > > > > @@ -7775,11 +7832,12 @@ enum {
> > > > > # define CHICKEN3_DGMG_REQ_OUT_FIX_DISABLE (1 << 5)
> > > > > # define CHICKEN3_DGMG_DONE_FIX_DISABLE (1 <<
> > > > > 2)
> > > > >
> > > > > -#define CHICKEN_PAR1_1 _MMIO(0x42080)
> > > > > +#define CHICKEN_PAR1_1 _MMIO(0x42080)
> > > > > #define SKL_DE_COMPRESSED_HASH_MODE (1 << 15)
> > > > > -#define DPA_MASK_VBLANK_SRD (1 << 15)
> > > > > -#define FORCE_ARB_IDLE_PLANES (1 << 14)
> > > > > -#define SKL_EDP_PSR_FIX_RDWRAP (1 << 3)
> > > > > +#define DPA_MASK_VBLANK_SRD (1 << 15)
> > > > > +#define FORCE_ARB_IDLE_PLANES (1 << 14)
> > > > > +#define SKL_EDP_PSR_FIX_RDWRAP (1 << 3)
> > > > > +#define IGNORE_PSR2_HW_TRACKING (1 << 1)
> > > > >
> > > > > #define CHICKEN_PAR2_1 _MMIO(0x42090)
> > > > > #define KVM_CONFIG_CHANGE_NOTIFICATION_SELECT (1 << 14)
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list