On Wed, 2021-06-23 at 22:06 +0300, Gwan-gyeong Mun wrote:
> On 6/16/21 11:31 PM, José Roberto de Souza wrote:
> > Implements changes around PSR for alderlake-P:
> > 
> > - EDP_SU_TRACK_ENABLE was removed and bit 30 now has other function
> > - Some bits of PSR2_MAN_TRK_CTL moved and SF_PARTIAL_FRAME_UPDATE was
> >    removed setting SU_REGION_START/END_ADDR will do this job
> > - SU_REGION_START/END_ADDR have now line granularity but will need to
> >    be aligned with DSC when the PSRS + DSC support lands
> > 
> > BSpec: 50422
> > BSpec: 50424
> > Cc: Gwan-gyeong Mun <[email protected]>
> > Cc: Anusha Srivatsa <[email protected]>
> > Signed-off-by: José Roberto de Souza <[email protected]>
> > Signed-off-by: Gwan-gyeong Mun <[email protected]>
> > ---
> >   drivers/gpu/drm/i915/display/intel_psr.c | 43 ++++++++++++++++++------
> >   drivers/gpu/drm/i915/i915_reg.h          | 26 ++++++++------
> >   2 files changed, 48 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c 
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 9643624fe160d..46bb19c4b63a4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -534,11 +534,13 @@ static u32 intel_psr2_get_tp_time(struct intel_dp 
> > *intel_dp)
> >   static void hsw_activate_psr2(struct intel_dp *intel_dp)
> >   {
> >     struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > -   u32 val;
> > +   u32 val = EDP_PSR2_ENABLE;
> > +
> > +   val |= psr_compute_idle_frames(intel_dp) << EDP_PSR2_IDLE_FRAME_SHIFT;
> >   
> > -   val = psr_compute_idle_frames(intel_dp) << EDP_PSR2_IDLE_FRAME_SHIFT;
> > +   if (!IS_ALDERLAKE_P(dev_priv))
> > +           val |= EDP_SU_TRACK_ENABLE;
> >   
> > -   val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
> >     if (DISPLAY_VER(dev_priv) >= 10 && DISPLAY_VER(dev_priv) <= 12)
> >             val |= EDP_Y_COORDINATE_ENABLE;
> >   
> > @@ -793,6 +795,7 @@ static bool intel_psr2_sel_fetch_config_valid(struct 
> > intel_dp *intel_dp,
> >   static bool psr2_granularity_check(struct intel_dp *intel_dp,
> >                                struct intel_crtc_state *crtc_state)
> >   {
> > +   struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >     const int crtc_hdisplay = crtc_state->hw.adjusted_mode.crtc_hdisplay;
> >     const int crtc_vdisplay = crtc_state->hw.adjusted_mode.crtc_vdisplay;
> >     u16 y_granularity = 0;
> > @@ -809,10 +812,13 @@ static bool psr2_granularity_check(struct intel_dp 
> > *intel_dp,
> >             return intel_dp->psr.su_y_granularity == 4;
> >   
> >     /*
> > -    * For SW tracking we can adjust the y to match sink requirement if
> > -    * multiple of 4
> > +    * adl_p has 1 line granularity for other platforms with SW tracking we
> > +    * can adjust the y coordinate to match sink requirement if multiple of
> > +    * 4
> >      */
> > -   if (intel_dp->psr.su_y_granularity <= 2)
> > +   if (IS_ALDERLAKE_P(dev_priv))
> > +           y_granularity = intel_dp->psr.su_y_granularity;
> > +   else if (intel_dp->psr.su_y_granularity <= 2)
> >             y_granularity = 4;
> >     else if ((intel_dp->psr.su_y_granularity % 4) == 0)
> >             y_granularity = intel_dp->psr.su_y_granularity;
> > @@ -1525,21 +1531,32 @@ void intel_psr2_program_trans_man_trk_ctl(const 
> > struct intel_crtc_state *crtc_st
> >   static void psr2_man_trk_ctl_calc(struct intel_crtc_state *crtc_state,
> >                               struct drm_rect *clip, bool full_update)
> >   {
> > +   struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >     u32 val = PSR2_MAN_TRK_CTL_ENABLE;
> The logic is not wrong, but the meaning of the register bit has changed.
> The 31st bit in ADL-P means "SF partial frame enable".
> It is recommended to add a macro such as 
> ADLP_PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_ENABLE to the code to clarify the 
> role of the changed register.

In my opinion the meaning is the same, enable manual/software tracking.
It was just a register rename done as part of changes of the other bits. 

But if you really think is necessary I can do that, please let me know.

> >   
> >     if (full_update) {
> > -           val |= PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME;
> > +           if (IS_ALDERLAKE_P(dev_priv))
> > +                   val |= ADLP_PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME;
> > +           else
> > +                   val |= PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME;
> > +
> >             goto exit;
> >     }
> >   
> >     if (clip->y1 == -1)
> >             goto exit;
> >   
> > -   drm_WARN_ON(crtc_state->uapi.crtc->dev, clip->y1 % 4 || clip->y2 % 4);
> > +   if (IS_ALDERLAKE_P(dev_priv)) {
> > +           val |= ADLP_PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(clip->y1);
> > +           val |= ADLP_PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2);
> > +   } else {
> > +           drm_WARN_ON(crtc_state->uapi.crtc->dev, clip->y1 % 4 || 
> > clip->y2 % 4);
> >   
> > -   val |= PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE;
> > -   val |= PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(clip->y1 / 4 + 1);
> > -   val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2 / 4 + 1);
> > +           val |= PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE;
> > +           val |= PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(clip->y1 / 4 + 1);
> > +           val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2 / 4 + 1);
> > +   }
> >   exit:
> >     crtc_state->psr2_man_track_ctl = val;
> >   }
> > @@ -1563,11 +1580,15 @@ static void clip_area_update(struct drm_rect 
> > *overlap_damage_area,
> >   static void intel_psr2_sel_fetch_pipe_alignment(const struct 
> > intel_crtc_state *crtc_state,
> >                                             struct drm_rect *pipe_clip)
> >   {
> > +   struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> >     const u16 y_alignment = crtc_state->su_y_granularity;
> >   
> >     pipe_clip->y1 -= pipe_clip->y1 % y_alignment;
> >     if (pipe_clip->y2 % y_alignment)
> >             pipe_clip->y2 = ((pipe_clip->y2 / y_alignment) + 1) * 
> > y_alignment;
> > +
> > +   if (IS_ALDERLAKE_P(dev_priv) && crtc_state->dsc.compression_enable)
> > +           drm_warn(&dev_priv->drm, "Missing PSR2 sel fetch alignment with 
> > DSC\n");
> >   }
> >   
> >   int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index e0bd60fe7a190..74dc5ebce60e7 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4586,7 +4586,7 @@ enum {
> >   #define _PSR2_CTL_EDP                             0x6f900
> >   #define EDP_PSR2_CTL(tran)                        _MMIO_TRANS2(tran, 
> > _PSR2_CTL_A)
> >   #define   EDP_PSR2_ENABLE                 (1 << 31)
> > -#define   EDP_SU_TRACK_ENABLE                      (1 << 30)
> > +#define   EDP_SU_TRACK_ENABLE                      (1 << 30) /* up to 
> > adl-p */
> >   #define   TGL_EDP_PSR2_BLOCK_COUNT_NUM_2  (0 << 28)
> >   #define   TGL_EDP_PSR2_BLOCK_COUNT_NUM_3  (1 << 28)
> >   #define   EDP_Y_COORDINATE_ENABLE         REG_BIT(25) /* display 10, 11 
> > and 12 */
> > @@ -4655,17 +4655,23 @@ 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_SU_REGION_START_ADDR_MASK        REG_GENMASK(30, 
> > 21)
> > -#define  PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(val)        
> > REG_FIELD_PREP(PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR_MASK, val)
> > +#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_SU_REGION_START_ADDR_MASK                
> > REG_GENMASK(30, 21)
> > +#define  PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(val)                
> > REG_FIELD_PREP(PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR_MASK, val)
> >   #define  PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR_MASK         REG_GENMASK(20, 
> > 11)
> >   #define  PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(val)         
> > REG_FIELD_PREP(PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR_MASK, val)
> > -#define  PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME             REG_BIT(3)
> > -#define  PSR2_MAN_TRK_CTL_SF_CONTINUOS_FULL_FRAME  REG_BIT(2)
> > -#define  PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE  REG_BIT(1)
> > +#define  PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME                     
> > REG_BIT(3)
> > +#define  PSR2_MAN_TRK_CTL_SF_CONTINUOS_FULL_FRAME          REG_BIT(2)
> > +#define  PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE          REG_BIT(1)
> > +#define  ADLP_PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR_MASK   REG_GENMASK(28, 
> > 16)
> > +#define  ADLP_PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(val)   
> > REG_FIELD_PREP(ADLP_PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR_MASK, val)
> > +#define  ADLP_PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR_MASK             
> > REG_GENMASK(12, 0)
> > +#define  ADLP_PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(val)             
> > REG_FIELD_PREP(ADLP_PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR_MASK, val)
> > +#define  ADLP_PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME                
> > REG_BIT(14)
> > +#define  ADLP_PSR2_MAN_TRK_CTL_SF_CONTINUOS_FULL_FRAME             
> > REG_BIT(13)
> >   
> >   /* Icelake DSC Rate Control Range Parameter Registers */
> >   #define DSCA_RC_RANGE_PARAMETERS_0                _MMIO(0x6B240)
> > 

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to