Re: [Intel-gfx] [PATCH CI 2/4] drm/i915/display/psr: Use plane damage clips to calculate damaged area

[Daniel Vetter <daniel@ffwll.ch>]

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.
-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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx