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

["Souza, Jose" <jose.souza@intel.com>]

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@ffwll.ch>
> > 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 | 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);
> >   
> >   	/*
> >