On Tue, 2020-09-15 at 12:57 -0700, Souza, Jose wrote:
> On Tue, 2020-09-15 at 20:28 +0100, Mun, Gwan-gyeong wrote:
> > 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@xxxxxxxxx
> > > > >
> > > > > Cc: Ville Syrjälä <
> > > > > ville.syrjala@xxxxxxxxxxxxxxx
> > > > >
> > > > > Signed-off-by: José Roberto de Souza <
> > > > > jose.souza@xxxxxxxxx
> > > > >
> > > > > ---
> > > > > 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.
>
> Bit 31 from PLANE_CTL is the enabled bit all the other fields are
> spare so we can program it without issues.
>
> > > > > + 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.
>
> Not familiar with this, could explain more?
As per Bspec: 55229,
Sel_Fetch Plane Y Offset = Plane Y offset + Update offset
the Plane Y offset is calculated with tiling information for non linear
surface. (linear surface also needs to calculate with the pitch ...)
therefore it seems that we need to separately calculate for Sel_Fetch
Plane Y Offset
refer. intel_adjust_aligned_offset()
> plane_state->color_plane[color_plane].x/y are the ones used to
> program plane_offset, so some calculation will be needed before sum
> plane_state-
> > color_plane[color_plane].y to clip->y1?
> > > > > + /* 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);
>
> DIV_ROUND_UP(clip->y2 +1, 4) + 1 do not work for numbers that divide
> by 4.
> clip->y2 = 1079, will result in 271
> clip->y2 = 1080, will result in 272, one more block than a 1080 frame
> have
>
> > > > > +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_SI
> > > > > NGLE_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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
