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
