[Intel-gfx] [PATCH 2/6] drm/i915/display/psr: Use plane damage clips to calculate damaged area
Souza, Jose
jose.souza at intel.com
Tue Dec 1 17:39:05 UTC 2020
On Tue, 2020-12-01 at 17:26 +0000, Mun, Gwan-gyeong wrote:
> On Tue, 2020-10-27 at 13:12 -0700, Souza, Jose wrote:
> > On Tue, 2020-10-27 at 01:04 +0000, Souza, Jose wrote:
> > > On Mon, 2020-10-26 at 21:40 +0000, Mun, Gwan-gyeong wrote:
> > > > On Tue, 2020-10-13 at 16:01 -0700, 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.
> > > > >
> > > > > 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 | 54
> > > > > +++++++++++++++++++++-
> > > > > --
> > > > > 1 file changed, 49 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > index 773a5b5fa078..0f1e9f0fa57f 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > @@ -1273,6 +1273,9 @@ int intel_psr2_sel_fetch_update(struct
> > > > > intel_atomic_state *state,
> > > > > 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_mode_rect *damaged_clips;
> > > > > + u32 num_clips;
> > > > > + int j;
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > if (new_plane_state->uapi.crtc != crtc_state-
> > > > > > uapi.crtc)
> > > > > continue;
> selective fetch area also can be affected by Selective Updated region.
> therefore Selective Updated region rect should be calculated first and
> then the plane's selective fetch area should be applied (intersected by
> calculated SU.)
> please check this implementation.
Why selective update region needs to be calculate first if it should be based on the plane damage areas/selective fetch areas?
Can you give a example where it gives not the expected result?
> (https://patchwork.freedesktop.org/patch/404264/?series=84340&rev=1)
> > > > > @@ -1291,13 +1294,54 @@ int intel_psr2_sel_fetch_update(struct
> > > > > intel_atomic_state *state,
> > > > > if (!new_plane_state->uapi.visible)
> > > > > continue;
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > + sel_fetch_area = &new_plane_state-
> > > > > > psr2_sel_fetch_area;
> > > > > + sel_fetch_area->y1 = -1;
> > > > > +
> > > > > + 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 plane moved, mark the whole plane area as
> > > > > damaged
> > > > > as it
> > > > > + * needs to be complete redraw in the new
> > > > > 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 (!drm_rect_equals(&new_plane_state-
> > > > > > uapi.dst,
> > > > > + &old_plane_state-
> > > > > > uapi.dst)) {
> > > > > + num_clips = 0;
> > > > > + sel_fetch_area->y1 = new_plane_state-
> > > > > > uapi.src.y1 >> 16;
> > > > > + sel_fetch_area->y2 = new_plane_state-
> > > > > > uapi.src.y2 >> 16;
> > > > > + } 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.
> > > > > + */
> > > > > + sel_fetch_area->y1 = new_plane_state-
> > > > > > uapi.src.y1 >> 16;
> > > > > + sel_fetch_area->y2 = new_plane_state-
> > > > > > uapi.src.y2 >> 16;
> > > > > + }
> > > > > +
> > > > why don't you use drm_atomic_helper_damage_merged() function
> > > > here?
> > >
> > > hum did not knew about this function, will take a look at as it
> > > does more than just merge all damaged areas.
> >
> > We can't use this function as it marks the whole plane area as
> > damaged if there is no damaged clips.
> > But for our use case this is bad as we add all planes of the
> > pipe/CRTC to the state, so it would cause a full fetch of the planes
> > without any
> > flip/modification.
> >
> > > > > + for (j = 0; j < num_clips; j++) {
> > > > > + struct drm_rect damage_area;
> > > > > +
> > > > > + damage_area.y1 = damaged_clips[j].y1;
> > > > > + damage_area.y2 = damaged_clips[j].y2;
> > > > > + clip_area_update(sel_fetch_area,
> > > > > &damage_area);
> > > > > + }
> > > > > +
> > > > > + /*
> > > > > + * No page flip, no plane moviment or no damage
> > > > > areas,
> > > > > so don't
> > > > typo (moviment -> movement)
> > >
> > > fixed
> > >
> > > > > + * fetch any pixel from memory for this plane
> > > > > + */
> > > > > + if (sel_fetch_area->y1 == -1) {
> > > > > + sel_fetch_area->y1 = 0;
> > > > > + sel_fetch_area->y2 = 0;
> > > > > + }
> > > > > +
> > > > > + /* Don't need to redraw plane damaged areas
> > > > > outside of
> > > > > screen */
> > > > > + j = sel_fetch_area->y2 + (new_plane_state-
> > > > > > uapi.dst.y1
> > > > > > > 16);
> > > > src coordinates of the drm_plane_state are 16.16 fixed point but
> > > > dst
> > > > coordinates are not 16.16 fixed point.
> > > > therefore we don't need to bit shift for dst.
> > > > Because the sel_fetch_area seems based on src coordinates, in
> > > > order to
> > > > apply to dst coordinates here, it requires coordinate
> > > > calculation.
> > >
> > > thanks for catching this, also fixed the same issue patch 1.
> > >
> > > > > + j = crtc_state-
> > > > > > uapi.adjusted_mode.crtc_vdisplay - j;
> > > > > + if (j < 0)
> > > > > + sel_fetch_area->y2 += j;
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > temp = *sel_fetch_area;
> > > > > temp.y1 += new_plane_state->uapi.dst.y1 >> 16;
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list