[Intel-gfx] [PATCH 5/6] drm/i915: Implement PSR2 selective fetch
Mun, Gwan-gyeong
gwan-gyeong.mun at intel.com
Tue Jun 16 20:33:38 UTC 2020
On Tue, 2020-06-16 at 10:29 -0700, Souza, Jose wrote:
> On Tue, 2020-06-16 at 16:16 +0100, Mun, Gwan-gyeong wrote:
> > On Mon, 2020-06-15 at 19:40 +0300, Ville Syrjälä wrote:
> > > On Fri, Jun 12, 2020 at 08:33:31PM +0000, Souza, Jose wrote:
> > > > On Fri, 2020-06-12 at 19:30 +0300, Ville Syrjälä wrote:
> > > > > On Tue, May 26, 2020 at 03:14:46PM -0700, José Roberto de
> > > > > Souza
> > > > > wrote:
> > > > > > All GEN12 platforms supports PSR2 selective fetch but not
> > > > > > all
> > > > > > GEN12
> > > > > > platforms supports PSR2 hardware tracking(aka RKL).
> > > > > >
> > > > > > This feature consists in software program registers with
> > > > > > the
> > > > > > damaged
> > > > > > area of each plane this way hardware will only fetch from
> > > > > > memory those
> > > > > > areas and sent the PSR2 selective update blocks to panel,
> > > > > > saving even
> > > > > > more power but to it actually happen userspace needs to
> > > > > > send
> > > > > > the
> > > > > > damaged areas otherwise it will still fetch the whole plane
> > > > > > as
> > > > > > fallback.
> > > > > > As today Gnome3 do not send damaged areas and the only
> > > > > > compositor that
> > > > > > I'm aware that sets the damaged areas is Weston.
> > > > > > https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/17
> > > > > >
> > > > > > So here implementing page flip part, it is still completely
> > > > > > missing
> > > > > > frontbuffer modifications, that is why the
> > > > > > enable_psr2_sel_fetch
> > > > > > parameter was added.
> > > > > >
> > > > > > The plan is to switch all GEN12 platforms to selective
> > > > > > fetch
> > > > > > when
> > > > > > ready, it will also depend in add some tests sending
> > > > > > damaged
> > > > > > areas.
> > > > > > I have a hacked version of kms_psr2_su with 3 planes that I
> > > > > > can
> > > > > > cleanup and send in a few days(99% of PSR2 selective fetch
> > > > > > changes was
> > > > > > done during my free time while bored during quarantine
> > > > > > rainy
> > > > > > days).
> > > > > >
> > > > > > BSpec: 55229
> > > > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > > Cc: Imre Deak <imre.deak at intel.com>
> > > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/display/intel_display.c | 5 +
> > > > > > .../drm/i915/display/intel_display_debugfs.c | 3 +
> > > > > > .../drm/i915/display/intel_display_types.h | 10 +
> > > > > > drivers/gpu/drm/i915/display/intel_psr.c | 329
> > > > > > +++++++++++++++++-
> > > > > > drivers/gpu/drm/i915/display/intel_psr.h | 10 +
> > > > > > drivers/gpu/drm/i915/display/intel_sprite.c | 2 +
> > > > > > drivers/gpu/drm/i915/i915_drv.h | 2 +
> > > > > > drivers/gpu/drm/i915/i915_params.c | 5 +
> > > > > > drivers/gpu/drm/i915/i915_params.h | 1 +
> > > > > > 9 files changed, 352 insertions(+), 15 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > index b69878334040..984809208c29 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > @@ -11729,6 +11729,8 @@ static void
> > > > > > i9xx_update_cursor(struct
> > > > > > intel_plane *plane,
> > > > > > if (INTEL_GEN(dev_priv) >= 9)
> > > > > > skl_write_cursor_wm(plane, crtc_state);
> > > > > >
> > > > > > + intel_psr2_program_plane_sel_fetch(plane, crtc_state,
> > > > > > plane_state);
> > > > > > +
> > > > > > if (plane->cursor.base != base ||
> > > > > > plane->cursor.size != fbc_ctl ||
> > > > > > plane->cursor.cntl != cntl) {
> > > > > > @@ -15115,6 +15117,8 @@ static void
> > > > > > commit_pipe_config(struct
> > > > > > intel_atomic_state *state,
> > > > > >
> > > > > > if (new_crtc_state->update_pipe)
> > > > > > intel_pipe_fastset(old_crtc_state,
> > > > > > new_crtc_state);
> > > > > > +
> > > > > > + intel_psr2_program_trans_man_trk_ctl(new_crtc_s
> > > > > > tate);
> > > > > > }
> > > > > >
> > > > > > if (dev_priv->display.atomic_update_watermarks)
> > > > > > @@ -15156,6 +15160,7 @@ static void
> > > > > > intel_update_crtc(struct
> > > > > > intel_atomic_state *state,
> > > > > > intel_color_load_luts(new_crtc_state);
> > > > > >
> > > > > > intel_pre_plane_update(state, crtc);
> > > > > > + intel_psr2_sel_fetch_update(state, crtc);
> > > > > >
> > > > > > if (new_crtc_state->update_pipe)
> > > > > > intel_encoders_update_pipe(state,
> > > > > > crtc);
> > > > > > diff --git
> > > > > > a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > > > > index 70525623bcdf..0f600974462b 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > > > > @@ -417,6 +417,9 @@ static int i915_edp_psr_status(struct
> > > > > > seq_file *m, void *data)
> > > > > > su_blocks = su_blocks >>
> > > > > > PSR2_SU_STATUS_SHIFT(frame);
> > > > > > seq_printf(m, "%d\t%d\n", frame,
> > > > > > su_blocks);
> > > > > > }
> > > > > > +
> > > > > > + seq_printf(m, "PSR2 selective fetch: %s\n",
> > > > > > + enableddisabled(psr-
> > > > > > > psr2_sel_fetch_enabled));
> > > > > > }
> > > > > >
> > > > > > unlock:
> > > > > > diff --git
> > > > > > a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > index 30b2767578dc..b77a512e5362 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > @@ -586,6 +586,13 @@ struct intel_plane_state {
> > > > > > u32 planar_slave;
> > > > > >
> > > > > > struct drm_intel_sprite_colorkey ckey;
> > > > > > +
> > > > > > + struct {
> > > > > > + u32 ctl;
> > > > > > + u32 pos;
> > > > > > + u32 offset;
> > > > > > + u32 size;
> > > > > > + } psr2_sel_fetch;
> > > > >
> > > > > Do we really need all that here? We don't store them for the
> > > > > normal
> > > > > plane updates either.
> > > >
> > > > For ctl we do, anyways could be removed if we store overlapping
> > > > damage are in here so intel_psr2_program_plane_sel_fetch()
> > > > would
> > > > incorporate
> > > > intel_psr2_plane_sel_fetch_calc() code, both looks good to me.
> > > >
> > > > > > };
> > > > > >
> > > > > > struct intel_initial_plane_config {
> > > > > > @@ -931,6 +938,7 @@ struct intel_crtc_state {
> > > > > >
> > > > > > bool has_psr;
> > > > > > bool has_psr2;
> > > > > > + bool enable_psr2_sel_fetch;
> > > > > > u32 dc3co_exitline;
> > > > > >
> > > > > > /*
> > > > > > @@ -1070,6 +1078,8 @@ struct intel_crtc_state {
> > > > > >
> > > > > > /* For DSB related info */
> > > > > > struct intel_dsb *dsb;
> > > > > > +
> > > > > > + u32 psr2_sw_man_track_ctl;
> > > > > > };
> > > > > >
> > > > > > enum intel_pipe_crc_source {
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > index 0c86e9e341a2..bc2a2e64fe2a 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > @@ -518,6 +518,14 @@ static void hsw_activate_psr2(struct
> > > > > > intel_dp *intel_dp)
> > > > > > else
> > > > > > val |= EDP_PSR2_TP2_TIME_2500us;
> > > > > >
> > > > > > + if (dev_priv->psr.psr2_sel_fetch_enabled)
> > > > > > + intel_de_write(dev_priv,
> > > > > > + PSR2_MAN_TRK_CTL(dev_priv-
> > > > > > > psr.transcoder),
> > > > > > + PSR2_MAN_TRK_CTL_ENABLE);
> > > > > > + else if (HAS_PSR2_SEL_FETCH(dev_priv))
> > > > > > + intel_de_write(dev_priv,
> > > > > > + PSR2_MAN_TRK_CTL(dev_priv-
> > > > > > > psr.transcoder), 0);
> > > > > > +
> > > > > > /*
> > > > > > * PSR2 HW is incorrectly using EDP_PSR_TP1_TP3_SEL and
> > > > > > BSpec is
> > > > > > * recommending keep this bit unset while PSR2 is
> > > > > > enabled.
> > > > > > @@ -628,6 +636,38 @@
> > > > > > tgl_dc3co_exitline_compute_config(struct
> > > > > > intel_dp *intel_dp,
> > > > > > crtc_state->dc3co_exitline = crtc_vdisplay -
> > > > > > exit_scanlines;
> > > > > > }
> > > > > >
> > > > > > +static bool intel_psr2_sel_fetch_config_valid(struct
> > > > > > intel_dp
> > > > > > *intel_dp,
> > > > > > + struct
> > > > > > intel_crtc_state *crtc_state)
> > > > > > +{
> > > > > > + struct intel_atomic_state *state =
> > > > > > to_intel_atomic_state(crtc_state->uapi.state);
> > > > > > + struct drm_i915_private *dev_priv =
> > > > > > dp_to_i915(intel_dp);
> > > > > > + struct intel_plane_state *plane_state;
> > > > > > + struct intel_plane *plane;
> > > > > > + int i;
> > > > > > +
> > > > > > + if (!i915_modparams.enable_psr2_sel_fetch) {
> > > > > > + drm_dbg_kms(&dev_priv->drm,
> > > > > > + "PSR2 sel fetch not enabled,
> > > > > > disabled by parameter\n");
> > > > > > + return false;
> > > > > > + }
> > > > > > +
> > > > > > + if (crtc_state->uapi.async_flip) {
> > > > > > + drm_dbg_kms(&dev_priv->drm,
> > > > > > + "PSR2 sel fetch not enabled, async
> > > > > > flip enabled\n");
> > > > > > + return false;
> > > > > > + }
> > > > >
> > > > > Not supported anyway.
> > > > >
> > > > > > +
> > > > > > + for_each_new_intel_plane_in_state(state, plane,
> > > > > > plane_state, i) {
> > > > > > + if (plane_state->uapi.rotation !=
> > > > > > DRM_MODE_ROTATE_0) {
> > > > > > + drm_dbg_kms(&dev_priv->drm,
> > > > > > + "PSR2 sel fetch not
> > > > > > enabled, plane rotated\n");
> > > > > > + return false;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + return crtc_state->enable_psr2_sel_fetch = true;
> > > > > > +}
> > > > > > +
> > > > > > static bool intel_psr2_config_valid(struct intel_dp
> > > > > > *intel_dp,
> > > > > > struct intel_crtc_state
> > > > > > *crtc_state)
> > > > > > {
> > > > > > @@ -697,22 +737,17 @@ static bool
> > > > > > intel_psr2_config_valid(struct intel_dp *intel_dp,
> > > > > > return false;
> > > > > > }
> > > > > >
> > > > > > - /*
> > > > > > - * Some platforms lack PSR2 HW tracking and instead
> > > > > > require manual
> > > > > > - * tracking by software. In this case, the driver is
> > > > > > required to track
> > > > > > - * the areas that need updates and program hardware to
> > > > > > send selective
> > > > > > - * updates.
> > > > > > - *
> > > > > > - * So until the software tracking is implemented, PSR2
> > > > > > needs to be
> > > > > > - * disabled for platforms without PSR2 HW tracking.
> > > > > > - */
> > > > > > - if (!HAS_PSR_HW_TRACKING(dev_priv)) {
> > > > > > - drm_dbg_kms(&dev_priv->drm,
> > > > > > - "No PSR2 HW tracking in the
> > > > > > platform\n");
> > > > > > - return false;
> > > > > > + if (HAS_PSR2_SEL_FETCH(dev_priv)) {
> > > > > > + if
> > > > > > (!intel_psr2_sel_fetch_config_valid(intel_dp, crtc_state)
> > > > > > &&
> > > > > > + !HAS_PSR_HW_TRACKING(dev_priv)) {
> > > > > > + drm_dbg_kms(&dev_priv->drm,
> > > > > > + "PSR2 not enabled,
> > > > > > selective fetch not valid and no HW tracking available\n");
> > > > > > + return false;
> > > > > > + }
> > > > > > }
> > > > > >
> > > > > > - if (crtc_hdisplay > psr_max_h || crtc_vdisplay >
> > > > > > psr_max_v) {
> > > > > > + if (!crtc_state->enable_psr2_sel_fetch &&
> > > > > > + (crtc_hdisplay > psr_max_h || crtc_vdisplay >
> > > > > > psr_max_v)) {
> > > > > > drm_dbg_kms(&dev_priv->drm,
> > > > > > "PSR2 not enabled, resolution %dx%d
> > > > > > > max supported %dx%d\n",
> > > > > > crtc_hdisplay, crtc_vdisplay,
> > > > > > @@ -863,6 +898,11 @@ static void
> > > > > > intel_psr_enable_source(struct
> > > > > > intel_dp *intel_dp,
> > > > > > val |= EXITLINE_ENABLE;
> > > > > > intel_de_write(dev_priv,
> > > > > > EXITLINE(cpu_transcoder), val);
> > > > > > }
> > > > > > +
> > > > > > + if (HAS_PSR_HW_TRACKING(dev_priv))
> > > > > > + intel_de_rmw(dev_priv, CHICKEN_PAR1_1,
> > > > > > IGNORE_PSR2_HW_TRACKING,
> > > > > > + dev_priv-
> > > > > > > psr.psr2_sel_fetch_enabled ?
> > > > > > + IGNORE_PSR2_HW_TRACKING : 0);
> > > > > > }
> > > > > >
> > > > > > static void intel_psr_enable_locked(struct
> > > > > > drm_i915_private
> > > > > > *dev_priv,
> > > > > > @@ -884,7 +924,7 @@ static void
> > > > > > intel_psr_enable_locked(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > > /* DC5/DC6 requires at least 6 idle frames */
> > > > > > val =
> > > > > > usecs_to_jiffies(intel_get_frame_time_us(crtc_state) * 6);
> > > > > > dev_priv->psr.dc3co_exit_delay = val;
> > > > > > -
> > > > > > + dev_priv->psr.psr2_sel_fetch_enabled = crtc_state-
> > > > > > > enable_psr2_sel_fetch;
> > > > > > /*
> > > > > > * If a PSR error happened and the driver is reloaded,
> > > > > > the EDP_PSR_IIR
> > > > > > * will still keep the error set even after the reset
> > > > > > done in the
> > > > > > @@ -1080,6 +1120,265 @@ 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)
> > > > > > +{
> > > > > > + struct drm_i915_private *dev_priv = to_i915(plane-
> > > > > > > base.dev);
> > > > > > + enum pipe pipe = plane->pipe;
> > > > > > +
> > > > > > + if (!HAS_PSR2_SEL_FETCH(dev_priv) ||
> > > > > > + !plane_state ||
> > > > > > + !crtc_state->enable_psr2_sel_fetch)
> > > > > > + return;
> > > > > > +
> > > > > > + intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe,
> > > > > > plane->id),
> > > > > > + plane_state->psr2_sel_fetch.ctl);
> > > > > > + if (!plane_state->psr2_sel_fetch.ctl || plane->id ==
> > > > > > PLANE_CURSOR)
> > > > > > + return;
> > > > > > +
> > > > > > + intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_POS(pipe,
> > > > > > plane->id),
> > > > > > + plane_state->psr2_sel_fetch.pos);
> > > > > > + intel_de_write_fw(dev_priv,
> > > > > > PLANE_SEL_FETCH_OFFSET(pipe, plane->id),
> > > > > > + plane_state->psr2_sel_fetch.offset);
> > > > > > + intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe,
> > > > > > plane->id),
> > > > > > + plane_state->psr2_sel_fetch.size);
> > > > > > +}
> > > > > > +
> > > > > > +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);
> > > > > > + struct drm_i915_private *dev_priv = to_i915(crtc-
> > > > > > > base.dev);
> > > > > > + struct i915_psr *psr = &dev_priv->psr;
> > > > > > +
> > > > > > + if (!HAS_PSR2_SEL_FETCH(dev_priv) ||
> > > > > > + !crtc_state->enable_psr2_sel_fetch)
> > > > > > + return;
> > > > > > +
> > > > > > + intel_de_write(dev_priv, PSR2_MAN_TRK_CTL(psr-
> > > > > > > transcoder),
> > > > > > + crtc_state->psr2_sw_man_track_ctl);
> > > > > > +}
> > > > > > +
> > > > > > +static void intel_psr2_plane_sel_fetch_calc(struct
> > > > > > intel_plane_state *plane_state,
> > > > > > + struct drm_rect
> > > > > > *clip)
> > > > > > +{
> > > > > > + int color_plane = plane_state->planar_linked_plane &&
> > > > > > !plane_state->planar_slave;
> > > > > > + struct intel_plane *plane = to_intel_plane(plane_state-
> > > > > > > uapi.plane);
> > > > > > + u32 val;
> > > > > > +
> > > > > > + if (plane->id == PLANE_CURSOR)
> > > > > > + return;
> > > > > > +
> > > > > > + val = (plane_state->color_plane[color_plane].y + clip-
> > > > > > > y1) << 16;
> > > > > > + val |= plane_state->color_plane[color_plane].x;
> > > > > > + plane_state->psr2_sel_fetch.offset = val;
> > > > > > +
> > > > > > + val = (clip->y1 + plane_state->uapi.crtc_y) << 16;
> > > > > > + val |= plane_state->uapi.crtc_x;
> > > > > > + plane_state->psr2_sel_fetch.pos = val;
> > > > > > +
> > > > > > + /* Sizes are 0 based */
> > > > > > + val = (clip->y2 - clip->y1 - 1) << 16;
> > > > > > + val |= (drm_rect_width(&plane_state->uapi.src) >> 16) -
> > > > > > 1;
> > > > > > + plane_state->psr2_sel_fetch.size = val;
> > > > > > +}
> > > > > > +
> > > > > > +static void intel_psr2_trans_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_SINGLE_FULL_FRAME;
> > > > > > + goto exit;
> > > > > > + }
> > > > > > +
> > > > > > + if (clip->y1 == -1)
> > > > > > + goto exit;
> > > > > > +
> > > > > > + val |= PSR2_MAN_TRK_CTL_PARTIAL_FRAME_UPDATE;
> > > > > > + val |= PSR2_MAN_TRK_CTL_REGION_START_ADDR(clip->y1 / 4
> > > > > > + 1);
> > > > > > + val |=
> > > > > > PSR2_MAN_TRK_CTL_REGION_END_ADDR(DIV_ROUND_UP(clip->y2, 4)
> > > > > > +
> > > > > > 1);
> > > > > > +exit:
> > > > > > + crtc_state->psr2_sw_man_track_ctl = val;
> > > > > > +}
> > > > > > +
> > > > > > +static void intel_psr2_plane_sel_fetch_ctl_calc(struct
> > > > > > intel_plane *plane,
> > > > > > + struct
> > > > > > intel_plane_state *plane_state,
> > > > > > + bool enable)
> > > > > > +{
> > > > > > + if (!enable)
> > > > > > + plane_state->psr2_sel_fetch.ctl = 0;
> > > > > > + else if (plane->id == PLANE_CURSOR)
> > > > > > + plane_state->psr2_sel_fetch.ctl = plane-
> > > > > > > cursor.cntl;
> > > > > > + else
> > > > > > + plane_state->psr2_sel_fetch.ctl = plane_state-
> > > > > > > ctl;
> > > > > > +}
> > > > > > +
> > > > > > +static void clip_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;
> > > > > > +}
> > > > > > +
> > > > > > +/* Update plane damage area if planes above moved or have
> > > > > > alpha */
> > > > > > +static void intel_psr2_pipe_dirty_areas_set(struct
> > > > > > intel_plane_state *plane_state,
> > > > > > + struct intel_plane
> > > > > > *plane,
> > > > > > + const struct
> > > > > > drm_rect *pipe_dirty_areas,
> > > > > > + struct drm_rect
> > > > > > *plane_clip)
> > > > > > +{
> > > > > > + enum plane_id i;
> > > > > > +
> > > > > > + for (i = PLANE_CURSOR; i > plane->id; i--) {
> > > > > > + int j;
> > > > > > +
> > > > > > + for (j = 0; j < 2; j++) {
> > > > > > + struct drm_rect r = pipe_dirty_areas[i
> > > > > > * 2 + j];
> > > > > > +
> > > > > > + if (!drm_rect_width(&r))
> > > > > > + continue;
> > > > > > + if (!drm_rect_intersect(&r,
> > > > > > &plane_state->uapi.dst))
> > > > > > + continue;
> > > > > > +
> > > > > > + r.y1 -= plane_state->uapi.crtc_y;
> > > > > > + r.y2 -= plane_state->uapi.crtc_y;
> > > > > > + clip_update(plane_clip, &r);
> > > > > > + }
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > > > +void 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 intel_plane_state *new_plane_state,
> > > > > > *old_plane_state;
> > > > > > + struct drm_rect pipe_dirty_areas[I915_MAX_PLANES * 2] =
> > > > > > {};
> > > > > > + struct drm_rect pipe_clip = { .y1 = -1 };
> > > > > > + struct intel_plane *plane;
> > > > > > + bool full_update = false;
> > > > > > + int i;
> > > > > > +
> > > > > > + if (!crtc_state->enable_psr2_sel_fetch)
> > > > > > + return;
> > > > > > +
> > > > > > + /*
> > > > > > + * Load all the pipes areas where there is a plane with
> > > > > > alpha or a plane
> > > > > > + * that moved or plane that the visibility changed in
> > > > > > those
> > > > > > + * cases planes bellow it will need to be fetched in
> > > > > > those intersection
> > > > > > + * areas even if they are not damaged in those areas.
> > > > > > + */
> > > > > > + for_each_oldnew_intel_plane_in_state(state, plane,
> > > > > > old_plane_state,
> > > > > > + new_plane_state,
> > > > > > i) {
> > > > > > + bool alpha, flip, dirty;
> > > > > > +
> > > > > > + if (new_plane_state->uapi.crtc != crtc_state-
> > > > > > > uapi.crtc)
> > > > > > + continue;
> > > > > > +
> > > > > > + alpha = new_plane_state->uapi.alpha != U16_MAX;
> > > > > > + alpha |= old_plane_state->uapi.alpha !=
> > > > > > U16_MAX;
> > > > > > + flip = new_plane_state->uapi.fb !=
> > > > > > old_plane_state->uapi.fb;
> > > > > > + dirty = alpha && flip;
> > > > > > + dirty |= !drm_rect_equals(&new_plane_state-
> > > > > > > uapi.dst,
> > > > > > + &old_plane_state-
> > > > > > > uapi.dst);
> > > > > > + dirty |= new_plane_state->uapi.visible !=
> > > > > > + old_plane_state->uapi.visible;
> > > > > > + if (!dirty)
> > > > > > + continue;
> > > > > > +
> > > > > > + if (old_plane_state->uapi.visible)
> > > > > > + pipe_dirty_areas[plane->id * 2] =
> > > > > > old_plane_state->uapi.dst;
> > > > > > + if (new_plane_state->uapi.visible)
> > > > > > + pipe_dirty_areas[plane->id * 2 + 1] =
> > > > > > new_plane_state->uapi.dst;
> > > > > > + }
> > > > > > +
> > > > > > + /*
> > > > > > + * Iterate over all planes, compute the damaged clip
> > > > > > area also including
> > > > > > + * the pipe_dirty_areas, compute plane registers and
> > > > > > update pipe damaged
> > > > > > + * area
> > > > > > + */
> > > > > > + for_each_oldnew_intel_plane_in_state(state, plane,
> > > > > > old_plane_state,
> > > > > > + new_plane_state,
> > > > > > i) {
> > > > > > + struct drm_rect plane_clip = { .y1 = -1 };
> > > > > > + struct drm_mode_rect *clips;
> > > > > > + u32 num_clips;
> > > > > > + int j;
> > > > > > +
> > > > > > + 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;
> > > > > > + }
> > > > > > +
> > > > > > + intel_psr2_plane_sel_fetch_ctl_calc(plane,
> > > > > > new_plane_state,
> > > > > > + new_plane_s
> > > > > > tate->uapi.visible);
> > > > > > + if (!new_plane_state->uapi.visible)
> > > > > > + continue;
> > > > > > +
> > > > > > + clips =
> > > > > > drm_plane_get_damage_clips(&new_plane_state->uapi);
> > > > > > + num_clips =
> > > > > > drm_plane_get_damage_clips_count(&new_plane_state->uapi);
> > > > > > +
> > > > > > + /*
> > > > > > + * If plane moved mark the whole plane area as
> > > > > > damaged so it
> > > > > > + * can be complete draw in the new position
> > > > > > + */
> > > > > > + if (!drm_rect_equals(&new_plane_state-
> > > > > > > uapi.dst,
> > > > > > + &old_plane_state-
> > > > > > > uapi.dst)) {
> > > > > > + num_clips = 0;
> > > > > > + plane_clip.y1 = new_plane_state-
> > > > > > > uapi.src.y1 >> 16;
> > > > > > + plane_clip.y2 = new_plane_state-
> > > > > > > uapi.src.y2 >> 16;
> > > > > > + } else if (!num_clips) {
> > > > > > + /*
> > > > > > + * If plane don't have damage areas but
> > > > > > the framebuffer
> > > > > > + * changed mark the whole plane as
> > > > > > damaged
> > > > > > + */
> > > > > > + if (new_plane_state->uapi.fb ==
> > > > > > old_plane_state->uapi.fb)
> > > > > > + continue;
> > > > > > +
> > > > > > + plane_clip.y1 = new_plane_state-
> > > > > > > uapi.src.y1 >> 16;
> > > > > > + plane_clip.y2 = new_plane_state-
> > > > > > > uapi.src.y2 >> 16;
> > > > > > + }
> > > > > > +
> > > > > > + for (j = 0; j < num_clips; j++) {
> > > > > > + struct drm_rect damage_area;
> > > > > > +
> > > > > > + damage_area.x1 = clips[j].x1;
> > > > > > + damage_area.x2 = clips[j].x2;
> > > > > > + damage_area.y1 = clips[j].y1;
> > > > > > + damage_area.y2 = clips[j].y2;
> > > > > > + clip_update(&plane_clip, &damage_area);
> > > > > > + }
> > > > > > +
> > > > > > + intel_psr2_pipe_dirty_areas_set(new_plane_state
> > > > > > , plane,
> > > > > > + pipe_dirty_area
> > > > > > s, &plane_clip);
> > > > > > + intel_psr2_plane_sel_fetch_calc(new_plane_state
> > > > > > , &plane_clip);
> > > > > > +
> > > > > > + plane_clip.y1 += new_plane_state->uapi.crtc_y;
> > > > > > + plane_clip.y2 += new_plane_state->uapi.crtc_y;
> > > > > > + clip_update(&pipe_clip, &plane_clip);
> > > > > > + }
> > > > >
> > > > > This whole thing seems rather convoluted. Also using lots of
> > > > > uapi
> > > > > state
> > > > > in places where I don't expect to see any.
> > > >
> > > > Not sure from where I should get this information then,
> > > > intel_plane_state don't have it.
> > > >
> > > > > I would suggest the correct way would be something like:
> > > > > 1) for_each_plane_in_state()
> > > > > hw.damage =
> > > > > translate_to_some_hw_coord_space(union(uapi.damages))
> > > > > or just use the full plane size if we have scaling i
> > > > > guess
> > > >
> > > > 99% of the time the coordinates used are based on pipe coord
> > > > space,
> > > > only to calculate the plane overlapping damaged area is used
> > > > plane
> > > > coord space.
> > > >
> > > > > 2) need to add all affected planes to the state and set the
> > > > > appropriate
> > > > > bitmask, which may mean we want to track the planes'
> > > > > positions
> > > > > in the
> > > > > crtc state. I think atm we only have it in the plane state
> > > >
> > > > This looks a "or" to me, have all the planes added to the state
> > > > when psr2 sel fetch is enabled or add track all the planes
> > > > position
> > > > in pipe.
> > >
> > > *Affected* planes, not all planes. Hmm. I guess affected planes
> > > are
> > > actually the ones whose selective fetch coordinates change. If
> > > they
> > > don't change then no need to add them to the state. Plane updates
> > > are
> > > rather expensive (lots of mmio) so I've generally tried to avoid
> > > pointless plane updates.
> > >
> > > But this whole thing might turn a bit annoying since we'd to keep
> > > adding affected planes until the total selective fetch region
> > > stops
> > > growing. I think that would probably want the two stage plane
> > > state
> > > compuation. So just blindly adding all of them would probably be
> > > simpler, albeit less efficient.
> > >
> > > > Although the second one would avoid us to do plane calculations
> > > > and
> > > > plane register sometimes, in some cases where a plane above a
> > > > non-
> > > > modified plane
> > > > moves the non-modified plane bellow will need to be added to
> > > > the
> > > > state so the plane sel_fetch registers are written.
> > > > We could go with the easy one(add all planes to the state) and
> > > > then
> > > > move to the second one latter.
> > > >
> > > > > 3) translate the damage further into the final plane src
> > > > > coordinate
> > > > > space. Dunno if we have enough state around still to do it
> > > > > cleanly.
> > > > > I was thinking maybe it could be done alongside all the
> > > > > other
> > > > > plane
> > > > > surface calculations, but there might be a chicken vs. egg
> > > > > situation
> > > > > here since we probably want to do the plane check stuff
> > > > > before
> > > > > doing
> > > > > step 1, but plane check is also where we do the surface
> > > > > calculations.
> > > > > Dunno if we may just want to split the plane check into
> > > > > two
> > > > > stages
> > > >
> > > > As right now it depends mostly in uapi this could be moved to
> > > > the
> > > > check phase, did not left there because this will never have a
> > > > error or a conflict
> > > > that will cause us to reject the state.
> > > >
> > > > > To keep things simple I guess what I'd suggest is to forget
> > > > > about
> > > > > the
> > > > > damage stuff in the first version of the series and just do
> > > > > full
> > > > > plane updates. That way we don't have to worry about so many
> > > > > coordinate
> > > > > space transformations.
> > > >
> > > > Do that would only save us the for bellow and the if to check
> > > > if
> > > > plane moved:
> > > >
> > > > for (j = 0; j < num_clips; j++) {
> > > > struct drm_rect damage_area;
> > > >
> > > > damage_area.x1 = clips[j].x1;
> > > > damage_area.x2 = clips[j].x2;
> > > > damage_area.y1 = clips[j].y1;
> > > > damage_area.y2 = clips[j].y2;
> > > > clip_update(&plane_clip, &damage_area);
> > > > }
> > >
> > > That's just some minor detail. The real issue is converting the
> > > damage
> > > between the various coordinate spaces we have for planes
> > > (original fb
> > > relative src coordiantes, final SURF relative src coordinates,
> > > crtc relative dst coordinates, and also the hw vs. uapi stuff
> > > affects
> > > this stuff).
> > >
> > For the most efficient power comsumption and usage of bandthwidth,
> > we
> > can use Selective Fetch of Plane and PSR2 Manual Tracking together.
> > But PSR2 Manual Tracking can be enabled without Selective Fetch of
> > Plane. (And pre GEN12 does not have a feature "Selective Fetch of
> > Plane".)
> > So can you split this commit to Selective Fetch and PSR2 Manual
> > Tracking?
>
> Pre GEN12 have selective fetch of plane, check BSpec: 33712 and
> 33711.
> The programming sequences states that program plane selective fetch
> registers and PSR2 Manual tracking must be combined, otherwise HW
> don't know if
> regular plane registers or selective fetch registers needs to be
> used.
Hi,
(Such as SKL and ICL LP platforms, they don't have a feature of
Selective Fetch Plane, therefore when PSR2_MAN_TRK is used, regular
plane registers will be used on that platforms. - but PreGEN12 is not
scope of this series.)
GEN12 (such as TGL LP )platform has "BitField: SF Partial Frame
Enable" on Register_PSR2_MAN_TRK_CTL.
The desciptions says "This field enables the planes to use the
SEL_FETCH registers for selective fetch on selective update frames.".
IMHO, this bit can be used to select a plane register (regular or
selective fetch) for PSR2_MAN_TRK.
More information about the Intel-gfx
mailing list