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

[Gwan-gyeong Mun]

On 9/13/21 7:45 PM, Souza, Jose wrote:
> 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 at ffwll.ch>
>>> Cc: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
>>> Signed-off-by: José Roberto de Souza <jose.souza at 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.
> 
ok then, the other changes look good to me.
Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
> 
>>> +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);
>>>
>>>    /*
>>>
>