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

[Souza, Jose]

On Tue, 2020-10-27 at 01:04 +0000, Souza, Jose wrote:
> 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.

We can't use this function as it marks the whole plane area as damaged if there is no damaged clips.
But for our use case this is bad as we add all planes of the pipe/CRTC to the state, so it would cause a full fetch of the planes without any
flip/modification.

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