On Wed, 2020-12-16 at 10:29 +0000, Mun, Gwan-gyeong wrote:
> On Tue, 2020-12-15 at 05:14 -0800, Souza, Jose wrote:
> > On Tue, 2020-12-15 at 13:02 +0000, Mun, Gwan-gyeong wrote:
> > > On Mon, 2020-12-14 at 09:49 -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.
> > > >
> > > Hi,
> > > > v2:
> > > > - do not shifthing new_plane_state->uapi.dst only src is in
> > > > 16.16
> > > Typo on commit message. 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
> > > >
> > > > 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 | 98
> > > > +++++++++++++++++++++-
> > > > --
> > > > 1 file changed, 86 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index d9a395c486d3..7daed098fa74 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -1269,8 +1269,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 +1282,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 = { .x1 = 0, .y1 =
> > > > -1, .x2 = INT_MAX, .y2 = -1 };
> > > > + struct drm_mode_rect *damaged_clips;
> > > > + u32 num_clips, j;
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > if (new_plane_state->uapi.crtc != crtc_state-
> > > > > uapi.crtc)
> > > > continue;
> > > > @@ -1300,23 +1308,89 @@ int intel_psr2_sel_fetch_update(struct
> > > > intel_atomic_state *state,
> > > > break;
> > > > }
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > - if (!new_plane_state->uapi.visible)
> > > > - continue;
> > > > + drm_rect_convert_16_16_to_regular(&new_plane_state-
> > > > > uapi.src, &src);
> > > > + 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 visibility or plane moved, mark the whole plane
> > > > area as
> > > > + * damaged as it needs to be complete redraw in the new
> > > > and old
> > > > + * position.
> > > > */
> > > > + 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.src.y1
> > > > > > 16;
> > > > + damaged_area.y1 = old_plane_state->uapi.src.y2
> > > > > > 16;
> > > > + damaged_area.y1 += old_plane_state-
> > > > > uapi.dst.y1;
> > > > + damaged_area.y2 += old_plane_state-
> > > > > uapi.dst.y1;
> > > > + clip_area_update(&pipe_clip, &damaged_area);
> > > > +
> > > > + num_clips = 0;
> > > > + damaged_area.y1 = src.y1;
> > > > + damaged_area.y2 = src.y2;
> > > 1. Visibility change case
> > > The pipe damaged area (The Selective Update region) needs to
> > > accumulate being visible plane's dst between old and new plane's
> > > dst.
> > >
> > > if you are implementing this scenario, the code shoule be like
> > > this,
> > >
> > > 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;
> > > }
> >
> > The current code does exactly above but discards the clipped in
> > uapi.src.
> >
> in order to accumlate the pipe damage area, we don't need to use and
> calculate src coordinates on move and visibility change scenarios.
> we can easily accumalates than additional calculation.
Using uapi.dst will cause additional and unnecessary area be included to the pipe damaged area, imagine that a plane have src.x1 = 50, src.y1 = 100,
... the plane invisible/cropped area don't need to be added to the pipe damaged area.
> > > 2. Move case
> > > The pipe damaged area (The Selective Update region) needs to
> > > accumulate both old and new plane's dst
> >
> > same
> >
> > > if you are implementing this scenario, the code shoule be like
> > > this,
> > >
> > > else if (!drm_rect_equals(&new_plane_state->uapi.dst,
> > > &old_plane_state-
> > > > uapi.dst)) {
> > > 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);
> > >
> > > 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 (new_plane_state->uapi.alpha !=
> > > > old_plane_state->uapi.alpha) {
> > > > + num_clips = 0;
> > > > + damaged_area.y1 = src.y1;
> > > > + damaged_area.y2 = src.y2;
> > > 3. alpha has changed ( because alpha handling section is not
> > > optimized,
> > > it can be treated with the move section)
> >
> Yes, you are right, there was a misunderstanding of the alpha scenario
> of me.
> > no need to handle like this for alpha, if the plane moved if will be
> > handled in the "if" above with or without alpha change, if it did not
> > moved but
> > alpha change no need to add the old state coordinate to the pipe
> > clip.
> >
> > > else if (new_plane_state->uapi.alpha != old_plane_state-
> > > > uapi.alpha) {
> > > 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);
> > >
> > > 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 (!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.
> > > > + */
> > > > + damaged_area.y1 = src.y1;
> > > > + damaged_area.y2 = src.y2;
> > > > + }
> > > > +
> > > 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.
> > > */
> > > 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;
> > > }
> > > > + for (j = 0; j < num_clips; j++) {
> > > > + struct drm_rect clip;
> > > > +
> > > > + clip.y1 = damaged_clips[j].y1;
> > > > + clip.y2 = damaged_clips[j].y2;
> > > > + clip_area_update(&damaged_area, &clip);
> > > > + }
> > > > +
> > > > + if (!drm_rect_intersect(&damaged_area, &src))
> > > > + continue;
> > > > +
> > > > + damaged_area.y1 += new_plane_state->uapi.dst.y1;
> > > > + damaged_area.y2 += new_plane_state->uapi.dst.y1;
> > > > + clip_area_update(&pipe_clip, &damaged_area);
> > > > + }
> > > > +
> > > else if (num_clips) {
> > > struct drm_rect plane_damaged_area;
> > > plane_damaged_area.x1 = src.x1;
> > > plane_damaged_area.x2 = src.x2;
> > > plane_damaged_area.y1 = 0;
> > > plane_damaged_area.y2 = 0;
> > >
> > > for (j = 0; j < num_clips; j++) {
> > > struct drm_rect clip;
> > >
> > > clip.x1 = damaged_clips[j].x1;
> > > clip.x2 = damaged_clips[j].x2;
> > > clip.y1 = damaged_clips[j].y1;
> > > clip.y2 = damaged_clips[j].y2;
> > >
> > > if (drm_rect_intersect(&clip, &src)) {
> > > clip_area_update(&plane_damaged_area, &clip);
> > > }
> >
> > Call intersect at every clip? that don't look optimized.
> >
> when the clip_area_update() is called it just accumulates the input
> rects to one output rect.
> it might lead to accumulating unneeded excessive update rect region
The excessive rect region will be removed once after sum all clips in the current approach.
> >
> > > }
> > >
> > > if (drm_rect_visible(plane_damaged_area)) {
> > > plane_damaged_area.y1 = plane_damaged_area.y1 - src.y1 +
> > > new_plane_state->uapi.dst.y1;
> > > plane_damaged_area.y2 = plane_damaged_area.y2 - src.y1 +
> > > new_plane_state->uapi.dst.y1;
> > >
> > > clip_area_update(&pipe_clip, &plane_damaged_area);
> > > continue;
> > > }
> > > }
> >
> > Current code sum all the damage clips, if the sum of the damage clips
> > insect with the src area it is translated to pipe coordinates and
> > called
> > clip_area_update() to update pipe_clip.
> >
> > > the calculation and translating cooridinates is alreay implemented
> > > here
> > > (
> > > https://patchwork.freedesktop.org/patch/404264/?series=84340&rev=1
> > > it has commetns which explains scenarios.)
> > > > + if (full_update)
> > > > + goto skip_sel_fetch_set_loop;
> > > > +
> > > > + /*
> > > > + * 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, src;
> > > > +
> > > > + 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;
> > > > +
> > > > + drm_rect_convert_16_16_to_regular(&new_plane_state-
> > > > > uapi.src, &src);
> > > > +
> > > > 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;
> > > > + sel_fetch_area->y1 = inter.y1 - new_plane_state-
> > > > > uapi.dst.y1;
> > > > + sel_fetch_area->y2 = inter.y2 - new_plane_state-
> > > > > uapi.dst.y1;
> > > 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;
> >
> > sel_fetch_area->y1 = inter.y1 - new_plane_state->uapi.dst.y1; +
> > drm_rect_intersect(sel_fetch_area, &src);
> > does a the same job and is easier to understand(translation + clip)
> >
> when the src's x1 and y1 indicates over than 0, does it work?
It will, we use it for scenarios like that in this function.
> > > > + sel_fetch_area->x1 = src.x1;
> > > > + sel_fetch_area->x2 = src.x2;
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > - 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);
> > > > + drm_rect_intersect(sel_fetch_area, &src);
> > > why this line is needed?
> >
> > explained above.
> >
> >
> > > > }
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > +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
