On Tue, 2020-06-16 at 21:33 +0100, Mun, Gwan-gyeong wrote: > 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@xxxxxxxxxxxxxxx> > > > > > > > Cc: Imre Deak <imre.deak@xxxxxxxxx> > > > > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx> > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> > > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx> > > > > > > > --- > > > > > > > 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. Will only focus in GEN12 in my comments. One of the following must be set to have the panel refreshed: "SF Partial Frame Enable", "SF Continuous full frame" and "SF Continuous full frame". Have a patch that only sets "SF Continuous full frame" or "SF Continuous full frame" would be very simple but then almost complete overwritten for the one using "SF Partial Frame Enable" and like you said the power savings and bandwidth would be at the same level as PSR1, not sure how useful it is. Anyways will check if useful when sending a new version of first 4 patches with comments addressed separated. _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
