| Message ID | 20210909230725.33735-4-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/i915/display/adlp: Fix PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR calculation | expand |
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); > > /* >
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); > > > > /* > >
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@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. > ok then, the other changes look good to me. Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@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); >>> >>> /* >>> >
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); + 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); /*
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(-)