From: "Souza, Jose" <jose.souza@intel.com>
To: "Mun, Gwan-gyeong" <gwan-gyeong.mun@intel.com>,
"intel-gfx@lists.freedesktop.org"
<intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 2/6] drm/i915/display/psr: Use plane damage clips to calculate damaged area
Date: Tue, 1 Dec 2020 21:15:14 +0000 [thread overview]
Message-ID: <08cdecd9a91e2b71d1adb34ef7c034b7e9518c29.camel@intel.com> (raw)
In-Reply-To: <958e544743e7c05f123704512e32d0f3ece306f8.camel@intel.com>
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@linux.intel.com>
> > > > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@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?
> >
> 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@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-12-01 21:15 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-13 23:01 [Intel-gfx] [PATCH 1/6] drm/i915/display/psr: Calculate selective fetch plane registers José Roberto de Souza
2020-10-13 23:01 ` [Intel-gfx] [PATCH 2/6] drm/i915/display/psr: Use plane damage clips to calculate damaged area José Roberto de Souza
2020-10-26 21:40 ` Mun, Gwan-gyeong
2020-10-27 1:04 ` Souza, Jose
2020-10-27 20:12 ` Souza, Jose
2020-12-01 17:26 ` Mun, Gwan-gyeong
2020-12-01 17:39 ` Souza, Jose
2020-12-01 19:40 ` Mun, Gwan-gyeong
2020-12-01 21:15 ` Souza, Jose [this message]
2020-12-02 10:22 ` Mun, Gwan-gyeong
2020-10-13 23:01 ` [Intel-gfx] [PATCH 3/6] drm/i915/display/psr: Consider other planes to damaged area calculation José Roberto de Souza
2020-10-27 13:34 ` Mun, Gwan-gyeong
2020-10-27 17:25 ` Souza, Jose
2020-12-01 17:33 ` Mun, Gwan-gyeong
2020-12-01 17:44 ` Souza, Jose
2020-12-01 19:44 ` Mun, Gwan-gyeong
2020-10-13 23:01 ` [Intel-gfx] [PATCH 4/6] drm/i915/display: Split and export main surface calculation from skl_check_main_surface() José Roberto de Souza
2020-10-13 23:01 ` [Intel-gfx] [PATCH 5/6] RFC/WIP: drm/i915/display/psr: Consider tiling when doing the plane offset calculation José Roberto de Souza
2020-10-13 23:01 ` [Intel-gfx] [PATCH 6/6] DEBUG: drm/i915/display: Add debug information to selective fetch José Roberto de Souza
2020-10-13 23:05 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/6] drm/i915/display/psr: Calculate selective fetch plane registers Patchwork
2020-10-13 23:06 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2020-10-13 23:31 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-10-14 15:36 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2020-10-26 21:13 ` [Intel-gfx] [PATCH 1/6] " Mun, Gwan-gyeong
2020-10-27 0:24 ` Souza, Jose
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=08cdecd9a91e2b71d1adb34ef7c034b7e9518c29.camel@intel.com \
--to=jose.souza@intel.com \
--cc=gwan-gyeong.mun@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).