[Intel-gfx] [PATCH 3/4] drm/i915/display: Program PSR2 selective fetch registers
Mun, Gwan-gyeong
gwan-gyeong.mun at intel.com
Tue Sep 15 19:28:57 UTC 2020
On Mon, 2020-09-14 at 13:15 -0700, Souza, Jose wrote:
> On Mon, 2020-09-14 at 17:28 +0300, Ville Syrjälä wrote:
> > On Mon, Aug 31, 2020 at 06:09:23PM -0700, José Roberto de Souza
> > wrote:
> > > Another step towards PSR2 selective fetch, here programming plane
> > > selective fetch registers and MAN_TRK_CTL enabling selective
> > > fetch but
> > > for now it is fetching the whole area of the planes.
> > > The damaged area calculation will come as next and final step.
> > >
> > > BSpec: 55229
> > > Cc: Gwan-gyeong Mun <
> > > gwan-gyeong.mun at intel.com
> > > Cc: Ville Syrjälä <
> > > ville.syrjala at linux.intel.com
> > > Signed-off-by: José Roberto de Souza <
> > > jose.souza at intel.com
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 10 +-
> > > .../drm/i915/display/intel_display_types.h | 2 +
> > > drivers/gpu/drm/i915/display/intel_psr.c | 129
> > > +++++++++++++++++-
> > > drivers/gpu/drm/i915/display/intel_psr.h | 10 +-
> > > drivers/gpu/drm/i915/display/intel_sprite.c | 3 +
> > > 5 files changed, 145 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index c8b1dd1a9e46..865486e89915 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -11799,6 +11799,9 @@ static void i9xx_update_cursor(struct
> > > intel_plane *plane,
> > > if (INTEL_GEN(dev_priv) >= 9)
> > > skl_write_cursor_wm(plane, crtc_state);
> > >
> > > + if (!needs_modeset(crtc_state))
> > > + intel_psr2_program_plane_sel_fetch(plane, crtc_state,
> > > plane_state, 0);
> > > +
> > > if (plane->cursor.base != base ||
> > > plane->cursor.size != fbc_ctl ||
> > > plane->cursor.cntl != cntl) {
> > > @@ -12810,8 +12813,11 @@ static int
> > > intel_crtc_atomic_check(struct intel_atomic_state *state,
> > >
> > > }
> > >
> > > - if (!mode_changed)
> > > - intel_psr2_sel_fetch_update(state, crtc);
> > > + if (!mode_changed) {
> > > + ret = intel_psr2_sel_fetch_update(state, crtc);
> > > + if (ret)
> > > + return ret;
> > > + }
> > >
> > > return 0;
> > > }
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 9349b15afff6..2138bb0f1587 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -586,6 +586,8 @@ struct intel_plane_state {
> > > u32 planar_slave;
> > >
> > > struct drm_intel_sprite_colorkey ckey;
> > > +
> > > + struct drm_rect psr2_sel_fetch_area;
> > > };
> > >
> > > struct intel_initial_plane_config {
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 6698d0209879..b60ea133a527 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -1173,6 +1173,46 @@ static void
> > > psr_force_hw_tracking_exit(struct drm_i915_private *dev_priv)
> > > intel_psr_exit(dev_priv);
> > > }
> > >
> > > +void intel_psr2_program_plane_sel_fetch(struct intel_plane
> > > *plane,
> > > + const struct intel_crtc_state
> > > *crtc_state,
> > > + const struct intel_plane_state
> > > *plane_state,
> > > + int color_plane)
> > > +{
> > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > + const struct drm_rect *clip;
> > > + enum pipe pipe = plane->pipe;
> > > + u32 val;
> > > +
> > > + if (!plane_state || !dev_priv->psr.psr2_sel_fetch_enabled)
> > > + return;
> > > +
> > > + /*
> > > + * skl_plane_ctl_crtc()/i9xx_cursor_ctl_crtc() return 0 for
> > > gen11+, so
> > > + * plane_state->ctl is the right value
> > > + */
As per Bspec 50420, "SEL_FETCH_PLANE_CTL[31]: Selective Fetch Plane
Enable bit" should be set.
And when "PSR2_MAN_TRK_CTL[1] : SF Partial Frame Enable bit" is enabled
selective fetch will be applied.
> > > + intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane-
> > > >id), plane_state->ctl);
As per
> > > + if (!plane_state->ctl || plane->id == PLANE_CURSOR)
> > > + return;
> > > +
> > > + clip = &plane_state->psr2_sel_fetch_area;
> > > +
> > > + val = (clip->y1 + plane_state->uapi.crtc_y) << 16;
> >
> > crtc_x/y are the raw values usrspace gave us. That is definitely
> > not
> > what we should be looking at.
>
> plane_state->uapi.dst then? but for what I found crtc_x/y is set from
> dst.
>
> plane_state->uapi.dst is used in skl_program_plane()
>
> skl_program_plane()
> int crtc_x = plane_state->uapi.dst.x1;
> int crtc_y = plane_state->uapi.dst.y1;
> ...
> intel_de_write_fw(dev_priv, PLANE_POS(pipe, plane_id), (crtc_y
> << 16) | crtc_x);
>
>
> > As the first step I think these functions should just program the
> > registers with *exactly* the same values as we program into the
> > normal plane register. That gets us to the point where we're
> > actually programming something into the register without having to
> > complicate things with calculating the selective fetch area.
>
> Okay, I can move this to other patch but please check the comment
> above so we have this agreed for first version of the future patch.
>
> > > + val |= plane_state->uapi.crtc_x;
> > > + intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_POS(pipe, plane-
> > > >id),
> > > + val);
> > > +
> > > + val = (clip->y1 + plane_state->color_plane[color_plane].y) <<
> > > 16;
> > > + val |= plane_state->color_plane[color_plane].x;
> > > + intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_OFFSET(pipe, plane-
> > > >id),
> > > + val);
> > > +
PLANE_SEL_FETCH_OFFSET values should be considered tiling information.
this code does not consider aux surfaces and fb offsets.
> > > + /* Sizes are 0 based */
> > > + val = (drm_rect_height(clip) - 1) << 16;
> > > + val |= (drm_rect_width(&plane_state->uapi.src) >> 16) - 1;
> > > + intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe, plane-
> > > >id),
> > > + val);
> > > +}
> > > +
> > > void intel_psr2_program_trans_man_trk_ctl(const struct
> > > intel_crtc_state *crtc_state)
> > > {
> > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > @@ -1187,17 +1227,96 @@ void
> > > intel_psr2_program_trans_man_trk_ctl(const struct
> > > intel_crtc_state *crtc_st
> > > crtc_state->psr2_man_track_ctl);
> > > }
> > >
> > > -void intel_psr2_sel_fetch_update(struct intel_atomic_state
> > > *state,
> > > - struct intel_crtc *crtc)
> > > +static void psr2_man_trk_ctl_calc(struct intel_crtc_state
> > > *crtc_state,
> > > + struct drm_rect *clip, bool
> > > full_update)
> > > +{
> > > + u32 val = PSR2_MAN_TRK_CTL_ENABLE;
> > > +
> > > + if (full_update) {
> > > + val |= PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME;
> > > + goto exit;
> > > + }
> > > +
> > > + drm_WARN_ON_ONCE(crtc_state->uapi.crtc->dev, clip->y1 == -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(DIV_ROUND_UP(clip-
> > > >y2, 4) + 1);
As per Bspec 50424, " The frame is divided into blocks of four scan
lines each. The blocks are addressed starting from 1 for the first
block of the frame and ending with ROUNDUP[(TRANS_VTOTAL Vertical
Active + 1) / 4]for the last block of the frame. Software must provide
the starting and ending block address of the selective update region.
The SU Region Start Address is programmed to the first block of the
selective update region. The SU Region End Address is programmed to the
final block of the selective update region + 1."
I think it should be like, val |=
PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(DIV_ROUND_UP(clip->y2 +1, 4) + 1);
> > > +exit:
> > > + crtc_state->psr2_man_track_ctl = val;
> > > +}
> > > +
> > > +static void clip_area_update(struct drm_rect
> > > *overlap_damage_area,
> > > + struct drm_rect *damage_area)
> > > +{
> > > + if (overlap_damage_area->y1 == -1) {
> > > + overlap_damage_area->y1 = damage_area->y1;
> > > + overlap_damage_area->y2 = damage_area->y2;
> > > + return;
> > > + }
> > > +
> > > + if (damage_area->y1 < overlap_damage_area->y1)
> > > + overlap_damage_area->y1 = damage_area->y1;
> > > +
> > > + if (damage_area->y2 > overlap_damage_area->y2)
> > > + overlap_damage_area->y2 = damage_area->y2;
> > > +}
> > > +
> > > +int intel_psr2_sel_fetch_update(struct intel_atomic_state
> > > *state,
> > > + struct intel_crtc *crtc)
> > > {
> > > struct intel_crtc_state *crtc_state =
> > > intel_atomic_get_new_crtc_state(state, crtc);
> > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > + struct intel_plane_state *new_plane_state, *old_plane_state;
> > > + struct drm_rect pipe_clip = { .y1 = -1 };
> > > + struct intel_plane *plane;
> > > + bool full_update = false;
> > > + int i, ret;
> > >
> > > if (!dev_priv->psr.psr2_sel_fetch_enabled)
> > > - return;
> > > + return 0;
> > > +
> > > + ret = drm_atomic_add_affected_planes(&state->base, &crtc-
> > > >base);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + for_each_oldnew_intel_plane_in_state(state, plane,
> > > old_plane_state,
> > > + new_plane_state, i) {
> > > + struct drm_rect *plane_sel_fetch_area, temp;
> > >
> > > - crtc_state->psr2_man_track_ctl = PSR2_MAN_TRK_CTL_ENABLE |
> > > - PSR2_MAN_TRK_CTL_SF_SINGLE_FUL
> > > L_FRAME;
> > > + if (new_plane_state->uapi.crtc != crtc_state-
> > > >uapi.crtc)
> > > + continue;
> > > +
> > > + /*
> > > + * TODO: Not clear how to handle planes with negative
> > > position,
> > > + * also planes are not updated if they have a negative
> > > X
> > > + * position so for now doing a full update in this
> > > cases
> > > + */
> > > + if (new_plane_state->uapi.crtc_y < 0 ||
> > > + new_plane_state->uapi.crtc_x < 0) {
> > > + full_update = true;
> > > + break;
> > > + }
> > > +
> > > + if (!new_plane_state->uapi.visible)
> > > + continue;
> > > +
> > > + /*
> > > + * For now doing a selective fetch in the whole plane
> > > area,
> > > + * optimizations will come in the future.
> > > + */
> > > + plane_sel_fetch_area = &new_plane_state-
> > > >psr2_sel_fetch_area;
> > > + plane_sel_fetch_area->y1 = new_plane_state->uapi.src.y1
> > > >> 16;
> > > + plane_sel_fetch_area->y2 = new_plane_state->uapi.src.y2
> > > >> 16;
> > > +
> > > + temp = *plane_sel_fetch_area;
> > > + temp.y1 += new_plane_state->uapi.crtc_y;
> > > + temp.y2 += new_plane_state->uapi.crtc_y;
> > > + clip_area_update(&pipe_clip, &temp);
> > > + }
> > > +
> > > + psr2_man_trk_ctl_calc(crtc_state, &pipe_clip, full_update);
> > > + return 0;
> > > }
> > >
> > > /**
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > > b/drivers/gpu/drm/i915/display/intel_psr.h
> > > index 6a83c8e682e6..3eca9dcec3c0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > > @@ -15,6 +15,8 @@ struct intel_crtc_state;
> > > struct intel_dp;
> > > struct intel_crtc;
> > > struct intel_atomic_state;
> > > +struct intel_plane_state;
> > > +struct intel_plane;
> > >
> > > #define CAN_PSR(dev_priv) (HAS_PSR(dev_priv) && dev_priv-
> > > >psr.sink_support)
> > > void intel_psr_init_dpcd(struct intel_dp *intel_dp);
> > > @@ -45,8 +47,12 @@ void intel_psr_atomic_check(struct
> > > drm_connector *connector,
> > > struct drm_connector_state *old_state,
> > > struct drm_connector_state *new_state);
> > > void intel_psr_set_force_mode_changed(struct intel_dp
> > > *intel_dp);
> > > -void intel_psr2_sel_fetch_update(struct intel_atomic_state
> > > *state,
> > > - struct intel_crtc *crtc);
> > > +int intel_psr2_sel_fetch_update(struct intel_atomic_state
> > > *state,
> > > + struct intel_crtc *crtc);
> > > void intel_psr2_program_trans_man_trk_ctl(const struct
> > > intel_crtc_state *crtc_state);
> > > +void intel_psr2_program_plane_sel_fetch(struct intel_plane
> > > *plane,
> > > + const struct intel_crtc_state
> > > *crtc_state,
> > > + const struct intel_plane_state
> > > *plane_state,
> > > + int color_plane);
> > >
> > > #endif /* __INTEL_PSR_H__ */
> > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > index 1797a06cfd60..24ee9b08ec4a 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > @@ -690,6 +690,9 @@ skl_program_plane(struct intel_plane *plane,
> > > intel_de_write_fw(dev_priv, PLANE_AUX_OFFSET(pipe,
> > > plane_id),
> > > (plane_state->color_plane[1].y << 16)
> > > | plane_state->color_plane[1].x);
> > >
> > > + if (!drm_atomic_crtc_needs_modeset(&crtc_state->uapi))
> > > + intel_psr2_program_plane_sel_fetch(plane, crtc_state,
> > > plane_state, color_plane);
> > > +
> > > /*
> > > * The control register self-arms if the plane was previously
> > > * disabled. Try to make the plane enable atomic by writing
> > > --
> > > 2.28.0
More information about the Intel-gfx
mailing list