Re: [Intel-gfx] [PATCH 4/6] drm/i915: Add PSR2 software tracking registers
On Mon, 2020-06-15 at 19:23 +, Souza, Jose wrote:
> On Mon, 2020-06-15 at 19:37 +0100, Mun, Gwan-gyeong wrote:
> > On Fri, 2020-06-12 at 21:49 +, 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
> > > > > > ---
> > > > > > 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_A0x60910
> > > > > > +#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_FRAMEREG_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_A0x70880
> > > > > > +#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,
> > > > > > \
> > > > > > +
Re: [Intel-gfx] [PATCH 4/6] drm/i915: Add PSR2 software tracking registers
On Mon, 2020-06-15 at 19:37 +0100, Mun, Gwan-gyeong wrote:
> On Fri, 2020-06-12 at 21:49 +, 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
> > > > > ---
> > > > > 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_FRAMES8
> > > > >
> > > > > +#define _PSR2_MAN_TRK_CTL_A 0x60910
> > > > > +#define _PSR2_MAN_TRK_CTL_EDP0x6f910
> > > > > +#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_A0x70890
> > > > > +#define _PLANE_SEL_FETCH_BASE_2_A0x708B0
> > > > > +#define _PLANE_SEL_FETCH_BASE_3_A0x708D0
> > > > > +#define _PLANE_SEL_FETCH_BASE_4_A0x708F0
> > > > > +#define _PLANE_SEL_FETCH_BASE_5_A0x70920
> > > > > +#define _PLANE_SEL_FETCH_BASE_6_A0x70940
> > > > > +#define _PLANE_SEL_FETCH_BASE_7_A0x70960
> > > > > +#define _PLANE_SEL_FETCH_BASE_CUR_A 0x70880
> > > > > +#define _PLANE_SEL_FETCH_BASE_1_B0x70990
> > > > > +
> > > > 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,
> > > > > \
> > > > > +
Re: [Intel-gfx] [PATCH 4/6] drm/i915: Add PSR2 software tracking registers
On Fri, 2020-06-12 at 21:49 +, 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
> > > > ---
> > > > 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_A0x60910
> > > > +#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_FRAMEREG_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_A0x70880
> > > > +#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
Re: [Intel-gfx] [PATCH 4/6] drm/i915: Add PSR2 software tracking registers
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
> > > ---
> > > 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_FRAMES8
> > >
> > > +#define _PSR2_MAN_TRK_CTL_A 0x60910
> > > +#define _PSR2_MAN_TRK_CTL_EDP0x6f910
> > > +#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_A0x70890
> > > +#define _PLANE_SEL_FETCH_BASE_2_A0x708B0
> > > +#define _PLANE_SEL_FETCH_BASE_3_A0x708D0
> > > +#define _PLANE_SEL_FETCH_BASE_4_A0x708F0
> > > +#define _PLANE_SEL_FETCH_BASE_5_A0x70920
> > > +#define _PLANE_SEL_FETCH_BASE_6_A0x70940
> > > +#define _PLANE_SEL_FETCH_BASE_7_A0x70960
> > > +#define _PLANE_SEL_FETCH_BASE_CUR_A 0x70880
> > > +#define _PLANE_SEL_FETCH_BASE_1_B0x70990
> > > +
> > 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) + \
> > > +
Re: [Intel-gfx] [PATCH 4/6] drm/i915: Add PSR2 software tracking registers
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
> > ---
> > 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_A0x60910
> > +#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_FRAMEREG_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_A0x70880
> > +#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
Re: [Intel-gfx] [PATCH 4/6] drm/i915: Add PSR2 software tracking registers
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
> ---
> 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_FRAMES8
>
> +#define _PSR2_MAN_TRK_CTL_A 0x60910
> +#define _PSR2_MAN_TRK_CTL_EDP0x6f910
> +#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_A0x70890
> +#define _PLANE_SEL_FETCH_BASE_2_A0x708B0
> +#define _PLANE_SEL_FETCH_BASE_3_A0x708D0
> +#define _PLANE_SEL_FETCH_BASE_4_A0x708F0
> +#define _PLANE_SEL_FETCH_BASE_5_A0x70920
> +#define _PLANE_SEL_FETCH_BASE_6_A0x70940
> +#define _PLANE_SEL_FETCH_BASE_7_A0x70960
> +#define _PLANE_SEL_FETCH_BASE_CUR_A 0x70880
> +#define _PLANE_SEL_FETCH_BASE_1_B0x70990
> +
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_A0x70898
> +#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 -
[Intel-gfx] [PATCH 4/6] drm/i915: Add PSR2 software tracking registers
This registers will be used to implement PSR2 software tracking.
BSpec: 55229
BSpec: 50424
BSpec: 50420
Signed-off-by: José Roberto de Souza
---
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_A0x60910
+#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_FRAMEREG_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_A0x70880
+#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_A0x7089C
+#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
