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@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.
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;
> > > }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
