On Tue, 2020-12-01 at 19:40 +0000, Mun, Gwan-gyeong wrote:
> On Tue, 2020-12-01 at 09:39 -0800, Souza, Jose wrote:
> > 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@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 | 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?
> >
> I drew the example here :
> https://docs.google.com/presentation/d/1MI20q_3GublGYsY2TDheRncOQsbIjMn20aQ99loaoFg/edit?usp=sharing
Thanks so much for the drawing, now I got what you were talking about.
This point is not very clear in BSpec but you are right we need to set plane selective fetch area using the whole area damaged in pipe.
Can you take a look at the last patch in this branch? https://github.com/zehortigoza/linux/tree/psr2-sel-fetch-part3-v4
Will do some testing and squash into the right commits and resend in the next couple of the days.
>
> >
> > > (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@xxxxxxxxxxxxxxxxxxxxx
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
