diff mbox series

[4/6] drm/i915: Add PSR2 software tracking registers

Message ID 20200526221447.64110-4-jose.souza@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/6] drm/i915/rkl: Disable PSR2 | expand

Commit Message

Souza, Jose May 26, 2020, 10:14 p.m. UTC
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@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 68 ++++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 5 deletions(-)

Comments

Gwan-gyeong Mun June 12, 2020, 8:57 p.m. UTC | #1
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@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_GENMASK(30,
> 21)
> +#define  PSR2_MAN_TRK_CTL_REGION_START_ADDR(val)	REG_FIELD_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

>  /* 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_ .

> +#define _PLANE_SEL_FETCH_BASE_A(plane) _PICK(plane, \
> +					     _PLANE_SEL_FETCH_BASE_1_A,
> \
> +					     _PLANE_SEL_FETCH_BASE_2_A,
> \
> +					     _PLANE_SEL_FETCH_BASE_3_A,
> \
> +					     _PLANE_SEL_FETCH_BASE_4_A,
> \
> +					     _PLANE_SEL_FETCH_BASE_5_A,
> \
> +					     _PLANE_SEL_FETCH_BASE_6_A,
> \
> +					     _PLANE_SEL_FETCH_BASE_7_A,
> \
> +					     _PLANE_SEL_FETCH_BASE_CUR_
> A)
> +#define _PLANE_SEL_FETCH_BASE_1(pipe) _PIPE(pipe,
> _PLANE_SEL_FETCH_BASE_1_A, _PLANE_SEL_FETCH_BASE_1_A)
> +#define PLANE_SEL_FETCH_BASE(pipe, plane)
> (_PLANE_SEL_FETCH_BASE_1(pipe) - \
> +					   _PLANE_SEL_FETCH_BASE_1_A +
> \
> +					   _PLANE_SEL_FETCH_BASE_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_FETCH_SIZE_1
> _A - \
> +						_PLANE_SEL_FETCH_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_FETCH_OFFS
> ET_1_A - \
> +						  _PLANE_SEL_FETCH_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)
Souza, Jose June 12, 2020, 9:18 p.m. UTC | #2
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@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_GENMASK(30,
> > 21)
> > +#define  PSR2_MAN_TRK_CTL_REGION_START_ADDR(val)	REG_FIELD_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.

> 
> > +#define _PLANE_SEL_FETCH_BASE_A(plane) _PICK(plane, \
> > +					     _PLANE_SEL_FETCH_BASE_1_A,
> > \
> > +					     _PLANE_SEL_FETCH_BASE_2_A,
> > \
> > +					     _PLANE_SEL_FETCH_BASE_3_A,
> > \
> > +					     _PLANE_SEL_FETCH_BASE_4_A,
> > \
> > +					     _PLANE_SEL_FETCH_BASE_5_A,
> > \
> > +					     _PLANE_SEL_FETCH_BASE_6_A,
> > \
> > +					     _PLANE_SEL_FETCH_BASE_7_A,
> > \
> > +					     _PLANE_SEL_FETCH_BASE_CUR_
> > A)
> > +#define _PLANE_SEL_FETCH_BASE_1(pipe) _PIPE(pipe,
> > _PLANE_SEL_FETCH_BASE_1_A, _PLANE_SEL_FETCH_BASE_1_A)
> > +#define PLANE_SEL_FETCH_BASE(pipe, plane)
> > (_PLANE_SEL_FETCH_BASE_1(pipe) - \
> > +					   _PLANE_SEL_FETCH_BASE_1_A +
> > \
> > +					   _PLANE_SEL_FETCH_BASE_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_FETCH_SIZE_1
> > _A - \
> > +						_PLANE_SEL_FETCH_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_FETCH_OFFS
> > ET_1_A - \
> > +						  _PLANE_SEL_FETCH_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)
Gwan-gyeong Mun June 12, 2020, 9:49 p.m. UTC | #3
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@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_GENMASK(30,
> > > 21)
> > > +#define  PSR2_MAN_TRK_CTL_REGION_START_ADDR(val)	REG_FIELD_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_BASE_1_A,
> > > \
> > > +					     _PLANE_SEL_FETCH_BASE_2_A,
> > > \
> > > +					     _PLANE_SEL_FETCH_BASE_3_A,
> > > \
> > > +					     _PLANE_SEL_FETCH_BASE_4_A,
> > > \
> > > +					     _PLANE_SEL_FETCH_BASE_5_A,
> > > \
> > > +					     _PLANE_SEL_FETCH_BASE_6_A,
> > > \
> > > +					     _PLANE_SEL_FETCH_BASE_7_A,
> > > \
> > > +					     _PLANE_SEL_FETCH_BASE_CUR_
> > > A)
> > > +#define _PLANE_SEL_FETCH_BASE_1(pipe) _PIPE(pipe,
> > > _PLANE_SEL_FETCH_BASE_1_A, _PLANE_SEL_FETCH_BASE_1_A)
> > > +#define PLANE_SEL_FETCH_BASE(pipe, plane)
> > > (_PLANE_SEL_FETCH_BASE_1(pipe) - \
> > > +					   _PLANE_SEL_FETCH_BASE_1_A +
> > > \
> > > +					   _PLANE_SEL_FETCH_BASE_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_FETCH_SIZE_1
> > > _A - \
> > > +						_PLANE_SEL_FETCH_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_FETCH_OFFS
> > > ET_1_A - \
> > > +						  _PLANE_SEL_FETCH_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)
Gwan-gyeong Mun June 15, 2020, 6:37 p.m. UTC | #4
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@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) "?

> > > > +#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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Souza, Jose June 15, 2020, 7:23 p.m. UTC | #5
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@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Souza, Jose June 25, 2020, 9:41 p.m. UTC | #6
On Mon, 2020-06-15 at 19:23 +0000, Souza, Jose wrote:
> 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@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. )

I don't think we are that restrict as there is several registers that don't match.
Will update the internal ones, anyone searching by BSpec name will find the internal ones and reach the exported ones.


> > > > > > +#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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

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_TRANS2(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_GENMASK(30, 21)
+#define  PSR2_MAN_TRK_CTL_REGION_START_ADDR(val)	REG_FIELD_PREP(PSR2_MAN_TRK_CTL_REGION_START_ADDR_MASK, val)
+#define  PSR2_MAN_TRK_CTL_REGION_END_ADDR_MASK		REG_GENMASK(20, 11)
+#define  PSR2_MAN_TRK_CTL_REGION_END_ADDR(val)		REG_FIELD_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)
+
 /* 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
+
+#define _PLANE_SEL_FETCH_BASE_A(plane) _PICK(plane, \
+					     _PLANE_SEL_FETCH_BASE_1_A, \
+					     _PLANE_SEL_FETCH_BASE_2_A, \
+					     _PLANE_SEL_FETCH_BASE_3_A, \
+					     _PLANE_SEL_FETCH_BASE_4_A, \
+					     _PLANE_SEL_FETCH_BASE_5_A, \
+					     _PLANE_SEL_FETCH_BASE_6_A, \
+					     _PLANE_SEL_FETCH_BASE_7_A, \
+					     _PLANE_SEL_FETCH_BASE_CUR_A)
+#define _PLANE_SEL_FETCH_BASE_1(pipe) _PIPE(pipe, _PLANE_SEL_FETCH_BASE_1_A, _PLANE_SEL_FETCH_BASE_1_A)
+#define PLANE_SEL_FETCH_BASE(pipe, plane) (_PLANE_SEL_FETCH_BASE_1(pipe) - \
+					   _PLANE_SEL_FETCH_BASE_1_A + \
+					   _PLANE_SEL_FETCH_BASE_A(plane))
+
+#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_FETCH_SIZE_1_A - \
+						_PLANE_SEL_FETCH_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_FETCH_OFFSET_1_A - \
+						  _PLANE_SEL_FETCH_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)