| Message ID | 20201217211400.231826-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7,1/5] drm: Add function to convert rect in 16.16 fixed format to regular format | expand |
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@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 | 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? > + > 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); > 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? > .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); 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, > + 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. > + 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? > + /* > + * 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; > } > > +skip_sel_fetch_set_loop: > psr2_man_trk_ctl_calc(crtc_state, &pipe_clip, full_update); > return 0; > }
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@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 | 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; > > }
On Fri, 2020-12-18 at 08:52 -0800, José Roberto de Souza wrote: > 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@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 | 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. Okay, the dst.y1 = dst.y2 = 0 of the invisible state will cause the damaged_area.y1 to always be 0. New version being send in a few minutes > > > 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; > > > } >
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); + 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); 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, .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; + } 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; + } + + drm_rect_fp_to_int(&src, &new_plane_state->uapi.src); + damaged_clips = drm_plane_get_damage_clips(&new_plane_state->uapi); + + 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; + + /* + * 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; } +skip_sel_fetch_set_loop: psr2_man_trk_ctl_calc(crtc_state, &pipe_clip, full_update); return 0; }
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 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@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 | 105 ++++++++++++++++++++--- 1 file changed, 91 insertions(+), 14 deletions(-)