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

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

On Tue, 2020-12-01 at 13:15 -0800, Souza, Jose wrote:
> On Tue, 2020-12-01 at 19:40 +0000, Mun, Gwan-gyeong wrote:
> > On Tue, 2020-12-01 at 09:39 -0800, Souza, Jose wrote:
> > > On Tue, 2020-12-01 at 17:26 +0000, Mun, Gwan-gyeong wrote:
> > > > On Tue, 2020-10-27 at 13:12 -0700, Souza, Jose wrote:
> > > > > 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@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 | 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;
> > > > selective fetch area also can be affected by Selective Updated
> > > > region. 
> > > > therefore Selective Updated region rect should be calculated
> > > > first
> > > > and
> > > > then the plane's selective fetch area should be applied
> > > > (intersected by
> > > > calculated SU.)
> > > > please check this implementation.
> > > 
> > > Why selective update region needs to be calculate first if it
> > > should
> > > be based on the plane damage areas/selective fetch areas?
> > > Can you give a example where it gives not the expected result?
> > > 
> > I drew the example here : 
> > https://docs.google.com/presentation/d/1MI20q_3GublGYsY2TDheRncOQsbIjMn20aQ99loaoFg/edit?usp=sharing
> 
> Thanks so much for the drawing, now I got what you were talking
> about.
> This point is not very clear in BSpec but you are right we need to
> set plane selective fetch area using the whole area damaged in pipe.
> 
> Can you take a look at the last patch in this branch? 
> https://github.com/zehortigoza/linux/tree/psr2-sel-fetch-part3-v4
I commented your WIP patch.
> Will do some testing and squash into the right commits and resend in
> the next couple of the days.
> 
> > > > (
> > > > https://patchwork.freedesktop.org/patch/404264/?series=84340&rev=1
> > > > )
> > > > > > > > @@ -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@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx