Re: [PATCH v7 2/5] 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 Fri, 2020-12-18 at 16:31 +0000, Mun, Gwan-gyeong wrote:
> On Thu, 2020-12-17 at 13:13 -0800, 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.
> > 
> > Now that we are not complete fetching each plane, there is another
> > loop needed as all the plane areas that intersect with the pipe
> > damaged area needs to be fetched from memory so the complete blending
> > of all planes can happen.
> > 
> > v2:
> > - do not shifthing new_plane_state->uapi.dst only src is in 16.16 
> Hi,
> typo here, shifthing -> shifting
> > format
> > 
> > v4:
> > - setting plane selective fetch area using the whole pipe damage area
> > - mark the whole plane area damaged if plane visibility or alpha
> > changed
> > 
> > v5:
> > - taking in consideration src.y1 in the damage coordinates
> > - adding to the pipe damaged area planes that were visible but are
> > invisible in the new state
> > 
> > v6:
> > - consider old state plane coordinates when visibility changes or it
> > moved to calculate damaged area
> > - remove from damaged area the portion not in src clip
> > 
> > v7:
> > - intersec every damage clip with src to minimize damaged area
> > 
> > v8:
> > - adjust pipe_damaged area to 4 lines grouping
> > - adjust calculation now that is understood that uapi.src is the
> > framebuffer coordinates that plane will start to fetch from
> > 
> > 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 | 105 ++++++++++++++++++++-
> > --
> >  1 file changed, 91 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index d9a395c486d3..29cae2802089 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1242,9 +1242,11 @@ static void psr2_man_trk_ctl_calc(struct
> > intel_crtc_state *crtc_state,
> >  	if (clip->y1 == -1)
> >  		goto exit;
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > +	drm_WARN_ON(crtc_state->uapi.crtc->dev, clip->y1 % 4 || clip-
> > > y2 % 4);
> why do you check this line?

To make sure that clip->y1 and y2 can divide by 4, otherwise PSR2_MAN_TRK_CTL will be programmed with truncated values.

> > +
> >  	val |= PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE;
> >  	val |= PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(clip->y1 / 4 + 1);
> > -	val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(DIV_ROUND_UP(clip-
> > > y2, 4) + 1);
> > +	val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2 / 4 + 1);
> PSR2_MAN_TRK_CTL's the programming note of bspec describes as,
> 
> The frame is divided into blocks of four scan lines each. The blocks
> are addressed starting from 1 for the first block of the frame and
> ending with ROUNDUP[(TRANS_VTOTAL Vertical Active + 1) / 4]for the last
> block of the frame.
> Software must provide the starting and ending block address of the
> selective update region.
> The SU Region Start Address is programmed to the first block of the
> selective update region.
> The SU Region End Address is programmed to the final block of the
> selective update region + 1.
> There can be only one selective update region in a frame.
> To disable selective update, set the selective update region to the
> full frame by programming SU Region Start Address to the startof the
> frame and SU Region End Address to the end of the frame.
> 
> if the why not you did not set like this?
>  val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2 / 4 + 2); 

Imagine that clip->y2 = 800.

800 / 4 + = 202
Now if you convert it back to pixel:

# it starts on 1
202 - 1 = 201
201 * 4 = 804

The roundup in the description above is because of cases like clip->y2 = 799, in this case the "+2" would work but it will not for numbers that divide
by 4.

> 
> >  exit:
> >  	crtc_state->psr2_man_track_ctl = val;
> >  }
> > @@ -1269,8 +1271,8 @@ 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 drm_rect pipe_clip = { .x1 = 0, .y1 = -1, .x2 = INT_MAX,
> why don't you use crtc_state->uapi.adjusted_mode.crtc_hdisplay for
> setting x2?

Because it do not matters if it is equals to uapi.adjusted_mode.crtc_hdisplay or larger, we will not use X values to program registers, using the line
above would only add one more line of code.

> > .y2 = -1 };
> >  	struct intel_plane_state *new_plane_state, *old_plane_state;
> > -	struct drm_rect pipe_clip = { .y1 = -1 };
> >  	struct intel_plane *plane;
> >  	bool full_update = false;
> >  	int i, ret;
> > @@ -1282,9 +1284,17 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  	if (ret)
> >  		return ret;
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > +	/*
> > +	 * Calculate minimal selective fetch area of each plane and
> > calculate
> > +	 * the pipe damaged area.
> > +	 * In the next loop the plane selective fetch area will
> > actually be set
> > +	 * using whole pipe damaged area.
> > +	 */
> >  	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_rect src, damaged_area = { .y1 = -1 };
> > +		struct drm_mode_rect *damaged_clips;
> > +		u32 num_clips, j;
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> >  		if (new_plane_state->uapi.crtc != crtc_state-
> > > uapi.crtc)
> >  			continue;
> > @@ -1300,23 +1310,90 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  			break;
> >  		}
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > -		if (!new_plane_state->uapi.visible)
> > -			continue;
> > +		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 visibility or plane moved, mark the whole plane
> > area as
> > +		 * damaged as it needs to be complete redraw in the new
> > and old
> > +		 * 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 (new_plane_state->uapi.visible != old_plane_state-
> > > uapi.visible ||
> > +		    !drm_rect_equals(&new_plane_state->uapi.dst,
> > +				     &old_plane_state->uapi.dst)) {
> > +			damaged_area.y1 = old_plane_state->uapi.dst.y1;
> > +			damaged_area.y2 = old_plane_state->uapi.dst.y2;
> > +			clip_area_update(&pipe_clip, &damaged_area);
> > +
> > +			damaged_area.y1 = new_plane_state->uapi.dst.y1;
> > +			damaged_area.y2 = new_plane_state->uapi.dst.y2;
> > +			clip_area_update(&pipe_clip, &damaged_area);
> > +			continue;
> if (new_plane_state->uapi.visible != old_plane_state->uapi.visible) {
> 	if (new_plane_state->uapi.visible) {
> 		damaged_area.y1 = new_plane_state->uapi.dst.y1;
> 		damaged_area.y2 = new_plane_state->uapi.dst.y2;
> 	} else {
> 		damaged_area.y1 = old_plane_state->uapi.dst.y1;
> 		damaged_area.y2 = old_plane_state->uapi.dst.y2;
> 	}
> 	clip_area_update(&pipe_clip, &damaged_area);

The invisible state will have dst.y1/y2 set to 0, so we can optimize and use the same code for visibiltiy change and move.

> 	continue;
> } else if (!drm_rect_equals(&new_plane_state->uapi.dst,
> &old_plane_state->uapi.dst) {
> 	damaged_area.y1 = old_plane_state->uapi.dst.y1;
> 	damaged_area.y2 = old_plane_state->uapi.dst.y2;
> 	clip_area_update(&pipe_clip, &damaged_area);
> 
> 	damaged_area.y1 = new_plane_state->uapi.dst.y1;
> 	damaged_area.y2 = new_plane_state->uapi.dst.y2;
> 	clip_area_update(&pipe_clip, &damaged_area);
> 	continue;
> }
> > +		} else if (new_plane_state->uapi.alpha !=
> > old_plane_state->uapi.alpha ||
> > +			   (!num_clips &&
> > +			    new_plane_state->uapi.fb !=
> > old_plane_state->uapi.fb)) {
> > +			/*
> > +			 * If the plane don't have damaged areas but
> > the
> > +			 * framebuffer changed or alpha changed, mark
> > the whole
> > +			 * plane area as damaged.
> > +			 */
> > +			damaged_area.y1 = new_plane_state->uapi.dst.y1;
> > +			damaged_area.y2 = new_plane_state->uapi.dst.y2;
> > +			clip_area_update(&pipe_clip, &damaged_area);
> > +			continue;
> > +		}
> > +
> if the num_clips is over 0, we need to do the below section,

What section?

> > +		drm_rect_fp_to_int(&src, &new_plane_state->uapi.src);
> > +		damaged_clips =
> > drm_plane_get_damage_clips(&new_plane_state->uapi);
> > +
> need to initialize damaged_area rect here too.

It is initialized.

struct drm_rect src, damaged_area = { .y1 = -1 };

> > +		for (j = 0; j < num_clips; j++) {
> > +			struct drm_rect clip;
> > +
> > +			clip.x1 = damaged_clips[j].x1;
> > +			clip.y1 = damaged_clips[j].y1;
> > +			clip.x2 = damaged_clips[j].x2;
> > +			clip.y2 = damaged_clips[j].y2;
> > +			if (drm_rect_intersect(&clip, &src))
> > +				clip_area_update(&damaged_area, &clip);
> > +		}
> > +
> > +		if (damaged_area.y1 == -1)
> > +			continue;
> > +
> > +		damaged_area.y1 += new_plane_state->uapi.dst.y1 -
> > src.y1;
> > +		damaged_area.y2 += new_plane_state->uapi.dst.y1 -
> > src.y1;
> > +		clip_area_update(&pipe_clip, &damaged_area);
> > +	}
> > +
> > +	if (full_update)
> > +		goto skip_sel_fetch_set_loop;
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > -		temp = *sel_fetch_area;
> > -		temp.y1 += new_plane_state->uapi.dst.y1;
> > -		temp.y2 += new_plane_state->uapi.dst.y2;
> > -		clip_area_update(&pipe_clip, &temp);
> > +	/* It must be aligned to 4 lines */
> > +	pipe_clip.y1 -= pipe_clip.y1 % 4;
> > +	if (pipe_clip.y2 % 4)
> > +		pipe_clip.y2 = ((pipe_clip.y2 / 4) + 1) * 4;
> > +
> which spec describes that we have to align by 4 for SelectiveUpdate?

"For PSR2 selective update, the frame is divided into blocks of four scan lines each. The update region must be expanded so it aligns to the 4 line
groups of transcoder vertical active."

BSpec 55229


> > +	/*
> > +	 * Now that we have the pipe damaged area check if it intersect
> > with
> > +	 * every plane, if it does set the plane selective fetch area.
> > +	 */
> > +	for_each_oldnew_intel_plane_in_state(state, plane,
> > old_plane_state,
> > +					     new_plane_state, i) {
> > +		struct drm_rect *sel_fetch_area, inter;
> > +
> > +		if (new_plane_state->uapi.crtc != crtc_state->uapi.crtc 
> > > > 
> > +		    !new_plane_state->uapi.visible)
> > +			continue;
> > +
> > +		inter = pipe_clip;
> > +		if (!drm_rect_intersect(&inter, &new_plane_state-
> > > uapi.dst))
> > +			continue;
> > +
> > +		sel_fetch_area = &new_plane_state->psr2_sel_fetch_area;
> > +		sel_fetch_area->y1 = inter.y1 - new_plane_state-
> > > uapi.dst.y1;
> > +		sel_fetch_area->y2 = inter.y2 - new_plane_state-
> > > uapi.dst.y1;
> struct drm_rect src;
> drm_rect_fp_to_int(&src, &new_plane_state->uapi.src);
> sel_fetch_area = &new_plane_state->psr2_sel_fetch_area;
> sel_fetch_area->x1 = src.x1;
> sel_fetch_area->x2 = src.x2;
> sel_fetch_area->y1 = inter.y1 - new_plane_state->uapi.dst.y1 + src.y1;
> sel_fetch_area->y2 = inter.y2 - new_plane_state->uapi.dst.y1 + src.y1;

PLANE_SEL_FETCH_POS needs to be programmed without the fb offset.

That is why we do the bellow in intel_psr2_program_plane_sel_fetch():

y = (plane_state->uapi.src.y1 >> 16) + clip->y1;
ret = skl_calc_main_surface_offset(plane_state, &x, &y, &offset);
if (ret)
	drm_warn_once(&dev_priv->drm, "skl_calc_main_surface_offset() returned %i\n",
		      ret);
val = y << 16 | x;
intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_OFFSET(pipe, plane->id),
			  val);
> >  	}
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > +skip_sel_fetch_set_loop:
> >  	psr2_man_trk_ctl_calc(crtc_state, &pipe_clip, full_update);
> >  	return 0;
> >  }

_______________________________________________
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