[Intel-gfx] [PATCH 1/6] drm/i915/display/psr: Calculate selective fetch plane registers

[Intel-gfx] [PATCH 1/6] drm/i915/display/psr: Calculate selective fetch plane registers

[José Roberto de Souza]

Add the calculations to set plane selective fetch registers depending
in the value of the area damaged.
It is still using the whole plane area as damaged but that will change
in next patches.

BSpec: 55229
Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 .../drm/i915/display/intel_display_types.h    |  2 ++
 drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++-----
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 0b5df8e44966..aeceb378bca3 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -603,6 +603,8 @@ struct intel_plane_state {
 	u32 planar_slave;
 
 	struct drm_intel_sprite_colorkey ckey;
+
+	struct drm_rect psr2_sel_fetch_area;
 };
 
 struct intel_initial_plane_config {
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index a591a475f148..773a5b5fa078 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1173,6 +1173,7 @@ void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
 {
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum pipe pipe = plane->pipe;
+	const struct drm_rect *clip;
 	u32 val;
 
 	if (!crtc_state->enable_psr2_sel_fetch)
@@ -1184,16 +1185,20 @@ void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
 	if (!val || plane->id == PLANE_CURSOR)
 		return;
 
-	val = plane_state->uapi.dst.y1 << 16 | plane_state->uapi.dst.x1;
+	clip = &plane_state->psr2_sel_fetch_area;
+
+	val = (clip->y1 + plane_state->uapi.dst.y1) << 16;
+	val |= plane_state->uapi.dst.x1;
 	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_POS(pipe, plane->id), val);
 
-	val = plane_state->color_plane[color_plane].y << 16;
+	/* TODO: consider tiling and auxiliary surfaces */
+	val = (clip->y1 + plane_state->color_plane[color_plane].y) << 16;
 	val |= plane_state->color_plane[color_plane].x;
 	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_OFFSET(pipe, plane->id),
 			  val);
 
 	/* Sizes are 0 based */
-	val = ((drm_rect_height(&plane_state->uapi.src) >> 16) - 1) << 16;
+	val = (drm_rect_height(clip) - 1) << 16;
 	val |= (drm_rect_width(&plane_state->uapi.src) >> 16) - 1;
 	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe, plane->id), val);
 }
@@ -1267,7 +1272,7 @@ 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 temp;
+		struct drm_rect *sel_fetch_area, temp;
 
 		if (new_plane_state->uapi.crtc != crtc_state->uapi.crtc)
 			continue;
@@ -1290,8 +1295,13 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
 		 * For now doing a selective fetch in the whole plane area,
 		 * optimizations will come in the future.
 		 */
-		temp.y1 = new_plane_state->uapi.dst.y1;
-		temp.y2 = new_plane_state->uapi.dst.y2;
+		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;
+
+		temp = *sel_fetch_area;
+		temp.y1 += new_plane_state->uapi.dst.y1 >> 16;
+		temp.y2 += new_plane_state->uapi.dst.y1 >> 16;
 		clip_area_update(&pipe_clip, &temp);
 	}
 
-- 
2.28.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/6] drm/i915/display/psr: Use plane damage clips to calculate damaged area

[José Roberto de Souza]

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.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
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 | 54 +++++++++++++++++++++---
 1 file changed, 49 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 773a5b5fa078..0f1e9f0fa57f 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1273,6 +1273,9 @@ 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 *sel_fetch_area, temp;
+		struct drm_mode_rect *damaged_clips;
+		u32 num_clips;
+		int j;
 
 		if (new_plane_state->uapi.crtc != crtc_state->uapi.crtc)
 			continue;
@@ -1291,13 +1294,54 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
 		if (!new_plane_state->uapi.visible)
 			continue;
 
+		sel_fetch_area = &new_plane_state->psr2_sel_fetch_area;
+		sel_fetch_area->y1 = -1;
+
+		damaged_clips = drm_plane_get_damage_clips(&new_plane_state->uapi);
+		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 plane moved, mark the whole plane area as damaged as it
+		 * needs to be complete redraw in the new 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 (!drm_rect_equals(&new_plane_state->uapi.dst,
+				     &old_plane_state->uapi.dst)) {
+			num_clips = 0;
+			sel_fetch_area->y1 = new_plane_state->uapi.src.y1 >> 16;
+			sel_fetch_area->y2 = new_plane_state->uapi.src.y2 >> 16;
+		} else if (!num_clips && new_plane_state->uapi.fb !=
+			   old_plane_state->uapi.fb) {
+			/*
+			 * If the plane don't have damage areas but the
+			 * framebuffer changed, mark the whole plane area as
+			 * damaged.
+			 */
+			sel_fetch_area->y1 = new_plane_state->uapi.src.y1 >> 16;
+			sel_fetch_area->y2 = new_plane_state->uapi.src.y2 >> 16;
+		}
+
+		for (j = 0; j < num_clips; j++) {
+			struct drm_rect damage_area;
+
+			damage_area.y1 = damaged_clips[j].y1;
+			damage_area.y2 = damaged_clips[j].y2;
+			clip_area_update(sel_fetch_area, &damage_area);
+		}
+
+		/*
+		 * No page flip, no plane moviment or no damage areas, so don't
+		 * fetch any pixel from memory for this plane
+		 */
+		if (sel_fetch_area->y1 == -1) {
+			sel_fetch_area->y1 = 0;
+			sel_fetch_area->y2 = 0;
+		}
+
+		/* Don't need to redraw plane damaged areas outside of screen */
+		j = sel_fetch_area->y2 + (new_plane_state->uapi.dst.y1 >> 16);
+		j = crtc_state->uapi.adjusted_mode.crtc_vdisplay - j;
+		if (j < 0)
+			sel_fetch_area->y2 += j;
 
 		temp = *sel_fetch_area;
 		temp.y1 += new_plane_state->uapi.dst.y1 >> 16;
-- 
2.28.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 3/6] drm/i915/display/psr: Consider other planes to damaged area calculation

[José Roberto de Souza]

Planes can individually have transparent, move or have visibility
changed if any of those happens, planes bellow it will be visible or
have more pixels of it visible than before.

This patch is taking care of this case for selective fetch by adding
to each plane damaged area all the intersections of planes above it
that matches with the characteristics described above.

There still some room from improvements here but at least this initial
version will take care of display what is expected saving some memory
reads.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
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 | 62 ++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 0f1e9f0fa57f..91ba97bf609b 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1253,11 +1253,38 @@ static void clip_area_update(struct drm_rect *overlap_damage_area,
 		overlap_damage_area->y2 = damage_area->y2;
 }
 
+/* Update plane damage area if planes above moved or have alpha */
+static void pipe_dirty_areas_set(struct intel_plane_state *plane_state,
+				 struct intel_plane *plane,
+				 const struct drm_rect *pipe_dirty_areas,
+				 struct drm_rect *sel_fetch_area)
+{
+	enum plane_id i;
+
+	for (i = PLANE_CURSOR; i > plane->id; i--) {
+		int j;
+
+		for (j = 0; j < 2; j++) {
+			struct drm_rect r = pipe_dirty_areas[i * 2 + j];
+
+			if (!drm_rect_width(&r))
+				continue;
+			if (!drm_rect_intersect(&r, &plane_state->uapi.dst))
+				continue;
+
+			r.y1 -= plane_state->uapi.dst.y1;
+			r.y2 -= plane_state->uapi.dst.y1;
+			clip_area_update(sel_fetch_area, &r);
+		}
+	}
+}
+
 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 intel_plane_state *new_plane_state, *old_plane_state;
+	struct drm_rect pipe_dirty_areas[I915_MAX_PLANES * 2] = {};
 	struct drm_rect pipe_clip = { .y1 = -1 };
 	struct intel_plane *plane;
 	bool full_update = false;
@@ -1270,6 +1297,38 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
 	if (ret)
 		return ret;
 
+	/*
+	 * Mark all the areas where there is a plane that matches one of this:
+	 * - transparent
+	 * - moved
+	 * - visibility changed
+	 * In all those cases, planes bellow it will need to be redraw.
+	 */
+	for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state,
+					     new_plane_state, i) {
+		bool alpha, flip, dirty;
+
+		if (new_plane_state->uapi.crtc != crtc_state->uapi.crtc)
+			continue;
+
+		alpha = new_plane_state->uapi.alpha != DRM_BLEND_ALPHA_OPAQUE;
+		alpha |= old_plane_state->uapi.alpha != DRM_BLEND_ALPHA_OPAQUE;
+		flip = new_plane_state->uapi.fb != old_plane_state->uapi.fb;
+		dirty = alpha && flip;
+
+		dirty |= !drm_rect_equals(&new_plane_state->uapi.dst,
+					  &old_plane_state->uapi.dst);
+		dirty |= new_plane_state->uapi.visible !=
+			 old_plane_state->uapi.visible;
+		if (!dirty)
+			continue;
+
+		if (old_plane_state->uapi.visible)
+			pipe_dirty_areas[plane->id * 2] = old_plane_state->uapi.dst;
+		if (new_plane_state->uapi.visible)
+			pipe_dirty_areas[plane->id * 2 + 1] = new_plane_state->uapi.dst;
+	}
+
 	for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state,
 					     new_plane_state, i) {
 		struct drm_rect *sel_fetch_area, temp;
@@ -1337,6 +1396,9 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
 			sel_fetch_area->y2 = 0;
 		}
 
+		pipe_dirty_areas_set(new_plane_state, plane, pipe_dirty_areas,
+				     sel_fetch_area);
+
 		/* Don't need to redraw plane damaged areas outside of screen */
 		j = sel_fetch_area->y2 + (new_plane_state->uapi.dst.y1 >> 16);
 		j = crtc_state->uapi.adjusted_mode.crtc_vdisplay - j;
-- 
2.28.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 4/6] drm/i915/display: Split and export main surface calculation from skl_check_main_surface()

[José Roberto de Souza]

The calculation the offsets of the main surface will be needed by PSR2
selective fetch code so here splitting and exporting it.
No functional changes were done here.

Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 95 ++++++++++++--------
 drivers/gpu/drm/i915/display/intel_display.h |  2 +
 2 files changed, 59 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index cf1417ff54d7..64242ecb8610 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -3889,6 +3889,56 @@ intel_plane_fence_y_offset(const struct intel_plane_state *plane_state)
 	return y;
 }
 
+int skl_calc_main_surface_offset(const struct intel_plane_state *plane_state,
+				 int *x, int *y, u32 *offset)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane_state->uapi.plane->dev);
+	const struct drm_framebuffer *fb = plane_state->hw.fb;
+	const int aux_plane = intel_main_to_aux_plane(fb, 0);
+	const u32 aux_offset = plane_state->color_plane[aux_plane].offset;
+	const u32 alignment = intel_surf_alignment(fb, 0);
+	const int w = drm_rect_width(&plane_state->uapi.src) >> 16;
+
+	intel_add_fb_offsets(x, y, plane_state, 0);
+	*offset = intel_plane_compute_aligned_offset(x, y, plane_state, 0);
+	if (drm_WARN_ON(&dev_priv->drm, alignment && !is_power_of_2(alignment)))
+		return -EINVAL;
+
+	/*
+	 * AUX surface offset is specified as the distance from the
+	 * main surface offset, and it must be non-negative. Make
+	 * sure that is what we will get.
+	 */
+	if (aux_plane && *offset > aux_offset)
+		*offset = intel_plane_adjust_aligned_offset(x, y, plane_state, 0,
+							    *offset,
+							    aux_offset & ~(alignment - 1));
+
+	/*
+	 * When using an X-tiled surface, the plane blows up
+	 * if the x offset + width exceed the stride.
+	 *
+	 * TODO: linear and Y-tiled seem fine, Yf untested,
+	 */
+	if (fb->modifier == I915_FORMAT_MOD_X_TILED) {
+		int cpp = fb->format->cpp[0];
+
+		while ((*x + w) * cpp > plane_state->color_plane[0].stride) {
+			if (*offset == 0) {
+				drm_dbg_kms(&dev_priv->drm,
+					    "Unable to find suitable display surface offset due to X-tiling\n");
+				return -EINVAL;
+			}
+
+			*offset = intel_plane_adjust_aligned_offset(x, y, plane_state, 0,
+								    *offset,
+								    *offset - alignment);
+		}
+	}
+
+	return 0;
+}
+
 static int skl_check_main_surface(struct intel_plane_state *plane_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(plane_state->uapi.plane->dev);
@@ -3899,9 +3949,10 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
 	int w = drm_rect_width(&plane_state->uapi.src) >> 16;
 	int h = drm_rect_height(&plane_state->uapi.src) >> 16;
 	int max_width, min_width, max_height;
-	u32 alignment, offset;
-	int aux_plane = intel_main_to_aux_plane(fb, 0);
-	u32 aux_offset = plane_state->color_plane[aux_plane].offset;
+	const int aux_plane = intel_main_to_aux_plane(fb, 0);
+	const u32 alignment = intel_surf_alignment(fb, 0);
+	u32 offset;
+	int ret;
 
 	if (INTEL_GEN(dev_priv) >= 11) {
 		max_width = icl_max_plane_width(fb, 0, rotation);
@@ -3926,41 +3977,9 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
 		return -EINVAL;
 	}
 
-	intel_add_fb_offsets(&x, &y, plane_state, 0);
-	offset = intel_plane_compute_aligned_offset(&x, &y, plane_state, 0);
-	alignment = intel_surf_alignment(fb, 0);
-	if (drm_WARN_ON(&dev_priv->drm, alignment && !is_power_of_2(alignment)))
-		return -EINVAL;
-
-	/*
-	 * AUX surface offset is specified as the distance from the
-	 * main surface offset, and it must be non-negative. Make
-	 * sure that is what we will get.
-	 */
-	if (aux_plane && offset > aux_offset)
-		offset = intel_plane_adjust_aligned_offset(&x, &y, plane_state, 0,
-							   offset, aux_offset & ~(alignment - 1));
-
-	/*
-	 * When using an X-tiled surface, the plane blows up
-	 * if the x offset + width exceed the stride.
-	 *
-	 * TODO: linear and Y-tiled seem fine, Yf untested,
-	 */
-	if (fb->modifier == I915_FORMAT_MOD_X_TILED) {
-		int cpp = fb->format->cpp[0];
-
-		while ((x + w) * cpp > plane_state->color_plane[0].stride) {
-			if (offset == 0) {
-				drm_dbg_kms(&dev_priv->drm,
-					    "Unable to find suitable display surface offset due to X-tiling\n");
-				return -EINVAL;
-			}
-
-			offset = intel_plane_adjust_aligned_offset(&x, &y, plane_state, 0,
-								   offset, offset - alignment);
-		}
-	}
+	ret = skl_calc_main_surface_offset(plane_state, &x, &y, &offset);
+	if (ret)
+		return ret;
 
 	/*
 	 * CCS AUX surface doesn't have its own x/y offsets, we must make sure
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index d10b7c8cde3f..4be68b5be149 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -609,6 +609,8 @@ u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state);
 u32 skl_plane_stride(const struct intel_plane_state *plane_state,
 		     int plane);
 int skl_check_plane_surface(struct intel_plane_state *plane_state);
+int skl_calc_main_surface_offset(const struct intel_plane_state *plane_state,
+				 int *x, int *y, u32 *offset);
 int i9xx_check_plane_surface(struct intel_plane_state *plane_state);
 int skl_format_to_fourcc(int format, bool rgb_order, bool alpha);
 unsigned int i9xx_plane_max_stride(struct intel_plane *plane,
-- 
2.28.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 5/6] RFC/WIP: drm/i915/display/psr: Consider tiling when doing the plane offset calculation

[José Roberto de Souza]

Do the calculation of x and y offsets using
skl_calc_main_surface_offset().

RFC/WIP: This causes the value of the calculated x to be different than
plane_state->color_plane[color_plane].x, not sure if that is expected.

Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 91ba97bf609b..c30d7069cbaa 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1174,7 +1174,8 @@ void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum pipe pipe = plane->pipe;
 	const struct drm_rect *clip;
-	u32 val;
+	u32 val, offset;
+	int ret, x, y;
 
 	if (!crtc_state->enable_psr2_sel_fetch)
 		return;
@@ -1191,9 +1192,14 @@ void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
 	val |= plane_state->uapi.dst.x1;
 	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_POS(pipe, plane->id), val);
 
-	/* TODO: consider tiling and auxiliary surfaces */
-	val = (clip->y1 + plane_state->color_plane[color_plane].y) << 16;
-	val |= plane_state->color_plane[color_plane].x;
+	/* TODO: consider auxiliary surfaces */
+	x = plane_state->uapi.src.x1 >> 16;
+	y = (plane_state->uapi.src.y1 >> 16) + clip->y1;
+	ret = skl_calc_main_surface_offset(plane_state, &x, &y, &offset);
+	if (ret)
+		drm_warn_once(&dev_priv->drm, "skl_calc_main_surface_offset() returned %i\n",
+			      ret);
+	val = y << 16 | x;
 	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_OFFSET(pipe, plane->id),
 			  val);
 
-- 
2.28.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 6/6] DEBUG: drm/i915/display: Add debug information to selective fetch

[José Roberto de Souza]

Just for feature development, not needed in production.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index c30d7069cbaa..1b2ae3bd02ee 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1194,11 +1194,19 @@ void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
 
 	/* TODO: consider auxiliary surfaces */
 	x = plane_state->uapi.src.x1 >> 16;
-	y = (plane_state->uapi.src.y1 >> 16) + clip->y1;
+	y = plane_state->uapi.src.y1 >> 16;
+	drm_info(&dev_priv->drm, "plane%c src.x=%i src.y=%i clip.y1=%i clip.y2=%i\n",
+		 plane_name(plane->id), x, y, clip->y1, clip->y2);
+	drm_info(&dev_priv->drm, "\tcolor_plane[color_plane].x=%i color_plane[color_plane].y=%i\n",
+		 plane_state->color_plane[color_plane].x,
+		 plane_state->color_plane[color_plane].y);
+
+	y += clip->y1;
 	ret = skl_calc_main_surface_offset(plane_state, &x, &y, &offset);
 	if (ret)
 		drm_warn_once(&dev_priv->drm, "skl_calc_main_surface_offset() returned %i\n",
 			      ret);
+	drm_info(&dev_priv->drm, "\tcalculated offset x=%i y=%i\n", x, y);
 	val = y << 16 | x;
 	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_OFFSET(pipe, plane->id),
 			  val);
@@ -1335,6 +1343,8 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
 			pipe_dirty_areas[plane->id * 2 + 1] = new_plane_state->uapi.dst;
 	}
 
+	drm_info(state->base.dev, "intel_psr2_sel_fetch_update()\n");
+
 	for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state,
 					     new_plane_state, i) {
 		struct drm_rect *sel_fetch_area, temp;
@@ -1411,6 +1421,11 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
 		if (j < 0)
 			sel_fetch_area->y2 += j;
 
+		drm_info(state->base.dev, "\tplane%c y1=%i y2=%i dst.y1=%i dst.y2=%i\n",
+			 plane_name(plane->id), sel_fetch_area->y1,
+			 sel_fetch_area->y2, new_plane_state->uapi.dst.y1,
+			 new_plane_state->uapi.dst.y2);
+
 		temp = *sel_fetch_area;
 		temp.y1 += new_plane_state->uapi.dst.y1 >> 16;
 		temp.y2 += new_plane_state->uapi.dst.y1 >> 16;
-- 
2.28.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/6] drm/i915/display/psr: Calculate selective fetch plane registers

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915/display/psr: Calculate selective fetch plane registers
URL   : https://patchwork.freedesktop.org/series/82646/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
2b304de51d75 drm/i915/display/psr: Calculate selective fetch plane registers
fbf763534040 drm/i915/display/psr: Use plane damage clips to calculate damaged area
ef08d57e6534 drm/i915/display/psr: Consider other planes to damaged area calculation
96a8b4924e3c drm/i915/display: Split and export main surface calculation from skl_check_main_surface()
6117d56f10f2 RFC/WIP: drm/i915/display/psr: Consider tiling when doing the plane offset calculation
029c35bb64f3 DEBUG: drm/i915/display: Add debug information to selective fetch
-:43: WARNING:EMBEDDED_FUNCTION_NAME: Prefer using '"%s...", __func__' to using 'intel_psr2_sel_fetch_update', this function's name, in a string
#43: FILE: drivers/gpu/drm/i915/display/intel_psr.c:1346:
+	drm_info(state->base.dev, "intel_psr2_sel_fetch_update()\n");

total: 0 errors, 1 warnings, 0 checks, 39 lines checked


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [1/6] drm/i915/display/psr: Calculate selective fetch plane registers

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915/display/psr: Calculate selective fetch plane registers
URL   : https://patchwork.freedesktop.org/series/82646/
State : warning

== Summary ==

$ dim sparse --fast origin/drm-tip
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.
-
+drivers/gpu/drm/i915/gt/intel_reset.c:1312:5: warning: context imbalance in 'intel_gt_reset_trylock' - different lock contexts for basic block
+drivers/gpu/drm/i915/gvt/mmio.c:290:23: warning: memcpy with byte count of 279040
+drivers/gpu/drm/i915/i915_perf.c:1440:15: warning: memset with byte count of 16777216
+drivers/gpu/drm/i915/i915_perf.c:1494:15: warning: memset with byte count of 16777216
+drivers/gpu/drm/i915/intel_wakeref.c:137:19: warning: context imbalance in 'wakeref_auto_timeout' - unexpected unlock
+./include/linux/seqlock.h:752:24: warning: trying to copy expression type 31
+./include/linux/seqlock.h:778:16: warning: trying to copy expression type 31
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'fwtable_read16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'fwtable_read32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'fwtable_read64' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'fwtable_read8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'fwtable_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'fwtable_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'fwtable_write8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen11_fwtable_read16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen11_fwtable_read32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen11_fwtable_read64' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen11_fwtable_read8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen11_fwtable_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen11_fwtable_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen11_fwtable_write8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen12_fwtable_read16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen12_fwtable_read32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen12_fwtable_read64' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen12_fwtable_read8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen12_fwtable_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen12_fwtable_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen12_fwtable_write8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen6_read16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen6_read32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen6_read64' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen6_read8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen6_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen6_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen6_write8' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen8_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen8_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:409:9: warning: context imbalance in 'gen8_write8' - different lock contexts for basic block


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/6] drm/i915/display/psr: Calculate selective fetch plane registers

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 7044 bytes --]

== Series Details ==

Series: series starting with [1/6] drm/i915/display/psr: Calculate selective fetch plane registers
URL   : https://patchwork.freedesktop.org/series/82646/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_9137 -> Patchwork_18694
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/index.html

Known issues
------------

  Here are the changes found in Patchwork_18694 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_flink_basic@flink-lifetime:
    - fi-tgl-y:           [PASS][1] -> [DMESG-WARN][2] ([i915#402]) +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/fi-tgl-y/igt@gem_flink_basic@flink-lifetime.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/fi-tgl-y/igt@gem_flink_basic@flink-lifetime.html

  * igt@i915_pm_rpm@module-reload:
    - fi-byt-j1900:       [PASS][3] -> [DMESG-WARN][4] ([i915#1982])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/fi-byt-j1900/igt@i915_pm_rpm@module-reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/fi-byt-j1900/igt@i915_pm_rpm@module-reload.html

  * igt@kms_busy@basic@flip:
    - fi-tgl-y:           [PASS][5] -> [DMESG-WARN][6] ([i915#1982])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/fi-tgl-y/igt@kms_busy@basic@flip.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/fi-tgl-y/igt@kms_busy@basic@flip.html

  * igt@vgem_basic@unload:
    - fi-skl-guc:         [PASS][7] -> [DMESG-WARN][8] ([i915#2203])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/fi-skl-guc/igt@vgem_basic@unload.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/fi-skl-guc/igt@vgem_basic@unload.html

  
#### Possible fixes ####

  * igt@i915_module_load@reload:
    - {fi-tgl-dsi}:       [DMESG-WARN][9] ([i915#1982] / [k.org#205379]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/fi-tgl-dsi/igt@i915_module_load@reload.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/fi-tgl-dsi/igt@i915_module_load@reload.html
    - fi-tgl-y:           [DMESG-WARN][11] ([i915#1982] / [k.org#205379]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/fi-tgl-y/igt@i915_module_load@reload.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/fi-tgl-y/igt@i915_module_load@reload.html

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-bsw-kefka:       [DMESG-WARN][13] ([i915#1982]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/fi-bsw-kefka/igt@i915_pm_rpm@basic-pci-d3-state.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/fi-bsw-kefka/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       [FAIL][15] ([i915#1161] / [i915#262]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - {fi-kbl-7560u}:     [DMESG-WARN][17] ([i915#1982]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/fi-kbl-7560u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/fi-kbl-7560u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - fi-icl-u2:          [DMESG-WARN][19] ([i915#1982]) -> [PASS][20] +1 similar issue
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/fi-icl-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/fi-icl-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c:
    - {fi-tgl-dsi}:       [DMESG-WARN][21] ([i915#1982]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/fi-tgl-dsi/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/fi-tgl-dsi/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c.html

  * {igt@prime_vgem@basic-userptr}:
    - fi-tgl-y:           [DMESG-WARN][23] ([i915#402]) -> [PASS][24] +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/fi-tgl-y/igt@prime_vgem@basic-userptr.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/fi-tgl-y/igt@prime_vgem@basic-userptr.html

  
#### Warnings ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-tgl-y:           [DMESG-WARN][25] ([i915#2411] / [i915#402]) -> [DMESG-WARN][26] ([i915#2411])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/fi-tgl-y/igt@gem_exec_suspend@basic-s3.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/fi-tgl-y/igt@gem_exec_suspend@basic-s3.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1161]: https://gitlab.freedesktop.org/drm/intel/issues/1161
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2203]: https://gitlab.freedesktop.org/drm/intel/issues/2203
  [i915#2411]: https://gitlab.freedesktop.org/drm/intel/issues/2411
  [i915#262]: https://gitlab.freedesktop.org/drm/intel/issues/262
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [k.org#205379]: https://bugzilla.kernel.org/show_bug.cgi?id=205379


Participating hosts (46 -> 41)
------------------------------

  Missing    (5): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper 


Build changes
-------------

  * Linux: CI_DRM_9137 -> Patchwork_18694

  CI-20190529: 20190529
  CI_DRM_9137: 9c7e985c2336328b14dd87c0f6a83af094f59d53 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5813: d4e6dd955a1dad02271aa41c9389f5097ee17765 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18694: 029c35bb64f394482d517de1e639975f425bbec9 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

029c35bb64f3 DEBUG: drm/i915/display: Add debug information to selective fetch
6117d56f10f2 RFC/WIP: drm/i915/display/psr: Consider tiling when doing the plane offset calculation
96a8b4924e3c drm/i915/display: Split and export main surface calculation from skl_check_main_surface()
ef08d57e6534 drm/i915/display/psr: Consider other planes to damaged area calculation
fbf763534040 drm/i915/display/psr: Use plane damage clips to calculate damaged area
2b304de51d75 drm/i915/display/psr: Calculate selective fetch plane registers

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/index.html

[-- Attachment #1.2: Type: text/html, Size: 8724 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.IGT: failure for series starting with [1/6] drm/i915/display/psr: Calculate selective fetch plane registers

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 14587 bytes --]

== Series Details ==

Series: series starting with [1/6] drm/i915/display/psr: Calculate selective fetch plane registers
URL   : https://patchwork.freedesktop.org/series/82646/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_9137_full -> Patchwork_18694_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_18694_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_18694_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_18694_full:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_whisper@basic-fds-priority-all:
    - shard-glk:          [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-glk7/igt@gem_exec_whisper@basic-fds-priority-all.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-glk9/igt@gem_exec_whisper@basic-fds-priority-all.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-indfb-plflip-blt:
    - shard-tglb:         [PASS][3] -> [FAIL][4] +2 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-tglb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-indfb-plflip-blt.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-tglb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-indfb-plflip-blt.html

  
#### Warnings ####

  * igt@kms_frontbuffer_tracking@fbcpsr-tiling-y:
    - shard-tglb:         [DMESG-FAIL][5] ([i915#1982]) -> [FAIL][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-tglb2/igt@kms_frontbuffer_tracking@fbcpsr-tiling-y.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-tglb8/igt@kms_frontbuffer_tracking@fbcpsr-tiling-y.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * {igt@gem_exec_capture@pi@bcs0}:
    - shard-glk:          [PASS][7] -> [INCOMPLETE][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-glk1/igt@gem_exec_capture@pi@bcs0.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-glk5/igt@gem_exec_capture@pi@bcs0.html

  
Known issues
------------

  Here are the changes found in Patchwork_18694_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_whisper@basic-queues-all:
    - shard-glk:          [PASS][9] -> [DMESG-WARN][10] ([i915#118] / [i915#95])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-glk7/igt@gem_exec_whisper@basic-queues-all.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-glk7/igt@gem_exec_whisper@basic-queues-all.html

  * igt@gem_userptr_blits@unsync-unmap-cycles:
    - shard-skl:          [PASS][11] -> [TIMEOUT][12] ([i915#2424]) +1 similar issue
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-skl2/igt@gem_userptr_blits@unsync-unmap-cycles.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-skl4/igt@gem_userptr_blits@unsync-unmap-cycles.html

  * igt@i915_module_load@reload:
    - shard-iclb:         [PASS][13] -> [DMESG-WARN][14] ([i915#1982])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-iclb3/igt@i915_module_load@reload.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-iclb4/igt@i915_module_load@reload.html

  * igt@kms_cursor_edge_walk@pipe-b-256x256-left-edge:
    - shard-skl:          [PASS][15] -> [DMESG-WARN][16] ([i915#1982]) +11 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-skl8/igt@kms_cursor_edge_walk@pipe-b-256x256-left-edge.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-skl9/igt@kms_cursor_edge_walk@pipe-b-256x256-left-edge.html

  * igt@kms_flip@busy-flip@a-edp1:
    - shard-tglb:         [PASS][17] -> [DMESG-WARN][18] ([i915#1982]) +1 similar issue
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-tglb8/igt@kms_flip@busy-flip@a-edp1.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-tglb5/igt@kms_flip@busy-flip@a-edp1.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1:
    - shard-skl:          [PASS][19] -> [FAIL][20] ([i915#79])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-skl3/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-skl10/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html

  * igt@kms_psr@psr2_suspend:
    - shard-iclb:         [PASS][21] -> [SKIP][22] ([fdo#109441]) +1 similar issue
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-iclb2/igt@kms_psr@psr2_suspend.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-iclb5/igt@kms_psr@psr2_suspend.html

  * igt@kms_vblank@pipe-b-accuracy-idle:
    - shard-skl:          [PASS][23] -> [FAIL][24] ([i915#43])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-skl9/igt@kms_vblank@pipe-b-accuracy-idle.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-skl5/igt@kms_vblank@pipe-b-accuracy-idle.html

  * igt@perf@polling-parameterized:
    - shard-glk:          [PASS][25] -> [FAIL][26] ([i915#1542])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-glk7/igt@perf@polling-parameterized.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-glk9/igt@perf@polling-parameterized.html

  * igt@perf@polling-small-buf:
    - shard-skl:          [PASS][27] -> [FAIL][28] ([i915#1722])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-skl1/igt@perf@polling-small-buf.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-skl9/igt@perf@polling-small-buf.html

  
#### Possible fixes ####

  * igt@gem_exec_whisper@basic-queues:
    - shard-glk:          [DMESG-WARN][29] ([i915#118] / [i915#95]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-glk9/igt@gem_exec_whisper@basic-queues.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-glk6/igt@gem_exec_whisper@basic-queues.html

  * igt@gen9_exec_parse@allowed-single:
    - shard-skl:          [DMESG-WARN][31] ([i915#1436] / [i915#716]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-skl4/igt@gen9_exec_parse@allowed-single.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-skl2/igt@gen9_exec_parse@allowed-single.html

  * {igt@kms_async_flips@async-flip-with-page-flip-events}:
    - shard-kbl:          [FAIL][33] ([i915#2521]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-kbl6/igt@kms_async_flips@async-flip-with-page-flip-events.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-kbl2/igt@kms_async_flips@async-flip-with-page-flip-events.html

  * igt@kms_big_fb@linear-64bpp-rotate-0:
    - shard-iclb:         [DMESG-WARN][35] ([i915#1982]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-iclb3/igt@kms_big_fb@linear-64bpp-rotate-0.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-iclb4/igt@kms_big_fb@linear-64bpp-rotate-0.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-pgflip-blt:
    - shard-tglb:         [FAIL][37] -> [PASS][38] +1 similar issue
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-tglb1/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-pgflip-blt.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-tglb1/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-pgflip-blt.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-a:
    - shard-skl:          [FAIL][39] ([i915#53]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-skl4/igt@kms_pipe_crc_basic@read-crc-pipe-a.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-skl2/igt@kms_pipe_crc_basic@read-crc-pipe-a.html

  * igt@kms_plane@plane-panning-top-left-pipe-c-planes:
    - shard-skl:          [DMESG-WARN][41] ([i915#1982]) -> [PASS][42] +6 similar issues
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-skl4/igt@kms_plane@plane-panning-top-left-pipe-c-planes.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-skl3/igt@kms_plane@plane-panning-top-left-pipe-c-planes.html

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-skl:          [FAIL][43] ([fdo#108145] / [i915#265]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-skl4/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-skl2/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html

  * igt@kms_psr@psr2_sprite_mmap_gtt:
    - shard-iclb:         [SKIP][45] ([fdo#109441]) -> [PASS][46] +1 similar issue
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-iclb7/igt@kms_psr@psr2_sprite_mmap_gtt.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-iclb2/igt@kms_psr@psr2_sprite_mmap_gtt.html

  * igt@kms_setmode@basic:
    - shard-glk:          [FAIL][47] ([i915#31]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-glk8/igt@kms_setmode@basic.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-glk8/igt@kms_setmode@basic.html

  * igt@kms_vblank@pipe-a-ts-continuation-suspend:
    - shard-skl:          [INCOMPLETE][49] ([i915#198]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-skl1/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-skl10/igt@kms_vblank@pipe-a-ts-continuation-suspend.html

  * igt@kms_vblank@pipe-b-wait-busy-hang:
    - shard-glk:          [DMESG-WARN][51] ([i915#1982]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-glk2/igt@kms_vblank@pipe-b-wait-busy-hang.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-glk2/igt@kms_vblank@pipe-b-wait-busy-hang.html

  * igt@perf_pmu@module-unload:
    - shard-tglb:         [DMESG-WARN][53] ([i915#1982]) -> [PASS][54] +2 similar issues
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-tglb3/igt@perf_pmu@module-unload.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-tglb3/igt@perf_pmu@module-unload.html

  
#### Warnings ####

  * igt@i915_pm_dc@dc3co-vpb-simulation:
    - shard-iclb:         [SKIP][55] ([i915#658]) -> [SKIP][56] ([i915#588])
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-iclb7/igt@i915_pm_dc@dc3co-vpb-simulation.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-iclb2/igt@i915_pm_dc@dc3co-vpb-simulation.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-shrfb-draw-blt:
    - shard-tglb:         [FAIL][57] -> [DMESG-FAIL][58] ([i915#1982])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-tglb8/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-shrfb-draw-blt.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-tglb2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-shrfb-draw-blt.html

  * igt@runner@aborted:
    - shard-glk:          [FAIL][59] ([i915#2439] / [k.org#202321]) -> ([FAIL][60], [FAIL][61]) ([i915#2263] / [i915#2439] / [k.org#202321])
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9137/shard-glk1/igt@runner@aborted.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-glk5/igt@runner@aborted.html
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/shard-glk9/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [i915#118]: https://gitlab.freedesktop.org/drm/intel/issues/118
  [i915#1436]: https://gitlab.freedesktop.org/drm/intel/issues/1436
  [i915#1542]: https://gitlab.freedesktop.org/drm/intel/issues/1542
  [i915#1635]: https://gitlab.freedesktop.org/drm/intel/issues/1635
  [i915#1722]: https://gitlab.freedesktop.org/drm/intel/issues/1722
  [i915#198]: https://gitlab.freedesktop.org/drm/intel/issues/198
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2263]: https://gitlab.freedesktop.org/drm/intel/issues/2263
  [i915#2424]: https://gitlab.freedesktop.org/drm/intel/issues/2424
  [i915#2439]: https://gitlab.freedesktop.org/drm/intel/issues/2439
  [i915#2521]: https://gitlab.freedesktop.org/drm/intel/issues/2521
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
  [i915#43]: https://gitlab.freedesktop.org/drm/intel/issues/43
  [i915#53]: https://gitlab.freedesktop.org/drm/intel/issues/53
  [i915#588]: https://gitlab.freedesktop.org/drm/intel/issues/588
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#716]: https://gitlab.freedesktop.org/drm/intel/issues/716
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95
  [k.org#202321]: https://bugzilla.kernel.org/show_bug.cgi?id=202321


Participating hosts (11 -> 11)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * Linux: CI_DRM_9137 -> Patchwork_18694

  CI-20190529: 20190529
  CI_DRM_9137: 9c7e985c2336328b14dd87c0f6a83af094f59d53 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5813: d4e6dd955a1dad02271aa41c9389f5097ee17765 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18694: 029c35bb64f394482d517de1e639975f425bbec9 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18694/index.html

[-- Attachment #1.2: Type: text/html, Size: 16763 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/6] drm/i915/display/psr: Calculate selective fetch plane registers

[Mun, Gwan-gyeong]

On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza wrote:
> Add the calculations to set plane selective fetch registers depending
> in the value of the area damaged.
> It is still using the whole plane area as damaged but that will
> change
> in next patches.
> 
> BSpec: 55229
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |  2 ++
>  drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++---
> --
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 0b5df8e44966..aeceb378bca3 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -603,6 +603,8 @@ struct intel_plane_state {
>  	u32 planar_slave;
>  
>  	struct drm_intel_sprite_colorkey ckey;
> +
> +	struct drm_rect psr2_sel_fetch_area;
>  };
>  
>  struct intel_initial_plane_config {
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index a591a475f148..773a5b5fa078 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1173,6 +1173,7 @@ void intel_psr2_program_plane_sel_fetch(struct
> intel_plane *plane,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  	enum pipe pipe = plane->pipe;
> +	const struct drm_rect *clip;
>  	u32 val;
>  
>  	if (!crtc_state->enable_psr2_sel_fetch)
> @@ -1184,16 +1185,20 @@ void
> intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
>  	if (!val || plane->id == PLANE_CURSOR)
>  		return;
>  
> -	val = plane_state->uapi.dst.y1 << 16 | plane_state-
> >uapi.dst.x1;
> +	clip = &plane_state->psr2_sel_fetch_area;
> +
> +	val = (clip->y1 + plane_state->uapi.dst.y1) << 16;

> +	val |= plane_state->uapi.dst.x1;
>  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_POS(pipe, plane-
> >id), val);
>  
> -	val = plane_state->color_plane[color_plane].y << 16;
> +	/* TODO: consider tiling and auxiliary surfaces */
> +	val = (clip->y1 + plane_state->color_plane[color_plane].y) <<
> 16;
>  	val |= plane_state->color_plane[color_plane].x;
>  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_OFFSET(pipe, plane-
> >id),
>  			  val);
>  
>  	/* Sizes are 0 based */
> -	val = ((drm_rect_height(&plane_state->uapi.src) >> 16) - 1) <<
> 16;
> +	val = (drm_rect_height(clip) - 1) << 16;
>  	val |= (drm_rect_width(&plane_state->uapi.src) >> 16) - 1;
>  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe, plane-
> >id), val);
>  }
> @@ -1267,7 +1272,7 @@ 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 temp;
> +		struct drm_rect *sel_fetch_area, temp;
>  
>  		if (new_plane_state->uapi.crtc != crtc_state-
> >uapi.crtc)
>  			continue;
> @@ -1290,8 +1295,13 @@ int intel_psr2_sel_fetch_update(struct
> intel_atomic_state *state,
>  		 * For now doing a selective fetch in the whole plane
> area,
>  		 * optimizations will come in the future.
>  		 */
> -		temp.y1 = new_plane_state->uapi.dst.y1;
> -		temp.y2 = new_plane_state->uapi.dst.y2;
> +		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;
> +
> +		temp = *sel_fetch_area;
> +		temp.y1 += new_plane_state->uapi.dst.y1 >> 16;
> +		temp.y2 += new_plane_state->uapi.dst.y1 >> 16;
It adds src to dst. 
For the whole plane damage area, these previous code looks correct.

 temp.y1 = new_plane_state->uapi.dst.y1;
 temp.y2 = new_plane_state->uapi.dst.y2;

>  		clip_area_update(&pipe_clip, &temp);
>  	}
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915/display/psr: Use plane damage clips to calculate damaged area

[Mun, Gwan-gyeong]

On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza 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.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 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 | 54 +++++++++++++++++++++-
> --
>  1 file changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 773a5b5fa078..0f1e9f0fa57f 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1273,6 +1273,9 @@ 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 *sel_fetch_area, temp;
> +		struct drm_mode_rect *damaged_clips;
> +		u32 num_clips;
> +		int j;
>  
>  		if (new_plane_state->uapi.crtc != crtc_state-
> >uapi.crtc)
>  			continue;
> @@ -1291,13 +1294,54 @@ int intel_psr2_sel_fetch_update(struct
> intel_atomic_state *state,
>  		if (!new_plane_state->uapi.visible)
>  			continue;
>  
> +		sel_fetch_area = &new_plane_state->psr2_sel_fetch_area;
> +		sel_fetch_area->y1 = -1;
> +
> +		damaged_clips =
> drm_plane_get_damage_clips(&new_plane_state->uapi);
> +		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 plane moved, mark the whole plane area as damaged
> as it
> +		 * needs to be complete redraw in the new 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 (!drm_rect_equals(&new_plane_state->uapi.dst,
> +				     &old_plane_state->uapi.dst)) {
> +			num_clips = 0;
> +			sel_fetch_area->y1 = new_plane_state-
> >uapi.src.y1 >> 16;
> +			sel_fetch_area->y2 = new_plane_state-
> >uapi.src.y2 >> 16;
> +		} else if (!num_clips && new_plane_state->uapi.fb !=
> +			   old_plane_state->uapi.fb) {
> +			/*
> +			 * If the plane don't have damage areas but the
> +			 * framebuffer changed, mark the whole plane
> area as
> +			 * damaged.
> +			 */
> +			sel_fetch_area->y1 = new_plane_state-
> >uapi.src.y1 >> 16;
> +			sel_fetch_area->y2 = new_plane_state-
> >uapi.src.y2 >> 16;
> +		}
> +
why don't you use drm_atomic_helper_damage_merged() function here?
> +		for (j = 0; j < num_clips; j++) {
> +			struct drm_rect damage_area;
> +
> +			damage_area.y1 = damaged_clips[j].y1;
> +			damage_area.y2 = damaged_clips[j].y2;
> +			clip_area_update(sel_fetch_area, &damage_area);
> +		}
> +
> +		/*
> +		 * No page flip, no plane moviment or no damage areas,
> so don't
typo (moviment -> movement)
> +		 * fetch any pixel from memory for this plane
> +		 */
> +		if (sel_fetch_area->y1 == -1) {
> +			sel_fetch_area->y1 = 0;
> +			sel_fetch_area->y2 = 0;
> +		}
> +
> +		/* Don't need to redraw plane damaged areas outside of
> screen */
> +		j = sel_fetch_area->y2 + (new_plane_state->uapi.dst.y1
> >> 16);
src coordinates of the drm_plane_state are 16.16 fixed point but dst
coordinates are not 16.16 fixed point.
therefore we don't need to bit shift for dst.
Because the sel_fetch_area seems based on src coordinates, in order to
apply to dst coordinates here,  it requires coordinate calculation. 
> +		j = crtc_state->uapi.adjusted_mode.crtc_vdisplay - j;
> +		if (j < 0)
> +			sel_fetch_area->y2 += j;
>  
>  		temp = *sel_fetch_area;
>  		temp.y1 += new_plane_state->uapi.dst.y1 >> 16;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/6] drm/i915/display/psr: Calculate selective fetch plane registers

[Souza, Jose]

On Mon, 2020-10-26 at 21:13 +0000, Mun, Gwan-gyeong wrote:
> On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza wrote:
> > Add the calculations to set plane selective fetch registers depending
> > in the value of the area damaged.
> > It is still using the whole plane area as damaged but that will
> > change
> > in next patches.
> > 
> > BSpec: 55229
> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  .../drm/i915/display/intel_display_types.h    |  2 ++
> >  drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++---
> > --
> >  2 files changed, 18 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 0b5df8e44966..aeceb378bca3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -603,6 +603,8 @@ struct intel_plane_state {
> >  	u32 planar_slave;
> >  
> > 
> > 
> > 
> >  	struct drm_intel_sprite_colorkey ckey;
> > +
> > +	struct drm_rect psr2_sel_fetch_area;
> >  };
> >  
> > 
> > 
> > 
> >  struct intel_initial_plane_config {
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index a591a475f148..773a5b5fa078 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1173,6 +1173,7 @@ void intel_psr2_program_plane_sel_fetch(struct
> > intel_plane *plane,
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >  	enum pipe pipe = plane->pipe;
> > +	const struct drm_rect *clip;
> >  	u32 val;
> >  
> > 
> > 
> > 
> >  	if (!crtc_state->enable_psr2_sel_fetch)
> > @@ -1184,16 +1185,20 @@ void
> > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> >  	if (!val || plane->id == PLANE_CURSOR)
> >  		return;
> >  
> > 
> > 
> > 
> > -	val = plane_state->uapi.dst.y1 << 16 | plane_state-
> > > uapi.dst.x1;
> > +	clip = &plane_state->psr2_sel_fetch_area;
> > +
> > +	val = (clip->y1 + plane_state->uapi.dst.y1) << 16;
> 
> > +	val |= plane_state->uapi.dst.x1;
> >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_POS(pipe, plane-
> > > id), val);
> >  
> > 
> > 
> > 
> > -	val = plane_state->color_plane[color_plane].y << 16;
> > +	/* TODO: consider tiling and auxiliary surfaces */
> > +	val = (clip->y1 + plane_state->color_plane[color_plane].y) <<
> > 16;
> >  	val |= plane_state->color_plane[color_plane].x;
> >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_OFFSET(pipe, plane-
> > > id),
> >  			  val);
> >  
> > 
> > 
> > 
> >  	/* Sizes are 0 based */
> > -	val = ((drm_rect_height(&plane_state->uapi.src) >> 16) - 1) <<
> > 16;
> > +	val = (drm_rect_height(clip) - 1) << 16;
> >  	val |= (drm_rect_width(&plane_state->uapi.src) >> 16) - 1;
> >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe, plane-
> > > id), val);
> >  }
> > @@ -1267,7 +1272,7 @@ 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 temp;
> > +		struct drm_rect *sel_fetch_area, temp;
> >  
> > 
> > 
> > 
> >  		if (new_plane_state->uapi.crtc != crtc_state-
> > > uapi.crtc)
> >  			continue;
> > @@ -1290,8 +1295,13 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  		 * For now doing a selective fetch in the whole plane
> > area,
> >  		 * optimizations will come in the future.
> >  		 */
> > -		temp.y1 = new_plane_state->uapi.dst.y1;
> > -		temp.y2 = new_plane_state->uapi.dst.y2;
> > +		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;
> > +
> > +		temp = *sel_fetch_area;
> > +		temp.y1 += new_plane_state->uapi.dst.y1 >> 16;
> > +		temp.y2 += new_plane_state->uapi.dst.y1 >> 16;
> It adds src to dst. 
> For the whole plane damage area, these previous code looks correct.

Nice catch, thanks.

> 
>  temp.y1 = new_plane_state->uapi.dst.y1;
>  temp.y2 = new_plane_state->uapi.dst.y2;
> 
> >  		clip_area_update(&pipe_clip, &temp);
> >  	}
> >  
> > 
> > 
> > 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915/display/psr: Use plane damage clips to calculate damaged area

[Souza, Jose]

On Mon, 2020-10-26 at 21:40 +0000, Mun, Gwan-gyeong wrote:
> On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza 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.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 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 | 54 +++++++++++++++++++++-
> > --
> >  1 file changed, 49 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 773a5b5fa078..0f1e9f0fa57f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1273,6 +1273,9 @@ 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 *sel_fetch_area, temp;
> > +		struct drm_mode_rect *damaged_clips;
> > +		u32 num_clips;
> > +		int j;
> >  
> > 
> > 
> > 
> >  		if (new_plane_state->uapi.crtc != crtc_state-
> > > uapi.crtc)
> >  			continue;
> > @@ -1291,13 +1294,54 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  		if (!new_plane_state->uapi.visible)
> >  			continue;
> >  
> > 
> > 
> > 
> > +		sel_fetch_area = &new_plane_state->psr2_sel_fetch_area;
> > +		sel_fetch_area->y1 = -1;
> > +
> > +		damaged_clips =
> > drm_plane_get_damage_clips(&new_plane_state->uapi);
> > +		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 plane moved, mark the whole plane area as damaged
> > as it
> > +		 * needs to be complete redraw in the new 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 (!drm_rect_equals(&new_plane_state->uapi.dst,
> > +				     &old_plane_state->uapi.dst)) {
> > +			num_clips = 0;
> > +			sel_fetch_area->y1 = new_plane_state-
> > > uapi.src.y1 >> 16;
> > +			sel_fetch_area->y2 = new_plane_state-
> > > uapi.src.y2 >> 16;
> > +		} else if (!num_clips && new_plane_state->uapi.fb !=
> > +			   old_plane_state->uapi.fb) {
> > +			/*
> > +			 * If the plane don't have damage areas but the
> > +			 * framebuffer changed, mark the whole plane
> > area as
> > +			 * damaged.
> > +			 */
> > +			sel_fetch_area->y1 = new_plane_state-
> > > uapi.src.y1 >> 16;
> > +			sel_fetch_area->y2 = new_plane_state-
> > > uapi.src.y2 >> 16;
> > +		}
> > +
> why don't you use drm_atomic_helper_damage_merged() function here?

hum did not knew about this function, will take a look at as it does more than just merge all damaged areas.

> > +		for (j = 0; j < num_clips; j++) {
> > +			struct drm_rect damage_area;
> > +
> > +			damage_area.y1 = damaged_clips[j].y1;
> > +			damage_area.y2 = damaged_clips[j].y2;
> > +			clip_area_update(sel_fetch_area, &damage_area);
> > +		}
> > +
> > +		/*
> > +		 * No page flip, no plane moviment or no damage areas,
> > so don't
> typo (moviment -> movement)

fixed

> > +		 * fetch any pixel from memory for this plane
> > +		 */
> > +		if (sel_fetch_area->y1 == -1) {
> > +			sel_fetch_area->y1 = 0;
> > +			sel_fetch_area->y2 = 0;
> > +		}
> > +
> > +		/* Don't need to redraw plane damaged areas outside of
> > screen */
> > +		j = sel_fetch_area->y2 + (new_plane_state->uapi.dst.y1
> > > > 16);
> src coordinates of the drm_plane_state are 16.16 fixed point but dst
> coordinates are not 16.16 fixed point.
> therefore we don't need to bit shift for dst.
> Because the sel_fetch_area seems based on src coordinates, in order to
> apply to dst coordinates here,  it requires coordinate calculation. 

thanks for catching this, also fixed the same issue patch 1.

> > +		j = crtc_state->uapi.adjusted_mode.crtc_vdisplay - j;
> > +		if (j < 0)
> > +			sel_fetch_area->y2 += j;
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> >  		temp = *sel_fetch_area;
> >  		temp.y1 += new_plane_state->uapi.dst.y1 >> 16;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/6] drm/i915/display/psr: Consider other planes to damaged area calculation

[Mun, Gwan-gyeong]

On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza wrote:
> Planes can individually have transparent, move or have visibility
> changed if any of those happens, planes bellow it will be visible or
> have more pixels of it visible than before.
> 
> This patch is taking care of this case for selective fetch by adding
> to each plane damaged area all the intersections of planes above it
> that matches with the characteristics described above.
> 
> There still some room from improvements here but at least this
> initial
> version will take care of display what is expected saving some memory
> reads.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 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 | 62
> ++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 0f1e9f0fa57f..91ba97bf609b 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1253,11 +1253,38 @@ static void clip_area_update(struct drm_rect
> *overlap_damage_area,
>  		overlap_damage_area->y2 = damage_area->y2;
>  }
>  
> +/* Update plane damage area if planes above moved or have alpha */
> +static void pipe_dirty_areas_set(struct intel_plane_state
> *plane_state,
> +				 struct intel_plane *plane,
> +				 const struct drm_rect
> *pipe_dirty_areas,
> +				 struct drm_rect *sel_fetch_area)
> +{
> +	enum plane_id i;
> +
> +	for (i = PLANE_CURSOR; i > plane->id; i--) {
> +		int j;
> +
> +		for (j = 0; j < 2; j++) {
> +			struct drm_rect r = pipe_dirty_areas[i * 2 +
> j];
> +
> +			if (!drm_rect_width(&r))
> +				continue;
> +			if (!drm_rect_intersect(&r, &plane_state-
> >uapi.dst))
> +				continue;
> +
> +			r.y1 -= plane_state->uapi.dst.y1;
> +			r.y2 -= plane_state->uapi.dst.y1;
typo of y2?
> +			clip_area_update(sel_fetch_area, &r);
sel_fetch_area has plane coordinates, but it tried to apply dst
coordinates.
> +		}
> +	}
> +}
> +
>  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 intel_plane_state *new_plane_state, *old_plane_state;
> +	struct drm_rect pipe_dirty_areas[I915_MAX_PLANES * 2] = {};
>  	struct drm_rect pipe_clip = { .y1 = -1 };
>  	struct intel_plane *plane;
>  	bool full_update = false;
> @@ -1270,6 +1297,38 @@ int intel_psr2_sel_fetch_update(struct
> intel_atomic_state *state,
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Mark all the areas where there is a plane that matches one
> of this:
> +	 * - transparent
> +	 * - moved
> +	 * - visibility changed
> +	 * In all those cases, planes bellow it will need to be redraw.
> +	 */
> +	for_each_oldnew_intel_plane_in_state(state, plane,
> old_plane_state,
> +					     new_plane_state, i) {
> +		bool alpha, flip, dirty;
> +
> +		if (new_plane_state->uapi.crtc != crtc_state-
> >uapi.crtc)
> +			continue;
> +
> +		alpha = new_plane_state->uapi.alpha !=
> DRM_BLEND_ALPHA_OPAQUE;
> +		alpha |= old_plane_state->uapi.alpha !=
> DRM_BLEND_ALPHA_OPAQUE;
> +		flip = new_plane_state->uapi.fb != old_plane_state-
> >uapi.fb;
> +		dirty = alpha && flip;
> +
> +		dirty |= !drm_rect_equals(&new_plane_state->uapi.dst,
> +					  &old_plane_state->uapi.dst);
> +		dirty |= new_plane_state->uapi.visible !=
> +			 old_plane_state->uapi.visible;
> +		if (!dirty)
> +			continue;
> +
> +		if (old_plane_state->uapi.visible)
> +			pipe_dirty_areas[plane->id * 2] =
> old_plane_state->uapi.dst;
> +		if (new_plane_state->uapi.visible)
> +			pipe_dirty_areas[plane->id * 2 + 1] =
> new_plane_state->uapi.dst;
> +	}
> +
>  	for_each_oldnew_intel_plane_in_state(state, plane,
> old_plane_state,
>  					     new_plane_state, i) {
>  		struct drm_rect *sel_fetch_area, temp;
> @@ -1337,6 +1396,9 @@ int intel_psr2_sel_fetch_update(struct
> intel_atomic_state *state,
>  			sel_fetch_area->y2 = 0;
>  		}
>  
> +		pipe_dirty_areas_set(new_plane_state, plane,
> pipe_dirty_areas,
> +				     sel_fetch_area);
> +
In my humble opinion, plane_state's alpha and visibility might affect
to PSR Selective Update, but it would not affect to damage region of
the Plane Selective Fetch program. therefore if we want to calculate
with changing of alpha and visibility, we need to separate the data
structure of clip rects and calculating clip region to "plane's
selective fetch" and "PSR's selective update".
>  		/* Don't need to redraw plane damaged areas outside of
> screen */
>  		j = sel_fetch_area->y2 + (new_plane_state->uapi.dst.y1
> >> 16);
>  		j = crtc_state->uapi.adjusted_mode.crtc_vdisplay - j;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/6] drm/i915/display/psr: Consider other planes to damaged area calculation

[Souza, Jose]

On Tue, 2020-10-27 at 13:34 +0000, Mun, Gwan-gyeong wrote:
> On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza wrote:
> > Planes can individually have transparent, move or have visibility
> > changed if any of those happens, planes bellow it will be visible or
> > have more pixels of it visible than before.
> > 
> > This patch is taking care of this case for selective fetch by adding
> > to each plane damaged area all the intersections of planes above it
> > that matches with the characteristics described above.
> > 
> > There still some room from improvements here but at least this
> > initial
> > version will take care of display what is expected saving some memory
> > reads.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 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 | 62
> > ++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 0f1e9f0fa57f..91ba97bf609b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1253,11 +1253,38 @@ static void clip_area_update(struct drm_rect
> > *overlap_damage_area,
> >  		overlap_damage_area->y2 = damage_area->y2;
> >  }
> >  
> > 
> > 
> > 
> > +/* Update plane damage area if planes above moved or have alpha */
> > +static void pipe_dirty_areas_set(struct intel_plane_state
> > *plane_state,
> > +				 struct intel_plane *plane,
> > +				 const struct drm_rect
> > *pipe_dirty_areas,
> > +				 struct drm_rect *sel_fetch_area)
> > +{
> > +	enum plane_id i;
> > +
> > +	for (i = PLANE_CURSOR; i > plane->id; i--) {
> > +		int j;
> > +
> > +		for (j = 0; j < 2; j++) {
> > +			struct drm_rect r = pipe_dirty_areas[i * 2 +
> > j];
> > +
> > +			if (!drm_rect_width(&r))
> > +				continue;
> > +			if (!drm_rect_intersect(&r, &plane_state-
> > > uapi.dst))
> > +				continue;
> > +
> > +			r.y1 -= plane_state->uapi.dst.y1;
> > +			r.y2 -= plane_state->uapi.dst.y1;
> typo of y2?

Not a typo in this case.

> > +			clip_area_update(sel_fetch_area, &r);
> sel_fetch_area has plane coordinates, but it tried to apply dst
> coordinates.

the subtraction above is converting pipe/dst coordinates to the current plane coordinates. 

> > +		}
> > +	}
> > +}
> > +
> >  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 intel_plane_state *new_plane_state, *old_plane_state;
> > +	struct drm_rect pipe_dirty_areas[I915_MAX_PLANES * 2] = {};
> >  	struct drm_rect pipe_clip = { .y1 = -1 };
> >  	struct intel_plane *plane;
> >  	bool full_update = false;
> > @@ -1270,6 +1297,38 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  	if (ret)
> >  		return ret;
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > +	/*
> > +	 * Mark all the areas where there is a plane that matches one
> > of this:
> > +	 * - transparent
> > +	 * - moved
> > +	 * - visibility changed
> > +	 * In all those cases, planes bellow it will need to be redraw.
> > +	 */
> > +	for_each_oldnew_intel_plane_in_state(state, plane,
> > old_plane_state,
> > +					     new_plane_state, i) {
> > +		bool alpha, flip, dirty;
> > +
> > +		if (new_plane_state->uapi.crtc != crtc_state-
> > > uapi.crtc)
> > +			continue;
> > +
> > +		alpha = new_plane_state->uapi.alpha !=
> > DRM_BLEND_ALPHA_OPAQUE;
> > +		alpha |= old_plane_state->uapi.alpha !=
> > DRM_BLEND_ALPHA_OPAQUE;
> > +		flip = new_plane_state->uapi.fb != old_plane_state-
> > > uapi.fb;
> > +		dirty = alpha && flip;
> > +
> > +		dirty |= !drm_rect_equals(&new_plane_state->uapi.dst,
> > +					  &old_plane_state->uapi.dst);
> > +		dirty |= new_plane_state->uapi.visible !=
> > +			 old_plane_state->uapi.visible;
> > +		if (!dirty)
> > +			continue;
> > +
> > +		if (old_plane_state->uapi.visible)
> > +			pipe_dirty_areas[plane->id * 2] =
> > old_plane_state->uapi.dst;
> > +		if (new_plane_state->uapi.visible)
> > +			pipe_dirty_areas[plane->id * 2 + 1] =
> > new_plane_state->uapi.dst;
> > +	}
> > +
> >  	for_each_oldnew_intel_plane_in_state(state, plane,
> > old_plane_state,
> >  					     new_plane_state, i) {
> >  		struct drm_rect *sel_fetch_area, temp;
> > @@ -1337,6 +1396,9 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  			sel_fetch_area->y2 = 0;
> >  		}
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > +		pipe_dirty_areas_set(new_plane_state, plane,
> > pipe_dirty_areas,
> > +				     sel_fetch_area);
> > +
> In my humble opinion, plane_state's alpha and visibility might affect
> to PSR Selective Update, but it would not affect to damage region of
> the Plane Selective Fetch program. therefore if we want to calculate
> with changing of alpha and visibility, we need to separate the data
> structure of clip rects and calculating clip region to "plane's
> selective fetch" and "PSR's selective update".

Why separate? We need one clip region to program selective fetch plane registers.
Like said in the commit description, this could be optimized in future like to check if pixels changed in the damaged overlap area with a plane with
alpha but that will make things really complicated so for now keeping this more simple approach, compositors are not even supporting damage area yet.

> >  		/* Don't need to redraw plane damaged areas outside of
> > screen */
> >  		j = sel_fetch_area->y2 + (new_plane_state->uapi.dst.y1
> > > > 16);
> >  		j = crtc_state->uapi.adjusted_mode.crtc_vdisplay - j;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915/display/psr: Use plane damage clips to calculate damaged area

[Souza, Jose]

On Tue, 2020-10-27 at 01:04 +0000, Souza, Jose wrote:
> On Mon, 2020-10-26 at 21:40 +0000, Mun, Gwan-gyeong wrote:
> > On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza 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.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 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 | 54 +++++++++++++++++++++-
> > > --
> > >  1 file changed, 49 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 773a5b5fa078..0f1e9f0fa57f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -1273,6 +1273,9 @@ 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 *sel_fetch_area, temp;
> > > +		struct drm_mode_rect *damaged_clips;
> > > +		u32 num_clips;
> > > +		int j;
> > >  
> > > 
> > > 
> > > 
> > >  		if (new_plane_state->uapi.crtc != crtc_state-
> > > > uapi.crtc)
> > >  			continue;
> > > @@ -1291,13 +1294,54 @@ int intel_psr2_sel_fetch_update(struct
> > > intel_atomic_state *state,
> > >  		if (!new_plane_state->uapi.visible)
> > >  			continue;
> > >  
> > > 
> > > 
> > > 
> > > +		sel_fetch_area = &new_plane_state->psr2_sel_fetch_area;
> > > +		sel_fetch_area->y1 = -1;
> > > +
> > > +		damaged_clips =
> > > drm_plane_get_damage_clips(&new_plane_state->uapi);
> > > +		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 plane moved, mark the whole plane area as damaged
> > > as it
> > > +		 * needs to be complete redraw in the new 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 (!drm_rect_equals(&new_plane_state->uapi.dst,
> > > +				     &old_plane_state->uapi.dst)) {
> > > +			num_clips = 0;
> > > +			sel_fetch_area->y1 = new_plane_state-
> > > > uapi.src.y1 >> 16;
> > > +			sel_fetch_area->y2 = new_plane_state-
> > > > uapi.src.y2 >> 16;
> > > +		} else if (!num_clips && new_plane_state->uapi.fb !=
> > > +			   old_plane_state->uapi.fb) {
> > > +			/*
> > > +			 * If the plane don't have damage areas but the
> > > +			 * framebuffer changed, mark the whole plane
> > > area as
> > > +			 * damaged.
> > > +			 */
> > > +			sel_fetch_area->y1 = new_plane_state-
> > > > uapi.src.y1 >> 16;
> > > +			sel_fetch_area->y2 = new_plane_state-
> > > > uapi.src.y2 >> 16;
> > > +		}
> > > +
> > why don't you use drm_atomic_helper_damage_merged() function here?
> 
> hum did not knew about this function, will take a look at as it does more than just merge all damaged areas.

We can't use this function as it marks the whole plane area as damaged if there is no damaged clips.
But for our use case this is bad as we add all planes of the pipe/CRTC to the state, so it would cause a full fetch of the planes without any
flip/modification.

> 
> > > +		for (j = 0; j < num_clips; j++) {
> > > +			struct drm_rect damage_area;
> > > +
> > > +			damage_area.y1 = damaged_clips[j].y1;
> > > +			damage_area.y2 = damaged_clips[j].y2;
> > > +			clip_area_update(sel_fetch_area, &damage_area);
> > > +		}
> > > +
> > > +		/*
> > > +		 * No page flip, no plane moviment or no damage areas,
> > > so don't
> > typo (moviment -> movement)
> 
> fixed
> 
> > > +		 * fetch any pixel from memory for this plane
> > > +		 */
> > > +		if (sel_fetch_area->y1 == -1) {
> > > +			sel_fetch_area->y1 = 0;
> > > +			sel_fetch_area->y2 = 0;
> > > +		}
> > > +
> > > +		/* Don't need to redraw plane damaged areas outside of
> > > screen */
> > > +		j = sel_fetch_area->y2 + (new_plane_state->uapi.dst.y1
> > > > > 16);
> > src coordinates of the drm_plane_state are 16.16 fixed point but dst
> > coordinates are not 16.16 fixed point.
> > therefore we don't need to bit shift for dst.
> > Because the sel_fetch_area seems based on src coordinates, in order to
> > apply to dst coordinates here,  it requires coordinate calculation. 
> 
> thanks for catching this, also fixed the same issue patch 1.
> 
> > > +		j = crtc_state->uapi.adjusted_mode.crtc_vdisplay - j;
> > > +		if (j < 0)
> > > +			sel_fetch_area->y2 += j;
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > >  		temp = *sel_fetch_area;
> > >  		temp.y1 += new_plane_state->uapi.dst.y1 >> 16;
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915/display/psr: Use plane damage clips to calculate damaged area

[Mun, Gwan-gyeong]

On Tue, 2020-10-27 at 13:12 -0700, Souza, Jose wrote:
> On Tue, 2020-10-27 at 01:04 +0000, Souza, Jose wrote:
> > On Mon, 2020-10-26 at 21:40 +0000, Mun, Gwan-gyeong wrote:
> > > On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza 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.
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 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 | 54
> > > > +++++++++++++++++++++-
> > > > --
> > > >  1 file changed, 49 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index 773a5b5fa078..0f1e9f0fa57f 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -1273,6 +1273,9 @@ 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 *sel_fetch_area, temp;
> > > > +		struct drm_mode_rect *damaged_clips;
> > > > +		u32 num_clips;
> > > > +		int j;
> > > >  
> > > > 
> > > > 
> > > > 
> > > >  		if (new_plane_state->uapi.crtc != crtc_state-
> > > > > uapi.crtc)
> > > >  			continue;
selective fetch area also can be affected by Selective Updated region. 
therefore Selective Updated region rect should be calculated first and
then the plane's selective fetch area should be applied (intersected by
calculated SU.)
please check this implementation.
(https://patchwork.freedesktop.org/patch/404264/?series=84340&rev=1)
> > > > @@ -1291,13 +1294,54 @@ int intel_psr2_sel_fetch_update(struct
> > > > intel_atomic_state *state,
> > > >  		if (!new_plane_state->uapi.visible)
> > > >  			continue;
> > > >  
> > > > 
> > > > 
> > > > 
> > > > +		sel_fetch_area = &new_plane_state-
> > > > >psr2_sel_fetch_area;
> > > > +		sel_fetch_area->y1 = -1;
> > > > +
> > > > +		damaged_clips =
> > > > drm_plane_get_damage_clips(&new_plane_state->uapi);
> > > > +		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 plane moved, mark the whole plane area as
> > > > damaged
> > > > as it
> > > > +		 * needs to be complete redraw in the new
> > > > 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 (!drm_rect_equals(&new_plane_state-
> > > > >uapi.dst,
> > > > +				     &old_plane_state-
> > > > >uapi.dst)) {
> > > > +			num_clips = 0;
> > > > +			sel_fetch_area->y1 = new_plane_state-
> > > > > uapi.src.y1 >> 16;
> > > > +			sel_fetch_area->y2 = new_plane_state-
> > > > > uapi.src.y2 >> 16;
> > > > +		} else if (!num_clips && new_plane_state-
> > > > >uapi.fb !=
> > > > +			   old_plane_state->uapi.fb) {
> > > > +			/*
> > > > +			 * If the plane don't have damage areas
> > > > but the
> > > > +			 * framebuffer changed, mark the whole
> > > > plane
> > > > area as
> > > > +			 * damaged.
> > > > +			 */
> > > > +			sel_fetch_area->y1 = new_plane_state-
> > > > > uapi.src.y1 >> 16;
> > > > +			sel_fetch_area->y2 = new_plane_state-
> > > > > uapi.src.y2 >> 16;
> > > > +		}
> > > > +
> > > why don't you use drm_atomic_helper_damage_merged() function
> > > here?
> > 
> > hum did not knew about this function, will take a look at as it
> > does more than just merge all damaged areas.
> 
> We can't use this function as it marks the whole plane area as
> damaged if there is no damaged clips.
> But for our use case this is bad as we add all planes of the
> pipe/CRTC to the state, so it would cause a full fetch of the planes
> without any
> flip/modification.
> 
> > > > +		for (j = 0; j < num_clips; j++) {
> > > > +			struct drm_rect damage_area;
> > > > +
> > > > +			damage_area.y1 = damaged_clips[j].y1;
> > > > +			damage_area.y2 = damaged_clips[j].y2;
> > > > +			clip_area_update(sel_fetch_area,
> > > > &damage_area);
> > > > +		}
> > > > +
> > > > +		/*
> > > > +		 * No page flip, no plane moviment or no damage
> > > > areas,
> > > > so don't
> > > typo (moviment -> movement)
> > 
> > fixed
> > 
> > > > +		 * fetch any pixel from memory for this plane
> > > > +		 */
> > > > +		if (sel_fetch_area->y1 == -1) {
> > > > +			sel_fetch_area->y1 = 0;
> > > > +			sel_fetch_area->y2 = 0;
> > > > +		}
> > > > +
> > > > +		/* Don't need to redraw plane damaged areas
> > > > outside of
> > > > screen */
> > > > +		j = sel_fetch_area->y2 + (new_plane_state-
> > > > >uapi.dst.y1
> > > > > > 16);
> > > src coordinates of the drm_plane_state are 16.16 fixed point but
> > > dst
> > > coordinates are not 16.16 fixed point.
> > > therefore we don't need to bit shift for dst.
> > > Because the sel_fetch_area seems based on src coordinates, in
> > > order to
> > > apply to dst coordinates here,  it requires coordinate
> > > calculation. 
> > 
> > thanks for catching this, also fixed the same issue patch 1.
> > 
> > > > +		j = crtc_state-
> > > > >uapi.adjusted_mode.crtc_vdisplay - j;
> > > > +		if (j < 0)
> > > > +			sel_fetch_area->y2 += j;
> > > >  
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > >  		temp = *sel_fetch_area;
> > > >  		temp.y1 += new_plane_state->uapi.dst.y1 >> 16;
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/6] drm/i915/display/psr: Consider other planes to damaged area calculation

[Mun, Gwan-gyeong]

On Tue, 2020-10-27 at 10:25 -0700, Souza, Jose wrote:
> On Tue, 2020-10-27 at 13:34 +0000, Mun, Gwan-gyeong wrote:
> > On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza wrote:
> > > Planes can individually have transparent, move or have visibility
> > > changed if any of those happens, planes bellow it will be visible
> > > or
> > > have more pixels of it visible than before.
> > > 
> > > This patch is taking care of this case for selective fetch by
> > > adding
> > > to each plane damaged area all the intersections of planes above
> > > it
> > > that matches with the characteristics described above.
> > > 
> > > There still some room from improvements here but at least this
> > > initial
> > > version will take care of display what is expected saving some
> > > memory
> > > reads.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 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 | 62
> > > ++++++++++++++++++++++++
> > >  1 file changed, 62 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 0f1e9f0fa57f..91ba97bf609b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -1253,11 +1253,38 @@ static void clip_area_update(struct
> > > drm_rect
> > > *overlap_damage_area,
> > >  		overlap_damage_area->y2 = damage_area->y2;
> > >  }
> > >  
> > > 
> > > 
> > > 
> > > +/* Update plane damage area if planes above moved or have alpha
> > > */
> > > +static void pipe_dirty_areas_set(struct intel_plane_state
> > > *plane_state,
> > > +				 struct intel_plane *plane,
> > > +				 const struct drm_rect
> > > *pipe_dirty_areas,
> > > +				 struct drm_rect *sel_fetch_area)
> > > +{
> > > +	enum plane_id i;
> > > +
> > > +	for (i = PLANE_CURSOR; i > plane->id; i--) {
> > > +		int j;
> > > +
> > > +		for (j = 0; j < 2; j++) {
> > > +			struct drm_rect r = pipe_dirty_areas[i * 2 +
> > > j];
> > > +
> > > +			if (!drm_rect_width(&r))
> > > +				continue;
> > > +			if (!drm_rect_intersect(&r, &plane_state-
> > > > uapi.dst))
> > > +				continue;
> > > +
> > > +			r.y1 -= plane_state->uapi.dst.y1;
> > > +			r.y2 -= plane_state->uapi.dst.y1;
> > typo of y2?
> 
> Not a typo in this case.
> 
> > > +			clip_area_update(sel_fetch_area, &r);
> > sel_fetch_area has plane coordinates, but it tried to apply dst
> > coordinates.
> 
> the subtraction above is converting pipe/dst coordinates to the
> current plane coordinates. 
> 
> > > +		}
> > > +	}
> > > +}
> > > +
> > >  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 intel_plane_state *new_plane_state, *old_plane_state;
> > > +	struct drm_rect pipe_dirty_areas[I915_MAX_PLANES * 2] = {};
> > >  	struct drm_rect pipe_clip = { .y1 = -1 };
> > >  	struct intel_plane *plane;
> > >  	bool full_update = false;
> > > @@ -1270,6 +1297,38 @@ int intel_psr2_sel_fetch_update(struct
> > > intel_atomic_state *state,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > +	/*
> > > +	 * Mark all the areas where there is a plane that matches one
> > > of this:
> > > +	 * - transparent
> > > +	 * - moved
> > > +	 * - visibility changed
> > > +	 * In all those cases, planes bellow it will need to be redraw.
> > > +	 */
(for future visibility and fully obscured plane checking) we
can  traverse from top to down, by adding  and using
for_each_oldnew_intel_plane_in_state_reverse()
> > > +	for_each_oldnew_intel_plane_in_state(state, plane,
> > > old_plane_state,
> > > +					     new_plane_state, i) {
> > > +		bool alpha, flip, dirty;
> > > +
> > > +		if (new_plane_state->uapi.crtc != crtc_state-
> > > > uapi.crtc)
> > > +			continue;
> > > +
> > > +		alpha = new_plane_state->uapi.alpha !=
> > > DRM_BLEND_ALPHA_OPAQUE;
> > > +		alpha |= old_plane_state->uapi.alpha !=
> > > DRM_BLEND_ALPHA_OPAQUE;
> > > +		flip = new_plane_state->uapi.fb != old_plane_state-
> > > > uapi.fb;
> > > +		dirty = alpha && flip;
> > > +
> > > +		dirty |= !drm_rect_equals(&new_plane_state->uapi.dst,
> > > +					  &old_plane_state->uapi.dst);
> > > +		dirty |= new_plane_state->uapi.visible !=
> > > +			 old_plane_state->uapi.visible;
> > > +		if (!dirty)
> > > +			continue;
> > > +
if we can calculate SU region here, we don't need to store all of the
dirty areas.
> > > +		if (old_plane_state->uapi.visible)
> > > +			pipe_dirty_areas[plane->id * 2] =
> > > old_plane_state->uapi.dst;
> > > +		if (new_plane_state->uapi.visible)
> > > +			pipe_dirty_areas[plane->id * 2 + 1] =
> > > new_plane_state->uapi.dst;
> > > +	}
> > > +
> > >  	for_each_oldnew_intel_plane_in_state(state, plane,
> > > old_plane_state,
> > >  					     new_plane_state, i) {
> > >  		struct drm_rect *sel_fetch_area, temp;
> > > @@ -1337,6 +1396,9 @@ int intel_psr2_sel_fetch_update(struct
> > > intel_atomic_state *state,
> > >  			sel_fetch_area->y2 = 0;
> > >  		}
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > +		pipe_dirty_areas_set(new_plane_state, plane,
> > > pipe_dirty_areas,
> > > +				     sel_fetch_area);
> > > +
> > In my humble opinion, plane_state's alpha and visibility might
> > affect
> > to PSR Selective Update, but it would not affect to damage region
> > of
> > the Plane Selective Fetch program. therefore if we want to
> > calculate
> > with changing of alpha and visibility, we need to separate the data
> > structure of clip rects and calculating clip region to "plane's
> > selective fetch" and "PSR's selective update".
> 
> Why separate? We need one clip region to program selective fetch
> plane registers.
> Like said in the commit description, this could be optimized in
> future like to check if pixels changed in the damaged overlap area
> with a plane with
> alpha but that will make things really complicated so for now keeping
> this more simple approach, compositors are not even supporting damage
> area yet.
weston (wayland reference compositor) supports damage area feature.
> 
> > >  		/* Don't need to redraw plane damaged areas outside of
> > > screen */
> > >  		j = sel_fetch_area->y2 + (new_plane_state->uapi.dst.y1
> > > > > 16);
> > >  		j = crtc_state->uapi.adjusted_mode.crtc_vdisplay - j;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915/display/psr: Use plane damage clips to calculate damaged area

[Souza, Jose]

On Tue, 2020-12-01 at 17:26 +0000, Mun, Gwan-gyeong wrote:
> On Tue, 2020-10-27 at 13:12 -0700, Souza, Jose wrote:
> > On Tue, 2020-10-27 at 01:04 +0000, Souza, Jose wrote:
> > > On Mon, 2020-10-26 at 21:40 +0000, Mun, Gwan-gyeong wrote:
> > > > On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza 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.
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 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 | 54
> > > > > +++++++++++++++++++++-
> > > > > --
> > > > >  1 file changed, 49 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > index 773a5b5fa078..0f1e9f0fa57f 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > @@ -1273,6 +1273,9 @@ 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 *sel_fetch_area, temp;
> > > > > +		struct drm_mode_rect *damaged_clips;
> > > > > +		u32 num_clips;
> > > > > +		int j;
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > >  		if (new_plane_state->uapi.crtc != crtc_state-
> > > > > > uapi.crtc)
> > > > >  			continue;
> selective fetch area also can be affected by Selective Updated region. 
> therefore Selective Updated region rect should be calculated first and
> then the plane's selective fetch area should be applied (intersected by
> calculated SU.)
> please check this implementation.

Why selective update region needs to be calculate first if it should be based on the plane damage areas/selective fetch areas?
Can you give a example where it gives not the expected result?


> (https://patchwork.freedesktop.org/patch/404264/?series=84340&rev=1)
> > > > > @@ -1291,13 +1294,54 @@ int intel_psr2_sel_fetch_update(struct
> > > > > intel_atomic_state *state,
> > > > >  		if (!new_plane_state->uapi.visible)
> > > > >  			continue;
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > +		sel_fetch_area = &new_plane_state-
> > > > > > psr2_sel_fetch_area;
> > > > > +		sel_fetch_area->y1 = -1;
> > > > > +
> > > > > +		damaged_clips =
> > > > > drm_plane_get_damage_clips(&new_plane_state->uapi);
> > > > > +		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 plane moved, mark the whole plane area as
> > > > > damaged
> > > > > as it
> > > > > +		 * needs to be complete redraw in the new
> > > > > 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 (!drm_rect_equals(&new_plane_state-
> > > > > > uapi.dst,
> > > > > +				     &old_plane_state-
> > > > > > uapi.dst)) {
> > > > > +			num_clips = 0;
> > > > > +			sel_fetch_area->y1 = new_plane_state-
> > > > > > uapi.src.y1 >> 16;
> > > > > +			sel_fetch_area->y2 = new_plane_state-
> > > > > > uapi.src.y2 >> 16;
> > > > > +		} else if (!num_clips && new_plane_state-
> > > > > > uapi.fb !=
> > > > > +			   old_plane_state->uapi.fb) {
> > > > > +			/*
> > > > > +			 * If the plane don't have damage areas
> > > > > but the
> > > > > +			 * framebuffer changed, mark the whole
> > > > > plane
> > > > > area as
> > > > > +			 * damaged.
> > > > > +			 */
> > > > > +			sel_fetch_area->y1 = new_plane_state-
> > > > > > uapi.src.y1 >> 16;
> > > > > +			sel_fetch_area->y2 = new_plane_state-
> > > > > > uapi.src.y2 >> 16;
> > > > > +		}
> > > > > +
> > > > why don't you use drm_atomic_helper_damage_merged() function
> > > > here?
> > > 
> > > hum did not knew about this function, will take a look at as it
> > > does more than just merge all damaged areas.
> > 
> > We can't use this function as it marks the whole plane area as
> > damaged if there is no damaged clips.
> > But for our use case this is bad as we add all planes of the
> > pipe/CRTC to the state, so it would cause a full fetch of the planes
> > without any
> > flip/modification.
> > 
> > > > > +		for (j = 0; j < num_clips; j++) {
> > > > > +			struct drm_rect damage_area;
> > > > > +
> > > > > +			damage_area.y1 = damaged_clips[j].y1;
> > > > > +			damage_area.y2 = damaged_clips[j].y2;
> > > > > +			clip_area_update(sel_fetch_area,
> > > > > &damage_area);
> > > > > +		}
> > > > > +
> > > > > +		/*
> > > > > +		 * No page flip, no plane moviment or no damage
> > > > > areas,
> > > > > so don't
> > > > typo (moviment -> movement)
> > > 
> > > fixed
> > > 
> > > > > +		 * fetch any pixel from memory for this plane
> > > > > +		 */
> > > > > +		if (sel_fetch_area->y1 == -1) {
> > > > > +			sel_fetch_area->y1 = 0;
> > > > > +			sel_fetch_area->y2 = 0;
> > > > > +		}
> > > > > +
> > > > > +		/* Don't need to redraw plane damaged areas
> > > > > outside of
> > > > > screen */
> > > > > +		j = sel_fetch_area->y2 + (new_plane_state-
> > > > > > uapi.dst.y1
> > > > > > > 16);
> > > > src coordinates of the drm_plane_state are 16.16 fixed point but
> > > > dst
> > > > coordinates are not 16.16 fixed point.
> > > > therefore we don't need to bit shift for dst.
> > > > Because the sel_fetch_area seems based on src coordinates, in
> > > > order to
> > > > apply to dst coordinates here,  it requires coordinate
> > > > calculation. 
> > > 
> > > thanks for catching this, also fixed the same issue patch 1.
> > > 
> > > > > +		j = crtc_state-
> > > > > > uapi.adjusted_mode.crtc_vdisplay - j;
> > > > > +		if (j < 0)
> > > > > +			sel_fetch_area->y2 += j;
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > >  		temp = *sel_fetch_area;
> > > > >  		temp.y1 += new_plane_state->uapi.dst.y1 >> 16;
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/6] drm/i915/display/psr: Consider other planes to damaged area calculation

[Souza, Jose]

On Tue, 2020-12-01 at 17:33 +0000, Mun, Gwan-gyeong wrote:
> On Tue, 2020-10-27 at 10:25 -0700, Souza, Jose wrote:
> > On Tue, 2020-10-27 at 13:34 +0000, Mun, Gwan-gyeong wrote:
> > > On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza wrote:
> > > > Planes can individually have transparent, move or have visibility
> > > > changed if any of those happens, planes bellow it will be visible
> > > > or
> > > > have more pixels of it visible than before.
> > > > 
> > > > This patch is taking care of this case for selective fetch by
> > > > adding
> > > > to each plane damaged area all the intersections of planes above
> > > > it
> > > > that matches with the characteristics described above.
> > > > 
> > > > There still some room from improvements here but at least this
> > > > initial
> > > > version will take care of display what is expected saving some
> > > > memory
> > > > reads.
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 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 | 62
> > > > ++++++++++++++++++++++++
> > > >  1 file changed, 62 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index 0f1e9f0fa57f..91ba97bf609b 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -1253,11 +1253,38 @@ static void clip_area_update(struct
> > > > drm_rect
> > > > *overlap_damage_area,
> > > >  		overlap_damage_area->y2 = damage_area->y2;
> > > >  }
> > > >  
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > +/* Update plane damage area if planes above moved or have alpha
> > > > */
> > > > +static void pipe_dirty_areas_set(struct intel_plane_state
> > > > *plane_state,
> > > > +				 struct intel_plane *plane,
> > > > +				 const struct drm_rect
> > > > *pipe_dirty_areas,
> > > > +				 struct drm_rect *sel_fetch_area)
> > > > +{
> > > > +	enum plane_id i;
> > > > +
> > > > +	for (i = PLANE_CURSOR; i > plane->id; i--) {
> > > > +		int j;
> > > > +
> > > > +		for (j = 0; j < 2; j++) {
> > > > +			struct drm_rect r = pipe_dirty_areas[i * 2 +
> > > > j];
> > > > +
> > > > +			if (!drm_rect_width(&r))
> > > > +				continue;
> > > > +			if (!drm_rect_intersect(&r, &plane_state-
> > > > > uapi.dst))
> > > > +				continue;
> > > > +
> > > > +			r.y1 -= plane_state->uapi.dst.y1;
> > > > +			r.y2 -= plane_state->uapi.dst.y1;
> > > typo of y2?
> > 
> > Not a typo in this case.
> > 
> > > > +			clip_area_update(sel_fetch_area, &r);
> > > sel_fetch_area has plane coordinates, but it tried to apply dst
> > > coordinates.
> > 
> > the subtraction above is converting pipe/dst coordinates to the
> > current plane coordinates. 
> > 
> > > > +		}
> > > > +	}
> > > > +}
> > > > +
> > > >  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 intel_plane_state *new_plane_state, *old_plane_state;
> > > > +	struct drm_rect pipe_dirty_areas[I915_MAX_PLANES * 2] = {};
> > > >  	struct drm_rect pipe_clip = { .y1 = -1 };
> > > >  	struct intel_plane *plane;
> > > >  	bool full_update = false;
> > > > @@ -1270,6 +1297,38 @@ int intel_psr2_sel_fetch_update(struct
> > > > intel_atomic_state *state,
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > +	/*
> > > > +	 * Mark all the areas where there is a plane that matches one
> > > > of this:
> > > > +	 * - transparent
> > > > +	 * - moved
> > > > +	 * - visibility changed
> > > > +	 * In all those cases, planes bellow it will need to be redraw.
> > > > +	 */
> (for future visibility and fully obscured plane checking) we
> can  traverse from top to down, by adding  and using
> for_each_oldnew_intel_plane_in_state_reverse()
> > > > +	for_each_oldnew_intel_plane_in_state(state, plane,
> > > > old_plane_state,
> > > > +					     new_plane_state, i) {
> > > > +		bool alpha, flip, dirty;
> > > > +
> > > > +		if (new_plane_state->uapi.crtc != crtc_state-
> > > > > uapi.crtc)
> > > > +			continue;
> > > > +
> > > > +		alpha = new_plane_state->uapi.alpha !=
> > > > DRM_BLEND_ALPHA_OPAQUE;
> > > > +		alpha |= old_plane_state->uapi.alpha !=
> > > > DRM_BLEND_ALPHA_OPAQUE;
> > > > +		flip = new_plane_state->uapi.fb != old_plane_state-
> > > > > uapi.fb;
> > > > +		dirty = alpha && flip;
> > > > +
> > > > +		dirty |= !drm_rect_equals(&new_plane_state->uapi.dst,
> > > > +					  &old_plane_state->uapi.dst);
> > > > +		dirty |= new_plane_state->uapi.visible !=
> > > > +			 old_plane_state->uapi.visible;
> > > > +		if (!dirty)
> > > > +			continue;
> > > > +
> if we can calculate SU region here, we don't need to store all of the
> dirty areas.

If the planes bellow don't have damaged areas or if it don't have any overlap with other planes we should not include it do the SU region to save
power.

> > > > +		if (old_plane_state->uapi.visible)
> > > > +			pipe_dirty_areas[plane->id * 2] =
> > > > old_plane_state->uapi.dst;
> > > > +		if (new_plane_state->uapi.visible)
> > > > +			pipe_dirty_areas[plane->id * 2 + 1] =
> > > > new_plane_state->uapi.dst;
> > > > +	}
> > > > +
> > > >  	for_each_oldnew_intel_plane_in_state(state, plane,
> > > > old_plane_state,
> > > >  					     new_plane_state, i) {
> > > >  		struct drm_rect *sel_fetch_area, temp;
> > > > @@ -1337,6 +1396,9 @@ int intel_psr2_sel_fetch_update(struct
> > > > intel_atomic_state *state,
> > > >  			sel_fetch_area->y2 = 0;
> > > >  		}
> > > >  
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > +		pipe_dirty_areas_set(new_plane_state, plane,
> > > > pipe_dirty_areas,
> > > > +				     sel_fetch_area);
> > > > +
> > > In my humble opinion, plane_state's alpha and visibility might
> > > affect
> > > to PSR Selective Update, but it would not affect to damage region
> > > of
> > > the Plane Selective Fetch program. therefore if we want to
> > > calculate
> > > with changing of alpha and visibility, we need to separate the data
> > > structure of clip rects and calculating clip region to "plane's
> > > selective fetch" and "PSR's selective update".
> > 
> > Why separate? We need one clip region to program selective fetch
> > plane registers.
> > Like said in the commit description, this could be optimized in
> > future like to check if pixels changed in the damaged overlap area
> > with a plane with
> > alpha but that will make things really complicated so for now keeping
> > this more simple approach, compositors are not even supporting damage
> > area yet.
> weston (wayland reference compositor) supports damage area feature.

Will try it, anything need to be enabled or configure to this to work?
Any instructions that you can share will save time for me.

> > 
> > > >  		/* Don't need to redraw plane damaged areas outside of
> > > > screen */
> > > >  		j = sel_fetch_area->y2 + (new_plane_state->uapi.dst.y1
> > > > > > 16);
> > > >  		j = crtc_state->uapi.adjusted_mode.crtc_vdisplay - j;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915/display/psr: Use plane damage clips to calculate damaged area

[Mun, Gwan-gyeong]

On Tue, 2020-12-01 at 09:39 -0800, Souza, Jose wrote:
> On Tue, 2020-12-01 at 17:26 +0000, Mun, Gwan-gyeong wrote:
> > On Tue, 2020-10-27 at 13:12 -0700, Souza, Jose wrote:
> > > On Tue, 2020-10-27 at 01:04 +0000, Souza, Jose wrote:
> > > > On Mon, 2020-10-26 at 21:40 +0000, Mun, Gwan-gyeong wrote:
> > > > > On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza
> > > > > 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.
> > > > > > 
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > 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 | 54
> > > > > > +++++++++++++++++++++-
> > > > > > --
> > > > > >  1 file changed, 49 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > index 773a5b5fa078..0f1e9f0fa57f 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > @@ -1273,6 +1273,9 @@ 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 *sel_fetch_area, temp;
> > > > > > +		struct drm_mode_rect *damaged_clips;
> > > > > > +		u32 num_clips;
> > > > > > +		int j;
> > > > > >  
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > >  		if (new_plane_state->uapi.crtc != crtc_state-
> > > > > > > uapi.crtc)
> > > > > >  			continue;
> > selective fetch area also can be affected by Selective Updated
> > region. 
> > therefore Selective Updated region rect should be calculated first
> > and
> > then the plane's selective fetch area should be applied
> > (intersected by
> > calculated SU.)
> > please check this implementation.
> 
> Why selective update region needs to be calculate first if it should
> be based on the plane damage areas/selective fetch areas?
> Can you give a example where it gives not the expected result?
> 
I drew the example here : 
https://docs.google.com/presentation/d/1MI20q_3GublGYsY2TDheRncOQsbIjMn20aQ99loaoFg/edit?usp=sharing

> 
> > (https://patchwork.freedesktop.org/patch/404264/?series=84340&rev=1
> > )
> > > > > > @@ -1291,13 +1294,54 @@ int
> > > > > > intel_psr2_sel_fetch_update(struct
> > > > > > intel_atomic_state *state,
> > > > > >  		if (!new_plane_state->uapi.visible)
> > > > > >  			continue;
> > > > > >  
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +		sel_fetch_area = &new_plane_state-
> > > > > > > psr2_sel_fetch_area;
> > > > > > +		sel_fetch_area->y1 = -1;
> > > > > > +
> > > > > > +		damaged_clips =
> > > > > > drm_plane_get_damage_clips(&new_plane_state->uapi);
> > > > > > +		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 plane moved, mark the whole plane area as
> > > > > > damaged
> > > > > > as it
> > > > > > +		 * needs to be complete redraw in the new
> > > > > > 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 (!drm_rect_equals(&new_plane_state-
> > > > > > > uapi.dst,
> > > > > > +				     &old_plane_state-
> > > > > > > uapi.dst)) {
> > > > > > +			num_clips = 0;
> > > > > > +			sel_fetch_area->y1 = new_plane_state-
> > > > > > > uapi.src.y1 >> 16;
> > > > > > +			sel_fetch_area->y2 = new_plane_state-
> > > > > > > uapi.src.y2 >> 16;
> > > > > > +		} else if (!num_clips && new_plane_state-
> > > > > > > uapi.fb !=
> > > > > > +			   old_plane_state->uapi.fb) {
> > > > > > +			/*
> > > > > > +			 * If the plane don't have damage areas
> > > > > > but the
> > > > > > +			 * framebuffer changed, mark the whole
> > > > > > plane
> > > > > > area as
> > > > > > +			 * damaged.
> > > > > > +			 */
> > > > > > +			sel_fetch_area->y1 = new_plane_state-
> > > > > > > uapi.src.y1 >> 16;
> > > > > > +			sel_fetch_area->y2 = new_plane_state-
> > > > > > > uapi.src.y2 >> 16;
> > > > > > +		}
> > > > > > +
> > > > > why don't you use drm_atomic_helper_damage_merged() function
> > > > > here?
> > > > 
> > > > hum did not knew about this function, will take a look at as it
> > > > does more than just merge all damaged areas.
> > > 
> > > We can't use this function as it marks the whole plane area as
> > > damaged if there is no damaged clips.
> > > But for our use case this is bad as we add all planes of the
> > > pipe/CRTC to the state, so it would cause a full fetch of the
> > > planes
> > > without any
> > > flip/modification.
> > > 
> > > > > > +		for (j = 0; j < num_clips; j++) {
> > > > > > +			struct drm_rect damage_area;
> > > > > > +
> > > > > > +			damage_area.y1 = damaged_clips[j].y1;
> > > > > > +			damage_area.y2 = damaged_clips[j].y2;
> > > > > > +			clip_area_update(sel_fetch_area,
> > > > > > &damage_area);
> > > > > > +		}
> > > > > > +
> > > > > > +		/*
> > > > > > +		 * No page flip, no plane moviment or no damage
> > > > > > areas,
> > > > > > so don't
> > > > > typo (moviment -> movement)
> > > > 
> > > > fixed
> > > > 
> > > > > > +		 * fetch any pixel from memory for this plane
> > > > > > +		 */
> > > > > > +		if (sel_fetch_area->y1 == -1) {
> > > > > > +			sel_fetch_area->y1 = 0;
> > > > > > +			sel_fetch_area->y2 = 0;
> > > > > > +		}
> > > > > > +
> > > > > > +		/* Don't need to redraw plane damaged areas
> > > > > > outside of
> > > > > > screen */
> > > > > > +		j = sel_fetch_area->y2 + (new_plane_state-
> > > > > > > uapi.dst.y1
> > > > > > > > 16);
> > > > > src coordinates of the drm_plane_state are 16.16 fixed point
> > > > > but
> > > > > dst
> > > > > coordinates are not 16.16 fixed point.
> > > > > therefore we don't need to bit shift for dst.
> > > > > Because the sel_fetch_area seems based on src coordinates, in
> > > > > order to
> > > > > apply to dst coordinates here,  it requires coordinate
> > > > > calculation. 
> > > > 
> > > > thanks for catching this, also fixed the same issue patch 1.
> > > > 
> > > > > > +		j = crtc_state-
> > > > > > > uapi.adjusted_mode.crtc_vdisplay - j;
> > > > > > +		if (j < 0)
> > > > > > +			sel_fetch_area->y2 += j;
> > > > > >  
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > >  		temp = *sel_fetch_area;
> > > > > >  		temp.y1 += new_plane_state->uapi.dst.y1 >> 16;
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/6] drm/i915/display/psr: Consider other planes to damaged area calculation

[Mun, Gwan-gyeong]

On Tue, 2020-12-01 at 09:44 -0800, Souza, Jose wrote:
> On Tue, 2020-12-01 at 17:33 +0000, Mun, Gwan-gyeong wrote:
> > On Tue, 2020-10-27 at 10:25 -0700, Souza, Jose wrote:
> > > On Tue, 2020-10-27 at 13:34 +0000, Mun, Gwan-gyeong wrote:
> > > > On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza wrote:
> > > > > Planes can individually have transparent, move or have
> > > > > visibility
> > > > > changed if any of those happens, planes bellow it will be
> > > > > visible
> > > > > or
> > > > > have more pixels of it visible than before.
> > > > > 
> > > > > This patch is taking care of this case for selective fetch by
> > > > > adding
> > > > > to each plane damaged area all the intersections of planes
> > > > > above
> > > > > it
> > > > > that matches with the characteristics described above.
> > > > > 
> > > > > There still some room from improvements here but at least
> > > > > this
> > > > > initial
> > > > > version will take care of display what is expected saving
> > > > > some
> > > > > memory
> > > > > reads.
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 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 | 62
> > > > > ++++++++++++++++++++++++
> > > > >  1 file changed, 62 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > index 0f1e9f0fa57f..91ba97bf609b 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > @@ -1253,11 +1253,38 @@ static void clip_area_update(struct
> > > > > drm_rect
> > > > > *overlap_damage_area,
> > > > >  		overlap_damage_area->y2 = damage_area->y2;
> > > > >  }
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > +/* Update plane damage area if planes above moved or have
> > > > > alpha
> > > > > */
> > > > > +static void pipe_dirty_areas_set(struct intel_plane_state
> > > > > *plane_state,
> > > > > +				 struct intel_plane *plane,
> > > > > +				 const struct drm_rect
> > > > > *pipe_dirty_areas,
> > > > > +				 struct drm_rect
> > > > > *sel_fetch_area)
> > > > > +{
> > > > > +	enum plane_id i;
> > > > > +
> > > > > +	for (i = PLANE_CURSOR; i > plane->id; i--) {
> > > > > +		int j;
> > > > > +
> > > > > +		for (j = 0; j < 2; j++) {
> > > > > +			struct drm_rect r = pipe_dirty_areas[i
> > > > > * 2 +
> > > > > j];
> > > > > +
> > > > > +			if (!drm_rect_width(&r))
> > > > > +				continue;
> > > > > +			if (!drm_rect_intersect(&r,
> > > > > &plane_state-
> > > > > > uapi.dst))
> > > > > +				continue;
> > > > > +
> > > > > +			r.y1 -= plane_state->uapi.dst.y1;
> > > > > +			r.y2 -= plane_state->uapi.dst.y1;
> > > > typo of y2?
> > > 
> > > Not a typo in this case.
> > > 
> > > > > +			clip_area_update(sel_fetch_area, &r);
> > > > sel_fetch_area has plane coordinates, but it tried to apply dst
> > > > coordinates.
> > > 
> > > the subtraction above is converting pipe/dst coordinates to the
> > > current plane coordinates. 
> > > 
> > > > > +		}
> > > > > +	}
> > > > > +}
> > > > > +
> > > > >  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 intel_plane_state *new_plane_state,
> > > > > *old_plane_state;
> > > > > +	struct drm_rect pipe_dirty_areas[I915_MAX_PLANES * 2] =
> > > > > {};
> > > > >  	struct drm_rect pipe_clip = { .y1 = -1 };
> > > > >  	struct intel_plane *plane;
> > > > >  	bool full_update = false;
> > > > > @@ -1270,6 +1297,38 @@ int intel_psr2_sel_fetch_update(struct
> > > > > intel_atomic_state *state,
> > > > >  	if (ret)
> > > > >  		return ret;
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > +	/*
> > > > > +	 * Mark all the areas where there is a plane that
> > > > > matches one
> > > > > of this:
> > > > > +	 * - transparent
> > > > > +	 * - moved
> > > > > +	 * - visibility changed
> > > > > +	 * In all those cases, planes bellow it will need to be
> > > > > redraw.
> > > > > +	 */
> > (for future visibility and fully obscured plane checking) we
> > can  traverse from top to down, by adding  and using
> > for_each_oldnew_intel_plane_in_state_reverse()
> > > > > +	for_each_oldnew_intel_plane_in_state(state, plane,
> > > > > old_plane_state,
> > > > > +					     new_plane_state,
> > > > > i) {
> > > > > +		bool alpha, flip, dirty;
> > > > > +
> > > > > +		if (new_plane_state->uapi.crtc != crtc_state-
> > > > > > uapi.crtc)
> > > > > +			continue;
> > > > > +
> > > > > +		alpha = new_plane_state->uapi.alpha !=
> > > > > DRM_BLEND_ALPHA_OPAQUE;
> > > > > +		alpha |= old_plane_state->uapi.alpha !=
> > > > > DRM_BLEND_ALPHA_OPAQUE;
> > > > > +		flip = new_plane_state->uapi.fb !=
> > > > > old_plane_state-
> > > > > > uapi.fb;
> > > > > +		dirty = alpha && flip;
> > > > > +
> > > > > +		dirty |= !drm_rect_equals(&new_plane_state-
> > > > > >uapi.dst,
> > > > > +					  &old_plane_state-
> > > > > >uapi.dst);
> > > > > +		dirty |= new_plane_state->uapi.visible !=
> > > > > +			 old_plane_state->uapi.visible;
> > > > > +		if (!dirty)
> > > > > +			continue;
> > > > > +
> > if we can calculate SU region here, we don't need to store all of
> > the
> > dirty areas.
> 
> If the planes bellow don't have damaged areas or if it don't have any
> overlap with other planes we should not include it do the SU region
> to save
> power.
> 
If you traverse to down from top and calculate visible area, we can
calculate without storing all of dirty areas.

> > > > > +		if (old_plane_state->uapi.visible)
> > > > > +			pipe_dirty_areas[plane->id * 2] =
> > > > > old_plane_state->uapi.dst;
> > > > > +		if (new_plane_state->uapi.visible)
> > > > > +			pipe_dirty_areas[plane->id * 2 + 1] =
> > > > > new_plane_state->uapi.dst;
> > > > > +	}
> > > > > +
> > > > >  	for_each_oldnew_intel_plane_in_state(state, plane,
> > > > > old_plane_state,
> > > > >  					     new_plane_state,
> > > > > i) {
> > > > >  		struct drm_rect *sel_fetch_area, temp;
> > > > > @@ -1337,6 +1396,9 @@ int intel_psr2_sel_fetch_update(struct
> > > > > intel_atomic_state *state,
> > > > >  			sel_fetch_area->y2 = 0;
> > > > >  		}
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > +		pipe_dirty_areas_set(new_plane_state, plane,
> > > > > pipe_dirty_areas,
> > > > > +				     sel_fetch_area);
> > > > > +
> > > > In my humble opinion, plane_state's alpha and visibility might
> > > > affect
> > > > to PSR Selective Update, but it would not affect to damage
> > > > region
> > > > of
> > > > the Plane Selective Fetch program. therefore if we want to
> > > > calculate
> > > > with changing of alpha and visibility, we need to separate the
> > > > data
> > > > structure of clip rects and calculating clip region to "plane's
> > > > selective fetch" and "PSR's selective update".
> > > 
> > > Why separate? We need one clip region to program selective fetch
> > > plane registers.
> > > Like said in the commit description, this could be optimized in
> > > future like to check if pixels changed in the damaged overlap
> > > area
> > > with a plane with
> > > alpha but that will make things really complicated so for now
> > > keeping
> > > this more simple approach, compositors are not even supporting
> > > damage
> > > area yet.
> > weston (wayland reference compositor) supports damage area feature.
> 
> Will try it, anything need to be enabled or configure to this to
> work?
> Any instructions that you can share will save time for me.
> 
> > > > >  		/* Don't need to redraw plane damaged areas
> > > > > outside of
> > > > > screen */
> > > > >  		j = sel_fetch_area->y2 + (new_plane_state-
> > > > > >uapi.dst.y1
> > > > > > > 16);
> > > > >  		j = crtc_state-
> > > > > >uapi.adjusted_mode.crtc_vdisplay - j;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915/display/psr: Use plane damage clips to calculate damaged area

[Souza, Jose]

On Tue, 2020-12-01 at 19:40 +0000, Mun, Gwan-gyeong wrote:
> On Tue, 2020-12-01 at 09:39 -0800, Souza, Jose wrote:
> > On Tue, 2020-12-01 at 17:26 +0000, Mun, Gwan-gyeong wrote:
> > > On Tue, 2020-10-27 at 13:12 -0700, Souza, Jose wrote:
> > > > On Tue, 2020-10-27 at 01:04 +0000, Souza, Jose wrote:
> > > > > On Mon, 2020-10-26 at 21:40 +0000, Mun, Gwan-gyeong wrote:
> > > > > > On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza
> > > > > > 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.
> > > > > > > 
> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > 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 | 54
> > > > > > > +++++++++++++++++++++-
> > > > > > > --
> > > > > > >  1 file changed, 49 insertions(+), 5 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > index 773a5b5fa078..0f1e9f0fa57f 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > @@ -1273,6 +1273,9 @@ 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 *sel_fetch_area, temp;
> > > > > > > +		struct drm_mode_rect *damaged_clips;
> > > > > > > +		u32 num_clips;
> > > > > > > +		int j;
> > > > > > >  
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > >  		if (new_plane_state->uapi.crtc != crtc_state-
> > > > > > > > uapi.crtc)
> > > > > > >  			continue;
> > > selective fetch area also can be affected by Selective Updated
> > > region. 
> > > therefore Selective Updated region rect should be calculated first
> > > and
> > > then the plane's selective fetch area should be applied
> > > (intersected by
> > > calculated SU.)
> > > please check this implementation.
> > 
> > Why selective update region needs to be calculate first if it should
> > be based on the plane damage areas/selective fetch areas?
> > Can you give a example where it gives not the expected result?
> > 
> I drew the example here : 
> https://docs.google.com/presentation/d/1MI20q_3GublGYsY2TDheRncOQsbIjMn20aQ99loaoFg/edit?usp=sharing

Thanks so much for the drawing, now I got what you were talking about.
This point is not very clear in BSpec but you are right we need to set plane selective fetch area using the whole area damaged in pipe.

Can you take a look at the last patch in this branch? https://github.com/zehortigoza/linux/tree/psr2-sel-fetch-part3-v4
Will do some testing and squash into the right commits and resend in the next couple of the days.

> 
> > 
> > > (https://patchwork.freedesktop.org/patch/404264/?series=84340&rev=1
> > > )
> > > > > > > @@ -1291,13 +1294,54 @@ int
> > > > > > > intel_psr2_sel_fetch_update(struct
> > > > > > > intel_atomic_state *state,
> > > > > > >  		if (!new_plane_state->uapi.visible)
> > > > > > >  			continue;
> > > > > > >  
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > +		sel_fetch_area = &new_plane_state-
> > > > > > > > psr2_sel_fetch_area;
> > > > > > > +		sel_fetch_area->y1 = -1;
> > > > > > > +
> > > > > > > +		damaged_clips =
> > > > > > > drm_plane_get_damage_clips(&new_plane_state->uapi);
> > > > > > > +		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 plane moved, mark the whole plane area as
> > > > > > > damaged
> > > > > > > as it
> > > > > > > +		 * needs to be complete redraw in the new
> > > > > > > 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 (!drm_rect_equals(&new_plane_state-
> > > > > > > > uapi.dst,
> > > > > > > +				     &old_plane_state-
> > > > > > > > uapi.dst)) {
> > > > > > > +			num_clips = 0;
> > > > > > > +			sel_fetch_area->y1 = new_plane_state-
> > > > > > > > uapi.src.y1 >> 16;
> > > > > > > +			sel_fetch_area->y2 = new_plane_state-
> > > > > > > > uapi.src.y2 >> 16;
> > > > > > > +		} else if (!num_clips && new_plane_state-
> > > > > > > > uapi.fb !=
> > > > > > > +			   old_plane_state->uapi.fb) {
> > > > > > > +			/*
> > > > > > > +			 * If the plane don't have damage areas
> > > > > > > but the
> > > > > > > +			 * framebuffer changed, mark the whole
> > > > > > > plane
> > > > > > > area as
> > > > > > > +			 * damaged.
> > > > > > > +			 */
> > > > > > > +			sel_fetch_area->y1 = new_plane_state-
> > > > > > > > uapi.src.y1 >> 16;
> > > > > > > +			sel_fetch_area->y2 = new_plane_state-
> > > > > > > > uapi.src.y2 >> 16;
> > > > > > > +		}
> > > > > > > +
> > > > > > why don't you use drm_atomic_helper_damage_merged() function
> > > > > > here?
> > > > > 
> > > > > hum did not knew about this function, will take a look at as it
> > > > > does more than just merge all damaged areas.
> > > > 
> > > > We can't use this function as it marks the whole plane area as
> > > > damaged if there is no damaged clips.
> > > > But for our use case this is bad as we add all planes of the
> > > > pipe/CRTC to the state, so it would cause a full fetch of the
> > > > planes
> > > > without any
> > > > flip/modification.
> > > > 
> > > > > > > +		for (j = 0; j < num_clips; j++) {
> > > > > > > +			struct drm_rect damage_area;
> > > > > > > +
> > > > > > > +			damage_area.y1 = damaged_clips[j].y1;
> > > > > > > +			damage_area.y2 = damaged_clips[j].y2;
> > > > > > > +			clip_area_update(sel_fetch_area,
> > > > > > > &damage_area);
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		/*
> > > > > > > +		 * No page flip, no plane moviment or no damage
> > > > > > > areas,
> > > > > > > so don't
> > > > > > typo (moviment -> movement)
> > > > > 
> > > > > fixed
> > > > > 
> > > > > > > +		 * fetch any pixel from memory for this plane
> > > > > > > +		 */
> > > > > > > +		if (sel_fetch_area->y1 == -1) {
> > > > > > > +			sel_fetch_area->y1 = 0;
> > > > > > > +			sel_fetch_area->y2 = 0;
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		/* Don't need to redraw plane damaged areas
> > > > > > > outside of
> > > > > > > screen */
> > > > > > > +		j = sel_fetch_area->y2 + (new_plane_state-
> > > > > > > > uapi.dst.y1
> > > > > > > > > 16);
> > > > > > src coordinates of the drm_plane_state are 16.16 fixed point
> > > > > > but
> > > > > > dst
> > > > > > coordinates are not 16.16 fixed point.
> > > > > > therefore we don't need to bit shift for dst.
> > > > > > Because the sel_fetch_area seems based on src coordinates, in
> > > > > > order to
> > > > > > apply to dst coordinates here,  it requires coordinate
> > > > > > calculation. 
> > > > > 
> > > > > thanks for catching this, also fixed the same issue patch 1.
> > > > > 
> > > > > > > +		j = crtc_state-
> > > > > > > > uapi.adjusted_mode.crtc_vdisplay - j;
> > > > > > > +		if (j < 0)
> > > > > > > +			sel_fetch_area->y2 += j;
> > > > > > >  
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > >  		temp = *sel_fetch_area;
> > > > > > >  		temp.y1 += new_plane_state->uapi.dst.y1 >> 16;
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915/display/psr: Use plane damage clips to calculate damaged area

[Mun, Gwan-gyeong]

On Tue, 2020-12-01 at 13:15 -0800, Souza, Jose wrote:
> On Tue, 2020-12-01 at 19:40 +0000, Mun, Gwan-gyeong wrote:
> > On Tue, 2020-12-01 at 09:39 -0800, Souza, Jose wrote:
> > > On Tue, 2020-12-01 at 17:26 +0000, Mun, Gwan-gyeong wrote:
> > > > On Tue, 2020-10-27 at 13:12 -0700, Souza, Jose wrote:
> > > > > On Tue, 2020-10-27 at 01:04 +0000, Souza, Jose wrote:
> > > > > > On Mon, 2020-10-26 at 21:40 +0000, Mun, Gwan-gyeong wrote:
> > > > > > > On Tue, 2020-10-13 at 16:01 -0700, José Roberto de Souza
> > > > > > > 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.
> > > > > > > > 
> > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > 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 | 54
> > > > > > > > +++++++++++++++++++++-
> > > > > > > > --
> > > > > > > >  1 file changed, 49 insertions(+), 5 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > index 773a5b5fa078..0f1e9f0fa57f 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > @@ -1273,6 +1273,9 @@ 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 *sel_fetch_area, temp;
> > > > > > > > +		struct drm_mode_rect *damaged_clips;
> > > > > > > > +		u32 num_clips;
> > > > > > > > +		int j;
> > > > > > > >  
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > >  		if (new_plane_state->uapi.crtc !=
> > > > > > > > crtc_state-
> > > > > > > > > uapi.crtc)
> > > > > > > >  			continue;
> > > > selective fetch area also can be affected by Selective Updated
> > > > region. 
> > > > therefore Selective Updated region rect should be calculated
> > > > first
> > > > and
> > > > then the plane's selective fetch area should be applied
> > > > (intersected by
> > > > calculated SU.)
> > > > please check this implementation.
> > > 
> > > Why selective update region needs to be calculate first if it
> > > should
> > > be based on the plane damage areas/selective fetch areas?
> > > Can you give a example where it gives not the expected result?
> > > 
> > I drew the example here : 
> > https://docs.google.com/presentation/d/1MI20q_3GublGYsY2TDheRncOQsbIjMn20aQ99loaoFg/edit?usp=sharing
> 
> Thanks so much for the drawing, now I got what you were talking
> about.
> This point is not very clear in BSpec but you are right we need to
> set plane selective fetch area using the whole area damaged in pipe.
> 
> Can you take a look at the last patch in this branch? 
> https://github.com/zehortigoza/linux/tree/psr2-sel-fetch-part3-v4
I commented your WIP patch.
> Will do some testing and squash into the right commits and resend in
> the next couple of the days.
> 
> > > > (
> > > > https://patchwork.freedesktop.org/patch/404264/?series=84340&rev=1
> > > > )
> > > > > > > > @@ -1291,13 +1294,54 @@ int
> > > > > > > > intel_psr2_sel_fetch_update(struct
> > > > > > > > intel_atomic_state *state,
> > > > > > > >  		if (!new_plane_state->uapi.visible)
> > > > > > > >  			continue;
> > > > > > > >  
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > +		sel_fetch_area = &new_plane_state-
> > > > > > > > > psr2_sel_fetch_area;
> > > > > > > > +		sel_fetch_area->y1 = -1;
> > > > > > > > +
> > > > > > > > +		damaged_clips =
> > > > > > > > drm_plane_get_damage_clips(&new_plane_state->uapi);
> > > > > > > > +		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 plane moved, mark the whole plane
> > > > > > > > area as
> > > > > > > > damaged
> > > > > > > > as it
> > > > > > > > +		 * needs to be complete redraw in the
> > > > > > > > new
> > > > > > > > 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 (!drm_rect_equals(&new_plane_state-
> > > > > > > > > uapi.dst,
> > > > > > > > +				     &old_plane_state-
> > > > > > > > > uapi.dst)) {
> > > > > > > > +			num_clips = 0;
> > > > > > > > +			sel_fetch_area->y1 =
> > > > > > > > new_plane_state-
> > > > > > > > > uapi.src.y1 >> 16;
> > > > > > > > +			sel_fetch_area->y2 =
> > > > > > > > new_plane_state-
> > > > > > > > > uapi.src.y2 >> 16;
> > > > > > > > +		} else if (!num_clips &&
> > > > > > > > new_plane_state-
> > > > > > > > > uapi.fb !=
> > > > > > > > +			   old_plane_state->uapi.fb) {
> > > > > > > > +			/*
> > > > > > > > +			 * If the plane don't have
> > > > > > > > damage areas
> > > > > > > > but the
> > > > > > > > +			 * framebuffer changed, mark
> > > > > > > > the whole
> > > > > > > > plane
> > > > > > > > area as
> > > > > > > > +			 * damaged.
> > > > > > > > +			 */
> > > > > > > > +			sel_fetch_area->y1 =
> > > > > > > > new_plane_state-
> > > > > > > > > uapi.src.y1 >> 16;
> > > > > > > > +			sel_fetch_area->y2 =
> > > > > > > > new_plane_state-
> > > > > > > > > uapi.src.y2 >> 16;
> > > > > > > > +		}
> > > > > > > > +
> > > > > > > why don't you use drm_atomic_helper_damage_merged()
> > > > > > > function
> > > > > > > here?
> > > > > > 
> > > > > > hum did not knew about this function, will take a look at
> > > > > > as it
> > > > > > does more than just merge all damaged areas.
> > > > > 
> > > > > We can't use this function as it marks the whole plane area
> > > > > as
> > > > > damaged if there is no damaged clips.
> > > > > But for our use case this is bad as we add all planes of the
> > > > > pipe/CRTC to the state, so it would cause a full fetch of the
> > > > > planes
> > > > > without any
> > > > > flip/modification.
> > > > > 
> > > > > > > > +		for (j = 0; j < num_clips; j++) {
> > > > > > > > +			struct drm_rect damage_area;
> > > > > > > > +
> > > > > > > > +			damage_area.y1 =
> > > > > > > > damaged_clips[j].y1;
> > > > > > > > +			damage_area.y2 =
> > > > > > > > damaged_clips[j].y2;
> > > > > > > > +			clip_area_update(sel_fetch_area
> > > > > > > > ,
> > > > > > > > &damage_area);
> > > > > > > > +		}
> > > > > > > > +
> > > > > > > > +		/*
> > > > > > > > +		 * No page flip, no plane moviment or
> > > > > > > > no damage
> > > > > > > > areas,
> > > > > > > > so don't
> > > > > > > typo (moviment -> movement)
> > > > > > 
> > > > > > fixed
> > > > > > 
> > > > > > > > +		 * fetch any pixel from memory for this
> > > > > > > > plane
> > > > > > > > +		 */
> > > > > > > > +		if (sel_fetch_area->y1 == -1) {
> > > > > > > > +			sel_fetch_area->y1 = 0;
> > > > > > > > +			sel_fetch_area->y2 = 0;
> > > > > > > > +		}
> > > > > > > > +
> > > > > > > > +		/* Don't need to redraw plane damaged
> > > > > > > > areas
> > > > > > > > outside of
> > > > > > > > screen */
> > > > > > > > +		j = sel_fetch_area->y2 +
> > > > > > > > (new_plane_state-
> > > > > > > > > uapi.dst.y1
> > > > > > > > > > 16);
> > > > > > > src coordinates of the drm_plane_state are 16.16 fixed
> > > > > > > point
> > > > > > > but
> > > > > > > dst
> > > > > > > coordinates are not 16.16 fixed point.
> > > > > > > therefore we don't need to bit shift for dst.
> > > > > > > Because the sel_fetch_area seems based on src
> > > > > > > coordinates, in
> > > > > > > order to
> > > > > > > apply to dst coordinates here,  it requires coordinate
> > > > > > > calculation. 
> > > > > > 
> > > > > > thanks for catching this, also fixed the same issue patch
> > > > > > 1.
> > > > > > 
> > > > > > > > +		j = crtc_state-
> > > > > > > > > uapi.adjusted_mode.crtc_vdisplay - j;
> > > > > > > > +		if (j < 0)
> > > > > > > > +			sel_fetch_area->y2 += j;
> > > > > > > >  
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > >  		temp = *sel_fetch_area;
> > > > > > > >  		temp.y1 += new_plane_state->uapi.dst.y1 
> > > > > > > > >> 16;
> > > > > > 
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx