[PATCH 0/3] Fixes for damage clips handling

[PATCH 0/3] Fixes for damage clips handling

[Jouni Högander]

Currently damage clips handling is broken for planes when using big
framebuffer + offset in case kms driver adjusts drm_plane_state.src
coords. This is because damage clips are using coords relative to
original coords from user-space.

This patchset is fixing this by using original
coords from user-space instead of drm_plane_state.src when iterating
damage_clips.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Mika Kahola <mika.kahola@intel.com>

Jouni Högander (3):
  drm: Use original src rect while initializing damage iterator
  drm/i915/display: Use original src in psr2 sel fetch area calculation
  drm/i915/display: Use drm helper instead of own loop for damage clips

 drivers/gpu/drm/drm_damage_helper.c      | 11 +++++++----
 drivers/gpu/drm/i915/display/intel_psr.c | 20 +++++++-------------
 2 files changed, 14 insertions(+), 17 deletions(-)

-- 
2.25.1

[PATCH 1/3] drm: Use original src rect while initializing damage iterator

[Jouni Högander]

drm_plane_state->src might be modified by the driver. This is done
e.g. in i915 driver when there is bigger framebuffer than the plane
and there is some offset within framebuffer. I915 driver calculates
separate offset and adjusts src rect coords to be relative to this
offset. Damage clips are still relative to original src coords
provided by user-space.

This patch ensures original coordinates provided by user-space are
used when initiliazing damage iterator.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
---
 drivers/gpu/drm/drm_damage_helper.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index 937b699ac2a8..d8b2955e88fd 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -224,6 +224,7 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
 				   const struct drm_plane_state *old_state,
 				   const struct drm_plane_state *state)
 {
+	struct drm_rect src;
 	memset(iter, 0, sizeof(*iter));
 
 	if (!state || !state->crtc || !state->fb || !state->visible)
@@ -233,10 +234,12 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
 	iter->num_clips = drm_plane_get_damage_clips_count(state);
 
 	/* Round down for x1/y1 and round up for x2/y2 to catch all pixels */
-	iter->plane_src.x1 = state->src.x1 >> 16;
-	iter->plane_src.y1 = state->src.y1 >> 16;
-	iter->plane_src.x2 = (state->src.x2 >> 16) + !!(state->src.x2 & 0xFFFF);
-	iter->plane_src.y2 = (state->src.y2 >> 16) + !!(state->src.y2 & 0xFFFF);
+	src = drm_plane_state_src(state);
+
+	iter->plane_src.x1 = src.x1 >> 16;
+	iter->plane_src.y1 = src.y1 >> 16;
+	iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0xFFFF);
+	iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0xFFFF);
 
 	if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src)) {
 		iter->clips = NULL;
-- 
2.25.1

Re: [Intel-gfx] [PATCH 1/3] drm: Use original src rect while initializing damage iterator

[Daniel Vetter]

On Fri, Jul 15, 2022 at 04:49:56PM +0300, Jouni Högander wrote:
> drm_plane_state->src might be modified by the driver. This is done
> e.g. in i915 driver when there is bigger framebuffer than the plane
> and there is some offset within framebuffer. I915 driver calculates
> separate offset and adjusts src rect coords to be relative to this
> offset. Damage clips are still relative to original src coords
> provided by user-space.
> 
> This patch ensures original coordinates provided by user-space are
> used when initiliazing damage iterator.
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>

Ah kunit test for this would be awesome. Iirc we have a few already for
rect/damage stuff, defo should extend/fix those.
-Daniel

> ---
>  drivers/gpu/drm/drm_damage_helper.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
> index 937b699ac2a8..d8b2955e88fd 100644
> --- a/drivers/gpu/drm/drm_damage_helper.c
> +++ b/drivers/gpu/drm/drm_damage_helper.c
> @@ -224,6 +224,7 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>  				   const struct drm_plane_state *old_state,
>  				   const struct drm_plane_state *state)
>  {
> +	struct drm_rect src;
>  	memset(iter, 0, sizeof(*iter));
>  
>  	if (!state || !state->crtc || !state->fb || !state->visible)
> @@ -233,10 +234,12 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>  	iter->num_clips = drm_plane_get_damage_clips_count(state);
>  
>  	/* Round down for x1/y1 and round up for x2/y2 to catch all pixels */
> -	iter->plane_src.x1 = state->src.x1 >> 16;
> -	iter->plane_src.y1 = state->src.y1 >> 16;
> -	iter->plane_src.x2 = (state->src.x2 >> 16) + !!(state->src.x2 & 0xFFFF);
> -	iter->plane_src.y2 = (state->src.y2 >> 16) + !!(state->src.y2 & 0xFFFF);
> +	src = drm_plane_state_src(state);
> +
> +	iter->plane_src.x1 = src.x1 >> 16;
> +	iter->plane_src.y1 = src.y1 >> 16;
> +	iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0xFFFF);
> +	iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0xFFFF);
>  
>  	if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src)) {
>  		iter->clips = NULL;
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 1/3] drm: Use original src rect while initializing damage iterator

[Hogander, Jouni]

On Thu, 2022-08-11 at 18:23 +0200, Daniel Vetter wrote:
> On Fri, Jul 15, 2022 at 04:49:56PM +0300, Jouni Högander wrote:
> > drm_plane_state->src might be modified by the driver. This is done
> > e.g. in i915 driver when there is bigger framebuffer than the plane
> > and there is some offset within framebuffer. I915 driver calculates
> > separate offset and adjusts src rect coords to be relative to this
> > offset. Damage clips are still relative to original src coords
> > provided by user-space.
> > 
> > This patch ensures original coordinates provided by user-space are
> > used when initiliazing damage iterator.
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> 
> Ah kunit test for this would be awesome. Iirc we have a few already
> for
> rect/damage stuff, defo should extend/fix those.

Can you please provide me some pointer to these tests? I have written
earlier one igt test which reveals this issue:

https://patchwork.freedesktop.org/series/103661/
https://patchwork.freedesktop.org/series/104488/

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_damage_helper.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_damage_helper.c
> > b/drivers/gpu/drm/drm_damage_helper.c
> > index 937b699ac2a8..d8b2955e88fd 100644
> > --- a/drivers/gpu/drm/drm_damage_helper.c
> > +++ b/drivers/gpu/drm/drm_damage_helper.c
> > @@ -224,6 +224,7 @@ drm_atomic_helper_damage_iter_init(struct
> > drm_atomic_helper_damage_iter *iter,
> >  				   const struct drm_plane_state
> > *old_state,
> >  				   const struct drm_plane_state *state)
> >  {
> > +	struct drm_rect src;
> >  	memset(iter, 0, sizeof(*iter));
> >  
> >  	if (!state || !state->crtc || !state->fb || !state->visible)
> > @@ -233,10 +234,12 @@ drm_atomic_helper_damage_iter_init(struct
> > drm_atomic_helper_damage_iter *iter,
> >  	iter->num_clips = drm_plane_get_damage_clips_count(state);
> >  
> >  	/* Round down for x1/y1 and round up for x2/y2 to catch all
> > pixels */
> > -	iter->plane_src.x1 = state->src.x1 >> 16;
> > -	iter->plane_src.y1 = state->src.y1 >> 16;
> > -	iter->plane_src.x2 = (state->src.x2 >> 16) + !!(state->src.x2 &
> > 0xFFFF);
> > -	iter->plane_src.y2 = (state->src.y2 >> 16) + !!(state->src.y2 &
> > 0xFFFF);
> > +	src = drm_plane_state_src(state);
> > +
> > +	iter->plane_src.x1 = src.x1 >> 16;
> > +	iter->plane_src.y1 = src.y1 >> 16;
> > +	iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0xFFFF);
> > +	iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0xFFFF);
> >  
> >  	if (!iter->clips || !drm_rect_equals(&state->src, &old_state-
> > >src)) {
> >  		iter->clips = NULL;
> > -- 
> > 2.25.1
> > 

Re: [PATCH 1/3] drm: Use original src rect while initializing damage iterator

[Maíra Canal]

Hi Jouni,

On 7/15/22 10:49, Jouni Högander wrote:
> drm_plane_state->src might be modified by the driver. This is done
> e.g. in i915 driver when there is bigger framebuffer than the plane
> and there is some offset within framebuffer. I915 driver calculates
> separate offset and adjusts src rect coords to be relative to this
> offset. Damage clips are still relative to original src coords
> provided by user-space.
> 
> This patch ensures original coordinates provided by user-space are
> used when initiliazing damage iterator.
> 

drm_damage_helper has some KUnit tests on drivers/gpu/drm/tests, and by
applying this patch the drm_damage_helper tests started to fail. Could
you also refactor the drm_damage_helper tests?

To run the tests, you can run:
$ ./tools/testing/kunit/kunit.py run \
--kunitconfig=drivers/gpu/drm/tests \
--kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y \
--kconfig_add CONFIG_VIRTIO_UML=y

There is also some documentation on the DRM KUnit Tests on [1].

[1] https://docs.kernel.org/gpu/drm-internals.html#unit-testing

Best Regards,
- Maíra Canal

> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>> ---
>  drivers/gpu/drm/drm_damage_helper.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
> index 937b699ac2a8..d8b2955e88fd 100644
> --- a/drivers/gpu/drm/drm_damage_helper.c
> +++ b/drivers/gpu/drm/drm_damage_helper.c
> @@ -224,6 +224,7 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>  				   const struct drm_plane_state *old_state,
>  				   const struct drm_plane_state *state)
>  {
> +	struct drm_rect src;
>  	memset(iter, 0, sizeof(*iter));
>  
>  	if (!state || !state->crtc || !state->fb || !state->visible)
> @@ -233,10 +234,12 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>  	iter->num_clips = drm_plane_get_damage_clips_count(state);
>  
>  	/* Round down for x1/y1 and round up for x2/y2 to catch all pixels */
> -	iter->plane_src.x1 = state->src.x1 >> 16;
> -	iter->plane_src.y1 = state->src.y1 >> 16;
> -	iter->plane_src.x2 = (state->src.x2 >> 16) + !!(state->src.x2 & 0xFFFF);
> -	iter->plane_src.y2 = (state->src.y2 >> 16) + !!(state->src.y2 & 0xFFFF);
> +	src = drm_plane_state_src(state);
> +
> +	iter->plane_src.x1 = src.x1 >> 16;
> +	iter->plane_src.y1 = src.y1 >> 16;
> +	iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0xFFFF);
> +	iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0xFFFF);
>  
>  	if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src)) {
>  		iter->clips = NULL;

Re: [PATCH 1/3] drm: Use original src rect while initializing damage iterator

[Hogander, Jouni]

On Fri, 2022-08-19 at 09:09 -0300, Maíra Canal wrote:
> Hi Jouni,
> 
> On 7/15/22 10:49, Jouni Högander wrote:
> > drm_plane_state->src might be modified by the driver. This is done
> > e.g. in i915 driver when there is bigger framebuffer than the plane
> > and there is some offset within framebuffer. I915 driver calculates
> > separate offset and adjusts src rect coords to be relative to this
> > offset. Damage clips are still relative to original src coords
> > provided by user-space.
> > 
> > This patch ensures original coordinates provided by user-space are
> > used when initiliazing damage iterator.
> > 
> 
> drm_damage_helper has some KUnit tests on drivers/gpu/drm/tests, and
> by
> applying this patch the drm_damage_helper tests started to fail.
> Could
> you also refactor the drm_damage_helper tests?
> 
> To run the tests, you can run:
> $ ./tools/testing/kunit/kunit.py run \
> --kunitconfig=drivers/gpu/drm/tests \
> --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y \
> --kconfig_add CONFIG_VIRTIO_UML=y
> 
> There is also some documentation on the DRM KUnit Tests on [1].
> 
> [1] https://docs.kernel.org/gpu/drm-internals.html#unit-testing

Ok, thank you for these. I will take a look.

> 
> Best Regards,
> - Maíra Canal
> 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>> ---
> >  drivers/gpu/drm/drm_damage_helper.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_damage_helper.c
> > b/drivers/gpu/drm/drm_damage_helper.c
> > index 937b699ac2a8..d8b2955e88fd 100644
> > --- a/drivers/gpu/drm/drm_damage_helper.c
> > +++ b/drivers/gpu/drm/drm_damage_helper.c
> > @@ -224,6 +224,7 @@ drm_atomic_helper_damage_iter_init(struct
> > drm_atomic_helper_damage_iter *iter,
> >  				   const struct drm_plane_state
> > *old_state,
> >  				   const struct drm_plane_state *state)
> >  {
> > +	struct drm_rect src;
> >  	memset(iter, 0, sizeof(*iter));
> >  
> >  	if (!state || !state->crtc || !state->fb || !state->visible)
> > @@ -233,10 +234,12 @@ drm_atomic_helper_damage_iter_init(struct
> > drm_atomic_helper_damage_iter *iter,
> >  	iter->num_clips = drm_plane_get_damage_clips_count(state);
> >  
> >  	/* Round down for x1/y1 and round up for x2/y2 to catch all
> > pixels */
> > -	iter->plane_src.x1 = state->src.x1 >> 16;
> > -	iter->plane_src.y1 = state->src.y1 >> 16;
> > -	iter->plane_src.x2 = (state->src.x2 >> 16) + !!(state->src.x2 &
> > 0xFFFF);
> > -	iter->plane_src.y2 = (state->src.y2 >> 16) + !!(state->src.y2 &
> > 0xFFFF);
> > +	src = drm_plane_state_src(state);
> > +
> > +	iter->plane_src.x1 = src.x1 >> 16;
> > +	iter->plane_src.y1 = src.y1 >> 16;
> > +	iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0xFFFF);
> > +	iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0xFFFF);
> >  
> >  	if (!iter->clips || !drm_rect_equals(&state->src, &old_state-
> > >src)) {
> >  		iter->clips = NULL;

[PATCH 2/3] drm/i915/display: Use original src in psr2 sel fetch area calculation

[Jouni Högander]

drm_plane_state->src is modified when offset is calculated:

before calculation:
src.x1 = 8192, src.y1 = 8192

after calculation (pitch = 65536, cpp = 4, alignment = 262144)
src.x1 = 8192, src.y1 = 0, offset = 0x20000000

Damage clips are relative to original coodrdinates provided by
user-space. To compare these against src coordinates we need to use
original coordinates as provided by user-space. These can be obtained
by using drm_plane_state_src.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 90599dd1cb1b..5c95e24dc8d6 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1767,7 +1767,8 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
 			continue;
 		}
 
-		drm_rect_fp_to_int(&src, &new_plane_state->uapi.src);
+		src = drm_plane_state_src(&new_plane_state->uapi);
+		drm_rect_fp_to_int(&src, &src);
 
 		drm_atomic_helper_damage_iter_init(&iter,
 						   &old_plane_state->uapi,
-- 
2.25.1

[PATCH 3/3] drm/i915/display: Use drm helper instead of own loop for damage clips

[Jouni Högander]

Use existing drm_atomic_helper_damage_merged from generic drm code
instead of implementing own loop to iterate over damage_clips.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 5c95e24dc8d6..d44662f19b53 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1721,8 +1721,6 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
 					     new_plane_state, i) {
 		struct drm_rect src, damaged_area = { .x1 = 0, .y1 = -1,
 						      .x2 = INT_MAX };
-		struct drm_atomic_helper_damage_iter iter;
-		struct drm_rect clip;
 
 		if (new_plane_state->uapi.crtc != crtc_state->uapi.crtc)
 			continue;
@@ -1770,20 +1768,15 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
 		src = drm_plane_state_src(&new_plane_state->uapi);
 		drm_rect_fp_to_int(&src, &src);
 
-		drm_atomic_helper_damage_iter_init(&iter,
-						   &old_plane_state->uapi,
-						   &new_plane_state->uapi);
-		drm_atomic_for_each_plane_damage(&iter, &clip) {
-			if (drm_rect_intersect(&clip, &src))
-				clip_area_update(&damaged_area, &clip,
-						 &crtc_state->pipe_src);
-		}
-
-		if (damaged_area.y1 == -1)
+		if (!drm_atomic_helper_damage_merged(&old_plane_state->uapi,
+						     &new_plane_state->uapi, &damaged_area))
 			continue;
 
 		damaged_area.y1 += new_plane_state->uapi.dst.y1 - src.y1;
 		damaged_area.y2 += new_plane_state->uapi.dst.y1 - src.y1;
+		damaged_area.x1 += new_plane_state->uapi.dst.x1 - src.x1;
+		damaged_area.x2 += new_plane_state->uapi.dst.x1 - src.x1;
+
 		clip_area_update(&pipe_clip, &damaged_area, &crtc_state->pipe_src);
 	}
 
-- 
2.25.1