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

[Souza, Jose]

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 at linux.intel.com>
> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at 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_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 (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.

> >  		/* 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;