[Intel-gfx] [PATCH 2/6] drm/i915/display/psr: Use plane damage clips to calculate damaged area

[Souza, Jose]

On Mon, 2020-10-26 at 21:40 +0000, Mun, Gwan-gyeong wrote:
> On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza wrote:
> > Now using plane damage clips property to calcualte the damaged area.
> > Selective fetch only supports one region to be fetched so software
> > needs to calculate a bounding box around all damage clips.
> > 
> > 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 | 54 +++++++++++++++++++++-
> > --
> >  1 file changed, 49 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 773a5b5fa078..0f1e9f0fa57f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1273,6 +1273,9 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  	for_each_oldnew_intel_plane_in_state(state, plane,
> > old_plane_state,
> >  					     new_plane_state, i) {
> >  		struct drm_rect *sel_fetch_area, temp;
> > +		struct drm_mode_rect *damaged_clips;
> > +		u32 num_clips;
> > +		int j;
> >  
> > 
> > 
> > 
> >  		if (new_plane_state->uapi.crtc != crtc_state-
> > > uapi.crtc)
> >  			continue;
> > @@ -1291,13 +1294,54 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  		if (!new_plane_state->uapi.visible)
> >  			continue;
> >  
> > 
> > 
> > 
> > +		sel_fetch_area = &new_plane_state->psr2_sel_fetch_area;
> > +		sel_fetch_area->y1 = -1;
> > +
> > +		damaged_clips =
> > drm_plane_get_damage_clips(&new_plane_state->uapi);
> > +		num_clips =
> > drm_plane_get_damage_clips_count(&new_plane_state->uapi);
> > +
> >  		/*
> > -		 * For now doing a selective fetch in the whole plane
> > area,
> > -		 * optimizations will come in the future.
> > +		 * If plane moved, mark the whole plane area as damaged
> > as it
> > +		 * needs to be complete redraw in the new position.
> >  		 */
> > -		sel_fetch_area = &new_plane_state->psr2_sel_fetch_area;
> > -		sel_fetch_area->y1 = new_plane_state->uapi.src.y1 >>
> > 16;
> > -		sel_fetch_area->y2 = new_plane_state->uapi.src.y2 >>
> > 16;
> > +		if (!drm_rect_equals(&new_plane_state->uapi.dst,
> > +				     &old_plane_state->uapi.dst)) {
> > +			num_clips = 0;
> > +			sel_fetch_area->y1 = new_plane_state-
> > > uapi.src.y1 >> 16;
> > +			sel_fetch_area->y2 = new_plane_state-
> > > uapi.src.y2 >> 16;
> > +		} else if (!num_clips && new_plane_state->uapi.fb !=
> > +			   old_plane_state->uapi.fb) {
> > +			/*
> > +			 * If the plane don't have damage areas but the
> > +			 * framebuffer changed, mark the whole plane
> > area as
> > +			 * damaged.
> > +			 */
> > +			sel_fetch_area->y1 = new_plane_state-
> > > uapi.src.y1 >> 16;
> > +			sel_fetch_area->y2 = new_plane_state-
> > > uapi.src.y2 >> 16;
> > +		}
> > +
> why don't you use drm_atomic_helper_damage_merged() function here?

hum did not knew about this function, will take a look at as it does more than just merge all damaged areas.

> > +		for (j = 0; j < num_clips; j++) {
> > +			struct drm_rect damage_area;
> > +
> > +			damage_area.y1 = damaged_clips[j].y1;
> > +			damage_area.y2 = damaged_clips[j].y2;
> > +			clip_area_update(sel_fetch_area, &damage_area);
> > +		}
> > +
> > +		/*
> > +		 * No page flip, no plane moviment or no damage areas,
> > so don't
> typo (moviment -> movement)

fixed

> > +		 * fetch any pixel from memory for this plane
> > +		 */
> > +		if (sel_fetch_area->y1 == -1) {
> > +			sel_fetch_area->y1 = 0;
> > +			sel_fetch_area->y2 = 0;
> > +		}
> > +
> > +		/* Don't need to redraw plane damaged areas outside of
> > screen */
> > +		j = sel_fetch_area->y2 + (new_plane_state->uapi.dst.y1
> > > > 16);
> src coordinates of the drm_plane_state are 16.16 fixed point but dst
> coordinates are not 16.16 fixed point.
> therefore we don't need to bit shift for dst.
> Because the sel_fetch_area seems based on src coordinates, in order to
> apply to dst coordinates here,  it requires coordinate calculation. 

thanks for catching this, also fixed the same issue patch 1.

> > +		j = crtc_state->uapi.adjusted_mode.crtc_vdisplay - j;
> > +		if (j < 0)
> > +			sel_fetch_area->y2 += j;
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> >  		temp = *sel_fetch_area;
> >  		temp.y1 += new_plane_state->uapi.dst.y1 >> 16;