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.
>
> 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)
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.
> }
>
> 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)
> > + 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
