Re: [Intel-gfx] [PATCH 3/6] drm/i915/display/psr: Consider other planes to damaged area calculation

["Mun, Gwan-gyeong" <gwan-gyeong.mun@intel.com>]

On Tue, 2020-12-01 at 09:44 -0800, Souza, Jose wrote:
> On Tue, 2020-12-01 at 17:33 +0000, Mun, Gwan-gyeong wrote:
> > On Tue, 2020-10-27 at 10:25 -0700, Souza, Jose wrote:
> > > On Tue, 2020-10-27 at 13:34 +0000, Mun, Gwan-gyeong wrote:
> > > > On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza wrote:
> > > > > Planes can individually have transparent, move or have
> > > > > visibility
> > > > > changed if any of those happens, planes bellow it will be
> > > > > visible
> > > > > or
> > > > > have more pixels of it visible than before.
> > > > > 
> > > > > This patch is taking care of this case for selective fetch by
> > > > > adding
> > > > > to each plane damaged area all the intersections of planes
> > > > > above
> > > > > it
> > > > > that matches with the characteristics described above.
> > > > > 
> > > > > There still some room from improvements here but at least
> > > > > this
> > > > > initial
> > > > > version will take care of display what is expected saving
> > > > > some
> > > > > memory
> > > > > reads.
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_psr.c | 62
> > > > > ++++++++++++++++++++++++
> > > > >  1 file changed, 62 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > index 0f1e9f0fa57f..91ba97bf609b 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > @@ -1253,11 +1253,38 @@ static void clip_area_update(struct
> > > > > drm_rect
> > > > > *overlap_damage_area,
> > > > >  		overlap_damage_area->y2 = damage_area->y2;
> > > > >  }
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > +/* Update plane damage area if planes above moved or have
> > > > > alpha
> > > > > */
> > > > > +static void pipe_dirty_areas_set(struct intel_plane_state
> > > > > *plane_state,
> > > > > +				 struct intel_plane *plane,
> > > > > +				 const struct drm_rect
> > > > > *pipe_dirty_areas,
> > > > > +				 struct drm_rect
> > > > > *sel_fetch_area)
> > > > > +{
> > > > > +	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.dst.y1;
> > > > > +			r.y2 -= plane_state->uapi.dst.y1;
> > > > typo of y2?
> > > 
> > > Not a typo in this case.
> > > 
> > > > > +			clip_area_update(sel_fetch_area, &r);
> > > > sel_fetch_area has plane coordinates, but it tried to apply dst
> > > > coordinates.
> > > 
> > > the subtraction above is converting pipe/dst coordinates to the
> > > current plane coordinates. 
> > > 
> > > > > +		}
> > > > > +	}
> > > > > +}
> > > > > +
> > > > >  int intel_psr2_sel_fetch_update(struct intel_atomic_state
> > > > > *state,
> > > > >  				struct intel_crtc *crtc)
> > > > >  {
> > > > >  	struct intel_crtc_state *crtc_state =
> > > > > intel_atomic_get_new_crtc_state(state, crtc);
> > > > >  	struct 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;
> > > > > @@ -1270,6 +1297,38 @@ int intel_psr2_sel_fetch_update(struct
> > > > > intel_atomic_state *state,
> > > > >  	if (ret)
> > > > >  		return ret;
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > +	/*
> > > > > +	 * Mark all the areas where there is a plane that
> > > > > matches one
> > > > > of this:
> > > > > +	 * - transparent
> > > > > +	 * - moved
> > > > > +	 * - visibility changed
> > > > > +	 * In all those cases, planes bellow it will need to be
> > > > > redraw.
> > > > > +	 */
> > (for future visibility and fully obscured plane checking) we
> > can  traverse from top to down, by adding  and using
> > for_each_oldnew_intel_plane_in_state_reverse()
> > > > > +	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 !=
> > > > > DRM_BLEND_ALPHA_OPAQUE;
> > > > > +		alpha |= old_plane_state->uapi.alpha !=
> > > > > DRM_BLEND_ALPHA_OPAQUE;
> > > > > +		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 we can calculate SU region here, we don't need to store all of
> > the
> > dirty areas.
> 
> If the planes bellow don't have damaged areas or if it don't have any
> overlap with other planes we should not include it do the SU region
> to save
> power.
> 
If you traverse to down from top and calculate visible area, we can
calculate without storing all of dirty areas.

> > > > > +		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;
> > > > > +	}
> > > > > +
> > > > >  	for_each_oldnew_intel_plane_in_state(state, plane,
> > > > > old_plane_state,
> > > > >  					     new_plane_state,
> > > > > i) {
> > > > >  		struct drm_rect *sel_fetch_area, temp;
> > > > > @@ -1337,6 +1396,9 @@ int intel_psr2_sel_fetch_update(struct
> > > > > intel_atomic_state *state,
> > > > >  			sel_fetch_area->y2 = 0;
> > > > >  		}
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > +		pipe_dirty_areas_set(new_plane_state, plane,
> > > > > pipe_dirty_areas,
> > > > > +				     sel_fetch_area);
> > > > > +
> > > > In my humble opinion, plane_state's alpha and visibility might
> > > > affect
> > > > to PSR Selective Update, but it would not affect to damage
> > > > region
> > > > of
> > > > the Plane Selective Fetch program. therefore if we want to
> > > > calculate
> > > > with changing of alpha and visibility, we need to separate the
> > > > data
> > > > structure of clip rects and calculating clip region to "plane's
> > > > selective fetch" and "PSR's selective update".
> > > 
> > > Why separate? We need one clip region to program selective fetch
> > > plane registers.
> > > Like said in the commit description, this could be optimized in
> > > future like to check if pixels changed in the damaged overlap
> > > area
> > > with a plane with
> > > alpha but that will make things really complicated so for now
> > > keeping
> > > this more simple approach, compositors are not even supporting
> > > damage
> > > area yet.
> > weston (wayland reference compositor) supports damage area feature.
> 
> Will try it, anything need to be enabled or configure to this to
> work?
> Any instructions that you can share will save time for me.
> 
> > > > >  		/* Don't need to redraw plane damaged areas
> > > > > outside of
> > > > > screen */
> > > > >  		j = sel_fetch_area->y2 + (new_plane_state-
> > > > > >uapi.dst.y1
> > > > > > > 16);
> > > > >  		j = crtc_state-
> > > > > >uapi.adjusted_mode.crtc_vdisplay - j;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx