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

[Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>]

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?
> +		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);
>   
>   	/*
>