On Mon, 2021-09-13 at 19:03 +0300, Gwan-gyeong Mun wrote:
>
> On 9/10/21 2:07 AM, José Roberto de Souza wrote:
> > drm_atomic_helper_damage_iter_init() + drm_atomic_for_each_plane_damage()
> > returns the full plane area in case no damaged area was set by
> > userspace or it was discarted by driver.
> >
> > This is important to fix the rendering of userspace applications that
> > does frontbuffer rendering and notify driver about dirty areas but do
> > not set any dirty clips.
> >
> > With this we don't need to worry about to check and mark the whole
> > area as damaged in page flips.
> >
> > Another important change here is the move of
> > drm_atomic_add_affected_planes() call, it needs to called late
> > otherwise the area of all the planes would be added to pipe_clip and
> > not saving power.
> >
> > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > 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 | 37 +++++++++---------------
> > 1 file changed, 13 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 1a3effa3ce709..670b0ceba110f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -22,6 +22,7 @@
> > */
> >
> > #include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_damage_helper.h>
> >
> > #include "display/intel_dp.h"
> >
> > @@ -1577,10 +1578,6 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
> > if (!crtc_state->enable_psr2_sel_fetch)
> > return 0;
> >
> > - ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
> > - if (ret)
> > - return ret;
> > -
> > /*
> > * Calculate minimal selective fetch area of each plane and calculate
> > * the pipe damaged area.
> > @@ -1590,8 +1587,8 @@ 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 src, damaged_area = { .y1 = -1 };
> > - struct drm_mode_rect *damaged_clips;
> > - u32 num_clips, j;
> > + struct drm_atomic_helper_damage_iter iter;
> > + struct drm_rect clip;
> >
> > if (new_plane_state->uapi.crtc != crtc_state->uapi.crtc)
> > continue;
> > @@ -1611,8 +1608,6 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
> > break;
> > }
> >
> > - num_clips = drm_plane_get_damage_clips_count(&new_plane_state->uapi);
> > -
> > /*
> > * If visibility or plane moved, mark the whole plane area as
> > * damaged as it needs to be complete redraw in the new and old
> > @@ -1633,14 +1628,8 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
> > 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.
> > - */
> > + } else if (new_plane_state->uapi.alpha != old_plane_state->uapi.alpha) {
> > + /* If 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);
> > @@ -1648,15 +1637,11 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
> > }
> >
> > 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;
> > + drm_atomic_helper_damage_iter_init(&iter,
> > + &old_plane_state->uapi,
> > + &new_plane_state->uapi);
> In the description of the drm_atomic_helper_damage_iter_init() function
> says, in order to use drm_atomic_helper_damage_iter_init(), the driver
> requires that the drm_atomic_helper_check_plane_state() helper function
> should be called in advance.
> However, in i915, drm_atomic_helper_check_plane_state() helper is not
> used, and intel_atomic_plane_check_clipping() handles src.
> And i915 is not using the atomic_check callback of
> drm_plane_helper_funcs. Is it fine to use
> drm_atomic_helper_damage_iter_init() in this case as well?
intel_atomic_plane_check_clipping() does the src rect rotation, scale and clipping that drm_atomic_helper_check_plane_state() also do, so we are safe
here.
> > + drm_atomic_for_each_plane_damage(&iter, &clip) {
> > if (drm_rect_intersect(&clip, &src))
> > clip_area_update(&damaged_area, &clip);
> > }
> > @@ -1672,6 +1657,10 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
> > if (full_update)
> > goto skip_sel_fetch_set_loop;
> >
> > + ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
> > + if (ret)
> > + return ret;
> > +
> > intel_psr2_sel_fetch_pipe_alignment(crtc_state, &pipe_clip);
> >
> > /*
> >
