intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: "Souza, Jose" <jose.souza@intel.com>,
	 Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH CI 2/4] drm/i915/display/psr: Use plane damage clips to calculate damaged area
Date: Wed, 21 Jul 2021 09:56:32 +0200	[thread overview]
Message-ID: <CAKMK7uHHKXVWhtX7Guguir6QuiPT9L_Pgd7+qjocEFnp+y5z-w@mail.gmail.com> (raw)
In-Reply-To: <CAKMK7uG3B2-EVp6hkeaeRuZfOQZFAE4yYRFS7FzVHQw8p5ssxg@mail.gmail.com>

On Wed, Jul 21, 2021 at 9:50 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Jul 20, 2021 at 6:55 PM Souza, Jose <jose.souza@intel.com> wrote:
> > On Tue, 2021-07-20 at 17:31 +0200, Daniel Vetter wrote:
> > > On Tue, Jul 20, 2021 at 5:16 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > On Tue, Jul 20, 2021 at 5:09 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > On Mon, Jan 4, 2021 at 9:56 PM José Roberto de Souza
> > > > > <jose.souza@intel.com> 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.
> > > > > >
> > > > > > v2:
> > > > > > - do not shifting new_plane_state->uapi.dst only src is in 16.16 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
> > > > > >
> > > > > > v7:
> > > > > > - intersec every damage clip with src to minimize damaged area
> > > > > >
> > > > > > v8:
> > > > > > - adjust pipe_damaged area to 4 lines grouping
> > > > > > - adjust calculation now that is understood that uapi.src is the
> > > > > > framebuffer coordinates that plane will start to fetch from
> > > > > >
> > > > > > v9:
> > > > > > - Only add plane dst or src to damaged_area if visible
> > > > > > - Early skip plane damage calculation if it was not visible in old and
> > > > > > new state
> > > > > >
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > > > > > Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > >
> > > > > Why is this not using drm_atomic_helper_damage_merged? I just stumbled
> > > > > over this, and this is one of the only two drivers that directly digs
> > > > > around in the damage area, and seems to reinvent a bunch of the stuff
> > > > > here.
> >
> > We can use drm_atomic_helper_damage_merged() but it would only save us one for loop.
>
> Yes please. The trouble with rolling our own copies for everything is
> that it does add up.
>
> > > > Also, did we merge the igts for this stuff? They unfortunately never
> > > > landed, when vmwgfx team did all this work, but for i915 we really
> > > > shouldn't even land new support without tests.
> > >
> > > Lo and behold, we merge the uapi enabling way earlier than this patch here:
> > >
> > > commit 093a3a30000926b8bda9eef773e4ed5079053350
> > > Author: José Roberto de Souza <jose.souza@intel.com>
> > > Date:   Thu Jun 25 18:01:47 2020 -0700
> > >
> > >    drm/i915: Add plane damage clips property
> > >
> > > And the igts are nowhere near to be seen, at least the stuff from
> > > vmwgfx didn't land. Please file a JIRA internally and ping me on that
> > > so this gets sorted out asap.
> >
> > Here the IGT: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_psr2_sf.c
>
> There was some igts that were cross-driver, not intel specific. The
> thing here is supposed to be a cross-vendor interface, so would be
> really good if you resurrect those from vmwgfx folks and land them
> too:
>
> https://patchwork.freedesktop.org/series/51087/

Also from a very quick look at the kms_psr2_sf tests there's really
not a whole lot specific about our psr2 implementation in there. The
test should work on any plane with a FB_DAMAGE_CLIPS rect available,
so there's no reason to make this specific to intel, much less to
psr2. I think we need to:
- make this a generic kms test, it should work
- merge with the testcases from the folks who merged FB_DAMAGE_CLIPS
originally to make sure all drivers follow the same contract.

KMS properties are generic, intel-specific tests for when there's
nothing intel-specific isn't good. Also adding Pankaj.

Cheers, Daniel

> >
> > >
> > > Thanks, Daniel
> > >
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_psr.c | 113 ++++++++++++++++++++---
> > > > > >  1 file changed, 99 insertions(+), 14 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > index d9a395c486d3..f5b9519b3756 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > @@ -1242,9 +1242,11 @@ static void psr2_man_trk_ctl_calc(struct intel_crtc_state *crtc_state,
> > > > > >         if (clip->y1 == -1)
> > > > > >                 goto exit;
> > > > > >
> > > > > > +       drm_WARN_ON(crtc_state->uapi.crtc->dev, clip->y1 % 4 || clip->y2 % 4);
> > > > > > +
> > > > > >         val |= PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE;
> > > > > >         val |= PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(clip->y1 / 4 + 1);
> > > > > > -       val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(DIV_ROUND_UP(clip->y2, 4) + 1);
> > > > > > +       val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2 / 4 + 1);
> > > > > >  exit:
> > > > > >         crtc_state->psr2_man_track_ctl = val;
> > > > > >  }
> > > > > > @@ -1269,8 +1271,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,13 +1284,25 @@ 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 = { .y1 = -1 };
> > > > > > +               struct drm_mode_rect *damaged_clips;
> > > > > > +               u32 num_clips, j;
> > > > > >
> > > > > >                 if (new_plane_state->uapi.crtc != crtc_state->uapi.crtc)
> > > > > >                         continue;
> > > > > >
> > > > > > +               if (!new_plane_state->uapi.visible &&
> > > > > > +                   !old_plane_state->uapi.visible)
> > > > > > +                       continue;
> > > > > > +
> > > > > >                 /*
> > > > > >                  * TODO: Not clear how to handle planes with negative position,
> > > > > >                  * also planes are not updated if they have a negative X
> > > > > > @@ -1300,23 +1314,94 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
> > > > > >                         break;
> > > > > >                 }
> > > > > >
> > > > > > -               if (!new_plane_state->uapi.visible)
> > > > > > -                       continue;
> > > > > > +               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.
> > > > > >                  */
> > > > > > -               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 (new_plane_state->uapi.visible != old_plane_state->uapi.visible ||
> > > > > > +                   !drm_rect_equals(&new_plane_state->uapi.dst,
> > > > > > +                                    &old_plane_state->uapi.dst)) {
> > > > > > +                       if (old_plane_state->uapi.visible) {
> > > > > > +                               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);
> > > > > > +                       }
> > > > > > +
> > > > > > +                       if (new_plane_state->uapi.visible) {
> > > > > > +                               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;
> > > > > > +               } else if (new_plane_state->uapi.alpha != old_plane_state->uapi.alpha ||
> > > > > > +                          (!num_clips &&
> > > > > > +                           new_plane_state->uapi.fb != old_plane_state->uapi.fb)) {
> > > > > > +                       /*
> > > > > > +                        * If the plane don't have damaged areas but the
> > > > > > +                        * framebuffer changed or alpha 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;
> > > > > > +               }
> > > > > > +
> > > > > > +               drm_rect_fp_to_int(&src, &new_plane_state->uapi.src);
> > > > > > +               damaged_clips = drm_plane_get_damage_clips(&new_plane_state->uapi);
> > > > > > +
> > > > > > +               for (j = 0; j < num_clips; j++) {
> > > > > > +                       struct drm_rect clip;
> > > > > > +
> > > > > > +                       clip.x1 = damaged_clips[j].x1;
> > > > > > +                       clip.y1 = damaged_clips[j].y1;
> > > > > > +                       clip.x2 = damaged_clips[j].x2;
> > > > > > +                       clip.y2 = damaged_clips[j].y2;
> > > > > > +                       if (drm_rect_intersect(&clip, &src))
> > > > > > +                               clip_area_update(&damaged_area, &clip);
> > > > > > +               }
> > > > > >
> > > > > > -               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);
> > > > > > +               if (damaged_area.y1 == -1)
> > > > > > +                       continue;
> > > > > > +
> > > > > > +               damaged_area.y1 += new_plane_state->uapi.dst.y1 - src.y1;
> > > > > > +               damaged_area.y2 += new_plane_state->uapi.dst.y1 - src.y1;
> > > > > > +               clip_area_update(&pipe_clip, &damaged_area);
> > > > > > +       }
> > > > > > +
> > > > > > +       if (full_update)
> > > > > > +               goto skip_sel_fetch_set_loop;
> > > > > > +
> > > > > > +       /* It must be aligned to 4 lines */
> > > > > > +       pipe_clip.y1 -= pipe_clip.y1 % 4;
> > > > > > +       if (pipe_clip.y2 % 4)
> > > > > > +               pipe_clip.y2 = ((pipe_clip.y2 / 4) + 1) * 4;
> > > > > > +
> > > > > > +       /*
> > > > > > +        * 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;
> > > > > > +
> > > > > > +               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;
> > > > > > +
> > > > > > +               sel_fetch_area = &new_plane_state->psr2_sel_fetch_area;
> > > > > > +               sel_fetch_area->y1 = inter.y1 - new_plane_state->uapi.dst.y1;
> > > > > > +               sel_fetch_area->y2 = inter.y2 - new_plane_state->uapi.dst.y1;
> > > > > >         }
> > > > > >
> > > > > > +skip_sel_fetch_set_loop:
> > > > > >         psr2_man_trk_ctl_calc(crtc_state, &pipe_clip, full_update);
> > > > > >         return 0;
> > > > > >  }
> > > > > > --
> > > > > > 2.30.0
> > > > > >
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > > >
> > > >
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > >
> > >
> > >
> >
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2021-07-21  7:56 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-04 20:56 [Intel-gfx] [PATCH CI 1/4] drm: Add function to convert rect in 16.16 fixed format to regular format José Roberto de Souza
2021-01-04 20:56 ` [Intel-gfx] [PATCH CI 2/4] drm/i915/display/psr: Use plane damage clips to calculate damaged area José Roberto de Souza
2021-07-20 15:09   ` Daniel Vetter
2021-07-20 15:16     ` Daniel Vetter
2021-07-20 15:31       ` Daniel Vetter
2021-07-20 16:55         ` Souza, Jose
2021-07-21  7:50           ` Daniel Vetter
2021-07-21  7:56             ` Daniel Vetter [this message]
2021-07-21  8:20               ` Daniel Vetter
2021-07-21 19:42               ` Souza, Jose
2021-07-22 12:19                 ` Daniel Vetter
2021-01-04 20:56 ` [Intel-gfx] [PATCH CI 3/4] drm/i915/display: Split and export main surface calculation from skl_check_main_surface() José Roberto de Souza
2021-01-04 20:56 ` [Intel-gfx] [PATCH CI 4/4] drm/i915/display/psr: Program plane's calculated offset to plane SF register José Roberto de Souza
2021-01-04 22:27 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [CI,1/4] drm: Add function to convert rect in 16.16 fixed format to regular format Patchwork
2021-01-04 22:54 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2021-01-05  1:04 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2021-01-05 13:29   ` 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=CAKMK7uHHKXVWhtX7Guguir6QuiPT9L_Pgd7+qjocEFnp+y5z-w@mail.gmail.com \
    --to=daniel@ffwll.ch \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=jose.souza@intel.com \
    --cc=pankaj.laxminarayan.bharadiya@intel.com \
    /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).