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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx>
> > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > > > ---
> > > > >  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?


> (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@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux