[Intel-gfx] [PATCH v6 2/5] drm/i915/display/psr: Use plane damage clips to calculate damaged area
Mun, Gwan-gyeong
gwan-gyeong.mun at intel.com
Thu Dec 17 16:04:34 UTC 2020
On Thu, 2020-12-17 at 05:34 -0800, Souza, Jose wrote:
> On Thu, 2020-12-17 at 11:51 +0000, Mun, Gwan-gyeong wrote:
> > On Wed, 2020-12-16 at 06:40 -0800, Souza, Jose wrote:
> > > On Wed, 2020-12-16 at 14:10 +0000, Mun, Gwan-gyeong wrote:
> > > > On Wed, 2020-12-16 at 05:17 -0800, Souza, Jose wrote:
> > > > > 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 at linux.intel.com>
> > > > > > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
> > > > > > > > > Signed-off-by: José Roberto de Souza <
> > > > > > > > > jose.souza at intel.com>
> > > > > > > > > ---
> > > > > > > > > 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_st
> > > > > > > > > ate-
> > > > > > > > > > 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.
> > > > >
> > > > easist way to get damaged area for crtc is just using uapi.dst
> > > > but because fb damage clip follows uapi.src coordinates, we
> > > > have to
> > > > translate fb damage clip's coordinates to dst coordinates for
> > > > accumulating crtc damage region.
> > > >
> > > > therefore can you explain what is the exact burden of using
> > > > uapi.dst
> > > > for calculating crtc damage area?
> > > >
> > >
> > > I hope this ASCI drawing do not break :P
> > >
> > > > ----------------------|
> > > > |
> > > > Pipe area |
> > > > ----------------| |
> > > > whole plane A | |
> > > > |----------| |
> > > > | src | |
> > > > | visible | |
> > > ------------------------
> > >
> > > Using dst would add the whole plane A area to the pipe damaged
> > > area
> > > while using src will only add the visible area.
> > >
> >
> > if you expect to be shown the src visible area to pipe area,
> > (that means the dst area has the same width and height, but the x1,
> > y1
> > will be based on crtc coordinates.)
> > in the crte point of view, the src visible area is the dst area.
> > (but because the src area use fb based coordinates, we should use
> > dst)
> >
> > and if you are expecting handling obscured area by other planes,
> > (for
> > optimizing)
> > we should use dst too.
> >
> > we only need to use src for handling fb damaged clip rects.
> > but after accumulating the fb damaged clip rects, it also has to be
> > translated to crtc coordinates from fb (src ) coordinates.
>
> That is being done, src are then translated to CRTC coordinates using
> dst but only using dst is not a optimized solution.
>
>
> damaged_area.y1 = old_plane_state->uapi.src.y1 >> 16;
> damaged_area.y1 = old_plane_state->uapi.src.y2 >> 16;
why do you save it to damaged_area.y1 twice?
> damaged_area.y1 += old_plane_state->uapi.dst.y1;
> damaged_area.y2 += old_plane_state->uapi.dst.y1;
and if we just want to accumulate pipe_clip for the move or visibility.
we should just use dst. (if src.y1 is over 0, your result leads to
wrong geometry.)
> clip_area_update(&pipe_clip, &damaged_area);
>
> num_clips = 0;
> damaged_area.y1 = src.y1;
> damaged_area.y2 = src.y2;
> ...
>
> 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);
>
>
>
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > > > > > 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.
> > > > >
> > > > on the link's 2nd page,
> > > > we only need a red Selective Fetch area, but your patch seems
> > > > Selective
> > > > Fetch area as green rect.
> > > > https://docs.google.com/presentation/d/1gOKE4JnC97RzRg15ZM8IaQzHnifIxwvwP_UbOoGFE9E/edit?usp=sharing
> > >
> > > Okay, now I got it.
> > > Will update this but please take a look to the other answers so
> > > we do
> > > only one more version.
> > >
> > >
> > > > > > > > }
> > > > > > > >
> > > > > > > > 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_st
> > > > > > > > > ate-
> > > > > > > > > > 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.
> > > > >
> > > > >
> > > > here is the problematic case,
> > > > if we assume src indicates 50 x 50 +10 +10
> > > > ,fb damage rect indicates 5 x 5 +55 +55
> > > > and dst indicates 50 x 50 +100 +100
> > > >
> > > > 1) if whole plane src area needs to be updated
> > > > inter: 50 x 50 +100 +100
> > > > sel_fetch_area->y1 = 100 - 100 => 0
> > > > sel_fetch_area->y2 = 100 - 100 => 0
> > > >
> > > >
> > > > drm_rect_intersect(sel_fetch_area, &src);
> > > > => sel_fetch_area does not fit whole src area.
> > >
> > > Your values are wrong.
> > >
> > I used DRM_RECT_FMT style.
> >
> > #define DRM_RECT_FMT "%dx%d%+d%+d"
> > #define DRM_RECT_ARG(r) drm_rect_width(r), drm_rect_height(r), (r)-
> > >x1,
> > (r)->y1
> >
> > > # 50 x 50 +10 +10
> > >
> > > src.x1 = 50
> > > src.y1 = 50
> > > src.x2 = 60
> > > src.y2 = 60
> > >
> > src.x1 = 10
> > src.y1 = 10
> > src.x2 = 60
> > src.y2 = 60
>
> If plane dst has 50px of width and height the src above is not
> possible.
if src width and height has 50x50, the dst can have 50 x 50 too.
> Adjusting it to:
>
> src.x2 = 50
> src.y2 = 50
>
> src.x1 = 10
> src.y1 = 10
> src.x2 = 50
src.x2 = 60
> src.y2 = 50
src.y2 = 60
>
> damaged.x1 = 55
> damaged.y1 = 55
> damaged.x2 = 60
> damaged.y2 = 60
>
> dst.x1 = 100
> dst.y1 = 100
> dst.x2 = 150
> dst.y2 = 150
>
> 1) if whole plane src are needs to be updated
> pipe_clip.y1 = src.y1 + dst.y1 = 110
> pipe_clip.y2 = src.y2 + dst.y1 = 150
pipe_clip.y2 = src.y2 + dst.y1 = 160
>
> # drm_rect_intersect(&inter, &new_plane_state->uapi.dst)
> iter.y1 = 110
> iter.y2 = 150
>
inter = pipe_clip;
pipe_clip.x1 = 0
pipe_clip.y1 = 110
=> in the crtc point of view, the pipe_clip.y1 shoule be 100.
but the middle of value is wrong.
pipe_clip.x2 = INT_MAX
pipe_clip.y2 = 160
=> in the crtc point of view, the pipe_clip.y1 shoule be 150.
but the
middle of value is wrong.
dst.x1 = 100
dst.y1 = 100
dst.x2 = 150
dst.y2 = 150
drm_rect_intersect(&inter, &new_plane_state->uapi.dst)
=>
iter.x1 = 100
iter.y1 = 110
iter.x2 = 150
iter.y2 = 150
because we only add one dst,
the inter should be same to dst.
but the result of iter does not have same geometry to dst.
wrong pipe_clip region leads wrong SelectiveUpdate region.
> # sel_fetch_area->y1 = inter.y1 - new_plane_state->uapi.dst.y1;
> sel_fetch_area.y1 = 110 - 100 = 10
> sel_fetch_area.y2 = 150 - 100 = 50
> drm_rect_intersect(sel_fetch_area, &src);
> sel_fetch_area.y1 = 10
> sel_fetch_area.y2 = 50
>
> > > # 5 x 5 +55 +55. * not used in example 1
> > > damaged.x1 = 5
> > > damaged.y1 = 5
> > > damaged.x2 = 60
> > > damaged.y2 = 60
> > >
> > damaged.x1 = 55
> > damaged.y1 = 55
> > damaged.x2 = 60
> > damaged.y2 = 60
> >
> > > # 50 x 50 +100 +100
> > > dst.x1 = 50
> > > dst.y1 = 50
> > > dst.x2 = 150
> > > dst.y2 = 150
> > >
> > dst.x1 = 100
> > dst.y1 = 100
> > dst.x2 = 150
> > dst.y2 = 150
> >
> > therefore below result seems to wrong result.
> >
> > > ####
> > >
> > > 1) if whole plane src are needs to be updated
> > > pipe_clip.x1 = src.x1 + dst.x1 = 100
> > > pipe_clip.y2 = src.y1 + dst.y1 = 100
> > > pipe_clip.x2 = src.x2 + dst.y1 = 110
> > > pipe_clip.y2 = src.y2 + dst.y2 = 110
> > >
> > > # drm_rect_intersect(&inter, &new_plane_state->uapi.dst)
> > > iter.x1 = 100
> > > iter.y1 = 100
> > > iter.x2 = 110
> > > iter.y2 = 110
> > >
> > > # sel_fetch_area->y1 = inter.y1 - new_plane_state->uapi.dst.y1;
> > > sel_fetch_area.y1 = 100 - 50 = 50
> > > sel_fetch_area.y2 = 110 - 50 = 60
> > >
> > > drm_rect_intersect(sel_fetch_area, &src);
> > >
> > > sel_fetch_area.y1 = 50
> > > sel_fetch_area.y2 = 60
> > >
> > > > 2) if only the plane fb damage needs to be updated
> > > > translate fb damage's coordinates to crtc coordinates
> > > > 5 x 5 +55 +55 => 5 x 5 +105 +105
> > > > inter: 5 x 5 +105 +105
> > > > sel_fetch_area->y1 = 105 - 100 => 5
> > > > sel_fetch_area->y2 = 105 - 100 => 5
> > > >
> > > > drm_rect_intersect(sel_fetch_area, &src);
> > > > => sel_fetch_area does not fit to src's damage area.
> > >
> > > Let me know if this case is broken after use the right values.
> > >
> > > > > > > > > + 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;
> > > > > > > > > }
More information about the Intel-gfx
mailing list