[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