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. _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
