On Tue, Jul 20, 2021 at 6:55 PM Souza, Jose <jose.souza@xxxxxxxxx> wrote: > On Tue, 2021-07-20 at 17:31 +0200, Daniel Vetter wrote: > > On Tue, Jul 20, 2021 at 5:16 PM Daniel Vetter <daniel@xxxxxxxx> wrote: > > > On Tue, Jul 20, 2021 at 5:09 PM Daniel Vetter <daniel@xxxxxxxx> wrote: > > > > On Mon, Jan 4, 2021 at 9:56 PM José Roberto de Souza > > > > <jose.souza@xxxxxxxxx> wrote: > > > > > > > > > > Now using plane damage clips property to calcualte the damaged area. > > > > > Selective fetch only supports one region to be fetched so software > > > > > needs to calculate a bounding box around all damage clips. > > > > > > > > > > Now that we are not complete fetching each plane, there is another > > > > > loop needed as all the plane areas that intersect with the pipe > > > > > damaged area needs to be fetched from memory so the complete blending > > > > > of all planes can happen. > > > > > > > > > > v2: > > > > > - do not shifting new_plane_state->uapi.dst only src is in 16.16 format > > > > > > > > > > v4: > > > > > - setting plane selective fetch area using the whole pipe damage area > > > > > - mark the whole plane area damaged if plane visibility or alpha > > > > > changed > > > > > > > > > > v5: > > > > > - taking in consideration src.y1 in the damage coordinates > > > > > - adding to the pipe damaged area planes that were visible but are > > > > > invisible in the new state > > > > > > > > > > v6: > > > > > - consider old state plane coordinates when visibility changes or it > > > > > moved to calculate damaged area > > > > > - remove from damaged area the portion not in src clip > > > > > > > > > > v7: > > > > > - intersec every damage clip with src to minimize damaged area > > > > > > > > > > v8: > > > > > - adjust pipe_damaged area to 4 lines grouping > > > > > - adjust calculation now that is understood that uapi.src is the > > > > > framebuffer coordinates that plane will start to fetch from > > > > > > > > > > v9: > > > > > - Only add plane dst or src to damaged_area if visible > > > > > - Early skip plane damage calculation if it was not visible in old and > > > > > new state > > > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx> > > > > > Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx> > > > > > > > > Why is this not using drm_atomic_helper_damage_merged? I just stumbled > > > > over this, and this is one of the only two drivers that directly digs > > > > around in the damage area, and seems to reinvent a bunch of the stuff > > > > here. > > We can use drm_atomic_helper_damage_merged() but it would only save us one for loop. Yes please. The trouble with rolling our own copies for everything is that it does add up. > > > Also, did we merge the igts for this stuff? They unfortunately never > > > landed, when vmwgfx team did all this work, but for i915 we really > > > shouldn't even land new support without tests. > > > > Lo and behold, we merge the uapi enabling way earlier than this patch here: > > > > commit 093a3a30000926b8bda9eef773e4ed5079053350 > > Author: José Roberto de Souza <jose.souza@xxxxxxxxx> > > Date: Thu Jun 25 18:01:47 2020 -0700 > > > > drm/i915: Add plane damage clips property > > > > And the igts are nowhere near to be seen, at least the stuff from > > vmwgfx didn't land. Please file a JIRA internally and ping me on that > > so this gets sorted out asap. > > Here the IGT: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_psr2_sf.c There was some igts that were cross-driver, not intel specific. The thing here is supposed to be a cross-vendor interface, so would be really good if you resurrect those from vmwgfx folks and land them too: https://patchwork.freedesktop.org/series/51087/ Cheers, Daniel > > > > > Thanks, Daniel > > > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_psr.c | 113 ++++++++++++++++++++--- > > > > > 1 file changed, 99 insertions(+), 14 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > index d9a395c486d3..f5b9519b3756 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > @@ -1242,9 +1242,11 @@ static void psr2_man_trk_ctl_calc(struct intel_crtc_state *crtc_state, > > > > > if (clip->y1 == -1) > > > > > goto exit; > > > > > > > > > > + drm_WARN_ON(crtc_state->uapi.crtc->dev, clip->y1 % 4 || clip->y2 % 4); > > > > > + > > > > > val |= PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE; > > > > > val |= PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(clip->y1 / 4 + 1); > > > > > - val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(DIV_ROUND_UP(clip->y2, 4) + 1); > > > > > + val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2 / 4 + 1); > > > > > exit: > > > > > crtc_state->psr2_man_track_ctl = val; > > > > > } > > > > > @@ -1269,8 +1271,8 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state, > > > > > struct intel_crtc *crtc) > > > > > { > > > > > struct intel_crtc_state *crtc_state = intel_atomic_get_new_crtc_state(state, crtc); > > > > > + struct drm_rect pipe_clip = { .x1 = 0, .y1 = -1, .x2 = INT_MAX, .y2 = -1 }; > > > > > struct intel_plane_state *new_plane_state, *old_plane_state; > > > > > - struct drm_rect pipe_clip = { .y1 = -1 }; > > > > > struct intel_plane *plane; > > > > > bool full_update = false; > > > > > int i, ret; > > > > > @@ -1282,13 +1284,25 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state, > > > > > if (ret) > > > > > return ret; > > > > > > > > > > + /* > > > > > + * Calculate minimal selective fetch area of each plane and calculate > > > > > + * the pipe damaged area. > > > > > + * In the next loop the plane selective fetch area will actually be set > > > > > + * using whole pipe damaged area. > > > > > + */ > > > > > for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, > > > > > new_plane_state, i) { > > > > > - struct drm_rect *sel_fetch_area, temp; > > > > > + struct drm_rect src, damaged_area = { .y1 = -1 }; > > > > > + struct drm_mode_rect *damaged_clips; > > > > > + u32 num_clips, j; > > > > > > > > > > if (new_plane_state->uapi.crtc != crtc_state->uapi.crtc) > > > > > continue; > > > > > > > > > > + if (!new_plane_state->uapi.visible && > > > > > + !old_plane_state->uapi.visible) > > > > > + continue; > > > > > + > > > > > /* > > > > > * TODO: Not clear how to handle planes with negative position, > > > > > * also planes are not updated if they have a negative X > > > > > @@ -1300,23 +1314,94 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state, > > > > > break; > > > > > } > > > > > > > > > > - if (!new_plane_state->uapi.visible) > > > > > - continue; > > > > > + num_clips = drm_plane_get_damage_clips_count(&new_plane_state->uapi); > > > > > > > > > > /* > > > > > - * For now doing a selective fetch in the whole plane area, > > > > > - * optimizations will come in the future. > > > > > + * If visibility or plane moved, mark the whole plane area as > > > > > + * damaged as it needs to be complete redraw in the new and old > > > > > + * position. > > > > > */ > > > > > - sel_fetch_area = &new_plane_state->psr2_sel_fetch_area; > > > > > - sel_fetch_area->y1 = new_plane_state->uapi.src.y1 >> 16; > > > > > - sel_fetch_area->y2 = new_plane_state->uapi.src.y2 >> 16; > > > > > + if (new_plane_state->uapi.visible != old_plane_state->uapi.visible || > > > > > + !drm_rect_equals(&new_plane_state->uapi.dst, > > > > > + &old_plane_state->uapi.dst)) { > > > > > + if (old_plane_state->uapi.visible) { > > > > > + damaged_area.y1 = old_plane_state->uapi.dst.y1; > > > > > + damaged_area.y2 = old_plane_state->uapi.dst.y2; > > > > > + clip_area_update(&pipe_clip, &damaged_area); > > > > > + } > > > > > + > > > > > + if (new_plane_state->uapi.visible) { > > > > > + 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); > > > > > + } > > > > > + 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. > > > > > + */ > > > > > + 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); > > > > > + continue; > > > > > + } > > > > > + > > > > > + 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; > > > > > + if (drm_rect_intersect(&clip, &src)) > > > > > + clip_area_update(&damaged_area, &clip); > > > > > + } > > > > > > > > > > - temp = *sel_fetch_area; > > > > > - temp.y1 += new_plane_state->uapi.dst.y1; > > > > > - temp.y2 += new_plane_state->uapi.dst.y2; > > > > > - clip_area_update(&pipe_clip, &temp); > > > > > + if (damaged_area.y1 == -1) > > > > > + continue; > > > > > + > > > > > + damaged_area.y1 += new_plane_state->uapi.dst.y1 - src.y1; > > > > > + damaged_area.y2 += new_plane_state->uapi.dst.y1 - src.y1; > > > > > + clip_area_update(&pipe_clip, &damaged_area); > > > > > + } > > > > > + > > > > > + if (full_update) > > > > > + goto skip_sel_fetch_set_loop; > > > > > + > > > > > + /* It must be aligned to 4 lines */ > > > > > + pipe_clip.y1 -= pipe_clip.y1 % 4; > > > > > + if (pipe_clip.y2 % 4) > > > > > + pipe_clip.y2 = ((pipe_clip.y2 / 4) + 1) * 4; > > > > > + > > > > > + /* > > > > > + * Now that we have the pipe damaged area check if it intersect with > > > > > + * every plane, if it does set the plane selective fetch area. > > > > > + */ > > > > > + for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, > > > > > + new_plane_state, i) { > > > > > + struct drm_rect *sel_fetch_area, inter; > > > > > + > > > > > + if (new_plane_state->uapi.crtc != crtc_state->uapi.crtc || > > > > > + !new_plane_state->uapi.visible) > > > > > + continue; > > > > > + > > > > > + inter = pipe_clip; > > > > > + if (!drm_rect_intersect(&inter, &new_plane_state->uapi.dst)) > > > > > + continue; > > > > > + > > > > > + sel_fetch_area = &new_plane_state->psr2_sel_fetch_area; > > > > > + sel_fetch_area->y1 = inter.y1 - new_plane_state->uapi.dst.y1; > > > > > + sel_fetch_area->y2 = inter.y2 - new_plane_state->uapi.dst.y1; > > > > > } > > > > > > > > > > +skip_sel_fetch_set_loop: > > > > > psr2_man_trk_ctl_calc(crtc_state, &pipe_clip, full_update); > > > > > return 0; > > > > > } > > > > > -- > > > > > 2.30.0 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > > > > > > > -- > > > > Daniel Vetter > > > > Software Engineer, Intel Corporation > > > > http://blog.ffwll.ch > > > > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
