All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/amd/display: Use orientated source size when checking cursor scaling
@ 2021-12-02 12:16 Vlad Zahorodnii
  2021-12-02 12:21 ` Simon Ser
  2021-12-02 12:52 ` [PATCH v2] drm/amd/display: Use oriented " Vlad Zahorodnii
  0 siblings, 2 replies; 7+ messages in thread
From: Vlad Zahorodnii @ 2021-12-02 12:16 UTC (permalink / raw)
  To: amd-gfx
  Cc: Vlad Zahorodnii, contact, Rodrigo.Siqueira, xaver.hugl,
	alexander.deucher, christian.koenig

dm_check_crtc_cursor() doesn't take into account plane transforms when
calculating plane scaling, this can result in false positives.

For example, if there's an output with resolution 3840x2160 and the
output is rotated 90 degrees, CRTC_W and CRTC_H will be 3840 and 2160,
respectively, but SRC_W and SRC_H will be 2160 and 3840, respectively.

Since the cursor plane usually has a square buffer attached to it, the
dm_check_crtc_cursor() will think that there's a scale factor mismatch
even though there isn't really.

This fixes an issue where kwin fails to use hardware plane transforms.

Signed-off-by: Vlad Zahorodnii <vlad.zahorodnii@kde.org>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 35 ++++++++++++++-----
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a3c0f2e4f4c1..27e5a26ae67c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10736,6 +10736,24 @@ static int dm_update_plane_state(struct dc *dc,
 	return ret;
 }
 
+static void dm_get_orientated_plane_size(struct drm_plane_state *plane_state,
+					 int *src_w, int *src_h)
+{
+	switch (plane_state->rotation & DRM_MODE_ROTATE_MASK) {
+	case DRM_MODE_ROTATE_90:
+	case DRM_MODE_ROTATE_270:
+		*src_w = plane_state->src_h >> 16;
+		*src_h = plane_state->src_w >> 16;
+		break;
+	case DRM_MODE_ROTATE_0:
+	case DRM_MODE_ROTATE_180:
+	default:
+		*src_w = plane_state->src_w >> 16;
+		*src_h = plane_state->src_h >> 16;
+		break;
+	}
+}
+
 static int dm_check_crtc_cursor(struct drm_atomic_state *state,
 				struct drm_crtc *crtc,
 				struct drm_crtc_state *new_crtc_state)
@@ -10744,6 +10762,8 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
 	struct drm_plane_state *new_cursor_state, *new_underlying_state;
 	int i;
 	int cursor_scale_w, cursor_scale_h, underlying_scale_w, underlying_scale_h;
+	int cursor_src_w, cursor_src_h;
+	int underlying_src_w, underlying_src_h;
 
 	/* On DCE and DCN there is no dedicated hardware cursor plane. We get a
 	 * cursor per pipe but it's going to inherit the scaling and
@@ -10755,10 +10775,9 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
 		return 0;
 	}
 
-	cursor_scale_w = new_cursor_state->crtc_w * 1000 /
-			 (new_cursor_state->src_w >> 16);
-	cursor_scale_h = new_cursor_state->crtc_h * 1000 /
-			 (new_cursor_state->src_h >> 16);
+	dm_get_orientated_plane_size(new_cursor_state, &cursor_src_w, &cursor_src_h);
+	cursor_scale_w = new_cursor_state->crtc_w * 1000 / cursor_src_w;
+	cursor_scale_h = new_cursor_state->crtc_h * 1000 / cursor_src_h;
 
 	for_each_new_plane_in_state_reverse(state, underlying, new_underlying_state, i) {
 		/* Narrow down to non-cursor planes on the same CRTC as the cursor */
@@ -10769,10 +10788,10 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
 		if (!new_underlying_state->fb)
 			continue;
 
-		underlying_scale_w = new_underlying_state->crtc_w * 1000 /
-				     (new_underlying_state->src_w >> 16);
-		underlying_scale_h = new_underlying_state->crtc_h * 1000 /
-				     (new_underlying_state->src_h >> 16);
+		dm_get_orientated_plane_size(new_underlying_state,
+					     &underlying_src_w, &underlying_src_h);
+		underlying_scale_w = new_underlying_state->crtc_w * 1000 / underlying_src_w;
+		underlying_scale_h = new_underlying_state->crtc_h * 1000 / underlying_src_h;
 
 		if (cursor_scale_w != underlying_scale_w ||
 		    cursor_scale_h != underlying_scale_h) {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/amd/display: Use orientated source size when checking cursor scaling
  2021-12-02 12:16 [PATCH] drm/amd/display: Use orientated source size when checking cursor scaling Vlad Zahorodnii
@ 2021-12-02 12:21 ` Simon Ser
  2021-12-02 12:23   ` Vlad Zahorodnii
  2021-12-02 12:52 ` [PATCH v2] drm/amd/display: Use oriented " Vlad Zahorodnii
  1 sibling, 1 reply; 7+ messages in thread
From: Simon Ser @ 2021-12-02 12:21 UTC (permalink / raw)
  To: Vlad Zahorodnii
  Cc: alexander.deucher, Rodrigo.Siqueira, christian.koenig, amd-gfx,
	xaver.hugl

Have you tested this? I have a similar patch, but the cursor position was off
when I tried it.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/amd/display: Use orientated source size when checking cursor scaling
  2021-12-02 12:21 ` Simon Ser
@ 2021-12-02 12:23   ` Vlad Zahorodnii
  0 siblings, 0 replies; 7+ messages in thread
From: Vlad Zahorodnii @ 2021-12-02 12:23 UTC (permalink / raw)
  To: Simon Ser
  Cc: alexander.deucher, Rodrigo.Siqueira, christian.koenig, amd-gfx,
	xaver.hugl

On 12/2/21 14:21, Simon Ser wrote:
> Have you tested this? I have a similar patch, but the cursor position was off
> when I tried it.
> 

Works fine on my machine with RX 5700 XT.

Cheers,
vlad

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] drm/amd/display: Use oriented source size when checking cursor scaling
  2021-12-02 12:16 [PATCH] drm/amd/display: Use orientated source size when checking cursor scaling Vlad Zahorodnii
  2021-12-02 12:21 ` Simon Ser
@ 2021-12-02 12:52 ` Vlad Zahorodnii
  2021-12-02 14:54   ` Kazlauskas, Nicholas
  1 sibling, 1 reply; 7+ messages in thread
From: Vlad Zahorodnii @ 2021-12-02 12:52 UTC (permalink / raw)
  To: amd-gfx
  Cc: Vlad Zahorodnii, contact, Rodrigo.Siqueira, xaver.hugl,
	alexander.deucher, christian.koenig

dm_check_crtc_cursor() doesn't take into account plane transforms when
calculating plane scaling, this can result in false positives.

For example, if there's an output with resolution 3840x2160 and the
output is rotated 90 degrees, CRTC_W and CRTC_H will be 3840 and 2160,
respectively, but SRC_W and SRC_H will be 2160 and 3840, respectively.

Since the cursor plane usually has a square buffer attached to it, the
dm_check_crtc_cursor() will think that there's a scale factor mismatch
even though there isn't really.

This fixes an issue where kwin fails to use hardware plane transforms.

Changes since version 1:
- s/orientated/oriented/g

Signed-off-by: Vlad Zahorodnii <vlad.zahorodnii@kde.org>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 35 ++++++++++++++-----
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a3c0f2e4f4c1..c009c668fbe2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10736,6 +10736,24 @@ static int dm_update_plane_state(struct dc *dc,
 	return ret;
 }
 
+static void dm_get_oriented_plane_size(struct drm_plane_state *plane_state,
+				       int *src_w, int *src_h)
+{
+	switch (plane_state->rotation & DRM_MODE_ROTATE_MASK) {
+	case DRM_MODE_ROTATE_90:
+	case DRM_MODE_ROTATE_270:
+		*src_w = plane_state->src_h >> 16;
+		*src_h = plane_state->src_w >> 16;
+		break;
+	case DRM_MODE_ROTATE_0:
+	case DRM_MODE_ROTATE_180:
+	default:
+		*src_w = plane_state->src_w >> 16;
+		*src_h = plane_state->src_h >> 16;
+		break;
+	}
+}
+
 static int dm_check_crtc_cursor(struct drm_atomic_state *state,
 				struct drm_crtc *crtc,
 				struct drm_crtc_state *new_crtc_state)
@@ -10744,6 +10762,8 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
 	struct drm_plane_state *new_cursor_state, *new_underlying_state;
 	int i;
 	int cursor_scale_w, cursor_scale_h, underlying_scale_w, underlying_scale_h;
+	int cursor_src_w, cursor_src_h;
+	int underlying_src_w, underlying_src_h;
 
 	/* On DCE and DCN there is no dedicated hardware cursor plane. We get a
 	 * cursor per pipe but it's going to inherit the scaling and
@@ -10755,10 +10775,9 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
 		return 0;
 	}
 
-	cursor_scale_w = new_cursor_state->crtc_w * 1000 /
-			 (new_cursor_state->src_w >> 16);
-	cursor_scale_h = new_cursor_state->crtc_h * 1000 /
-			 (new_cursor_state->src_h >> 16);
+	dm_get_oriented_plane_size(new_cursor_state, &cursor_src_w, &cursor_src_h);
+	cursor_scale_w = new_cursor_state->crtc_w * 1000 / cursor_src_w;
+	cursor_scale_h = new_cursor_state->crtc_h * 1000 / cursor_src_h;
 
 	for_each_new_plane_in_state_reverse(state, underlying, new_underlying_state, i) {
 		/* Narrow down to non-cursor planes on the same CRTC as the cursor */
@@ -10769,10 +10788,10 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
 		if (!new_underlying_state->fb)
 			continue;
 
-		underlying_scale_w = new_underlying_state->crtc_w * 1000 /
-				     (new_underlying_state->src_w >> 16);
-		underlying_scale_h = new_underlying_state->crtc_h * 1000 /
-				     (new_underlying_state->src_h >> 16);
+		dm_get_oriented_plane_size(new_underlying_state,
+					   &underlying_src_w, &underlying_src_h);
+		underlying_scale_w = new_underlying_state->crtc_w * 1000 / underlying_src_w;
+		underlying_scale_h = new_underlying_state->crtc_h * 1000 / underlying_src_h;
 
 		if (cursor_scale_w != underlying_scale_w ||
 		    cursor_scale_h != underlying_scale_h) {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] drm/amd/display: Use oriented source size when checking cursor scaling
  2021-12-02 12:52 ` [PATCH v2] drm/amd/display: Use oriented " Vlad Zahorodnii
@ 2021-12-02 14:54   ` Kazlauskas, Nicholas
  2021-12-02 17:11     ` Alex Deucher
  2021-12-02 17:47     ` Vlad Zahorodnii
  0 siblings, 2 replies; 7+ messages in thread
From: Kazlauskas, Nicholas @ 2021-12-02 14:54 UTC (permalink / raw)
  To: Vlad Zahorodnii, amd-gfx
  Cc: alexander.deucher, contact, Rodrigo.Siqueira, xaver.hugl,
	christian.koenig

On 2021-12-02 7:52 a.m., Vlad Zahorodnii wrote:
> dm_check_crtc_cursor() doesn't take into account plane transforms when
> calculating plane scaling, this can result in false positives.
> 
> For example, if there's an output with resolution 3840x2160 and the
> output is rotated 90 degrees, CRTC_W and CRTC_H will be 3840 and 2160,
> respectively, but SRC_W and SRC_H will be 2160 and 3840, respectively.
> 
> Since the cursor plane usually has a square buffer attached to it, the
> dm_check_crtc_cursor() will think that there's a scale factor mismatch
> even though there isn't really.
> 
> This fixes an issue where kwin fails to use hardware plane transforms.
> 
> Changes since version 1:
> - s/orientated/oriented/g
> 
> Signed-off-by: Vlad Zahorodnii <vlad.zahorodnii@kde.org>

This looks correct to me. I guess it's also not modifying the actual 
programming position, just the check to ensure that the cursor is going 
to be unscaled in the correct orientation.

Would be good to have some IGT tests for these scaled cases to verify 
atomic check pass/fail assumptions, but for now:

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

Regards,
Nicholas Kazlauskas

> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 35 ++++++++++++++-----
>   1 file changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index a3c0f2e4f4c1..c009c668fbe2 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10736,6 +10736,24 @@ static int dm_update_plane_state(struct dc *dc,
>   	return ret;
>   }
>   
> +static void dm_get_oriented_plane_size(struct drm_plane_state *plane_state,
> +				       int *src_w, int *src_h)
> +{
> +	switch (plane_state->rotation & DRM_MODE_ROTATE_MASK) {
> +	case DRM_MODE_ROTATE_90:
> +	case DRM_MODE_ROTATE_270:
> +		*src_w = plane_state->src_h >> 16;
> +		*src_h = plane_state->src_w >> 16;
> +		break;
> +	case DRM_MODE_ROTATE_0:
> +	case DRM_MODE_ROTATE_180:
> +	default:
> +		*src_w = plane_state->src_w >> 16;
> +		*src_h = plane_state->src_h >> 16;
> +		break;
> +	}
> +}
> +
>   static int dm_check_crtc_cursor(struct drm_atomic_state *state,
>   				struct drm_crtc *crtc,
>   				struct drm_crtc_state *new_crtc_state)
> @@ -10744,6 +10762,8 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
>   	struct drm_plane_state *new_cursor_state, *new_underlying_state;
>   	int i;
>   	int cursor_scale_w, cursor_scale_h, underlying_scale_w, underlying_scale_h;
> +	int cursor_src_w, cursor_src_h;
> +	int underlying_src_w, underlying_src_h;
>   
>   	/* On DCE and DCN there is no dedicated hardware cursor plane. We get a
>   	 * cursor per pipe but it's going to inherit the scaling and
> @@ -10755,10 +10775,9 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
>   		return 0;
>   	}
>   
> -	cursor_scale_w = new_cursor_state->crtc_w * 1000 /
> -			 (new_cursor_state->src_w >> 16);
> -	cursor_scale_h = new_cursor_state->crtc_h * 1000 /
> -			 (new_cursor_state->src_h >> 16);
> +	dm_get_oriented_plane_size(new_cursor_state, &cursor_src_w, &cursor_src_h);
> +	cursor_scale_w = new_cursor_state->crtc_w * 1000 / cursor_src_w;
> +	cursor_scale_h = new_cursor_state->crtc_h * 1000 / cursor_src_h;
>   
>   	for_each_new_plane_in_state_reverse(state, underlying, new_underlying_state, i) {
>   		/* Narrow down to non-cursor planes on the same CRTC as the cursor */
> @@ -10769,10 +10788,10 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
>   		if (!new_underlying_state->fb)
>   			continue;
>   
> -		underlying_scale_w = new_underlying_state->crtc_w * 1000 /
> -				     (new_underlying_state->src_w >> 16);
> -		underlying_scale_h = new_underlying_state->crtc_h * 1000 /
> -				     (new_underlying_state->src_h >> 16);
> +		dm_get_oriented_plane_size(new_underlying_state,
> +					   &underlying_src_w, &underlying_src_h);
> +		underlying_scale_w = new_underlying_state->crtc_w * 1000 / underlying_src_w;
> +		underlying_scale_h = new_underlying_state->crtc_h * 1000 / underlying_src_h;
>   
>   		if (cursor_scale_w != underlying_scale_w ||
>   		    cursor_scale_h != underlying_scale_h) {
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] drm/amd/display: Use oriented source size when checking cursor scaling
  2021-12-02 14:54   ` Kazlauskas, Nicholas
@ 2021-12-02 17:11     ` Alex Deucher
  2021-12-02 17:47     ` Vlad Zahorodnii
  1 sibling, 0 replies; 7+ messages in thread
From: Alex Deucher @ 2021-12-02 17:11 UTC (permalink / raw)
  To: Kazlauskas, Nicholas
  Cc: Vlad Zahorodnii, Simon Ser, Siqueira, Rodrigo, xaver.hugl,
	amd-gfx list, Deucher, Alexander, Christian Koenig

Applied.  thanks!

Alex

On Thu, Dec 2, 2021 at 10:09 AM Kazlauskas, Nicholas
<nicholas.kazlauskas@amd.com> wrote:
>
> On 2021-12-02 7:52 a.m., Vlad Zahorodnii wrote:
> > dm_check_crtc_cursor() doesn't take into account plane transforms when
> > calculating plane scaling, this can result in false positives.
> >
> > For example, if there's an output with resolution 3840x2160 and the
> > output is rotated 90 degrees, CRTC_W and CRTC_H will be 3840 and 2160,
> > respectively, but SRC_W and SRC_H will be 2160 and 3840, respectively.
> >
> > Since the cursor plane usually has a square buffer attached to it, the
> > dm_check_crtc_cursor() will think that there's a scale factor mismatch
> > even though there isn't really.
> >
> > This fixes an issue where kwin fails to use hardware plane transforms.
> >
> > Changes since version 1:
> > - s/orientated/oriented/g
> >
> > Signed-off-by: Vlad Zahorodnii <vlad.zahorodnii@kde.org>
>
> This looks correct to me. I guess it's also not modifying the actual
> programming position, just the check to ensure that the cursor is going
> to be unscaled in the correct orientation.
>
> Would be good to have some IGT tests for these scaled cases to verify
> atomic check pass/fail assumptions, but for now:
>
> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>
> Regards,
> Nicholas Kazlauskas
>
> > ---
> >   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 35 ++++++++++++++-----
> >   1 file changed, 27 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index a3c0f2e4f4c1..c009c668fbe2 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -10736,6 +10736,24 @@ static int dm_update_plane_state(struct dc *dc,
> >       return ret;
> >   }
> >
> > +static void dm_get_oriented_plane_size(struct drm_plane_state *plane_state,
> > +                                    int *src_w, int *src_h)
> > +{
> > +     switch (plane_state->rotation & DRM_MODE_ROTATE_MASK) {
> > +     case DRM_MODE_ROTATE_90:
> > +     case DRM_MODE_ROTATE_270:
> > +             *src_w = plane_state->src_h >> 16;
> > +             *src_h = plane_state->src_w >> 16;
> > +             break;
> > +     case DRM_MODE_ROTATE_0:
> > +     case DRM_MODE_ROTATE_180:
> > +     default:
> > +             *src_w = plane_state->src_w >> 16;
> > +             *src_h = plane_state->src_h >> 16;
> > +             break;
> > +     }
> > +}
> > +
> >   static int dm_check_crtc_cursor(struct drm_atomic_state *state,
> >                               struct drm_crtc *crtc,
> >                               struct drm_crtc_state *new_crtc_state)
> > @@ -10744,6 +10762,8 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
> >       struct drm_plane_state *new_cursor_state, *new_underlying_state;
> >       int i;
> >       int cursor_scale_w, cursor_scale_h, underlying_scale_w, underlying_scale_h;
> > +     int cursor_src_w, cursor_src_h;
> > +     int underlying_src_w, underlying_src_h;
> >
> >       /* On DCE and DCN there is no dedicated hardware cursor plane. We get a
> >        * cursor per pipe but it's going to inherit the scaling and
> > @@ -10755,10 +10775,9 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
> >               return 0;
> >       }
> >
> > -     cursor_scale_w = new_cursor_state->crtc_w * 1000 /
> > -                      (new_cursor_state->src_w >> 16);
> > -     cursor_scale_h = new_cursor_state->crtc_h * 1000 /
> > -                      (new_cursor_state->src_h >> 16);
> > +     dm_get_oriented_plane_size(new_cursor_state, &cursor_src_w, &cursor_src_h);
> > +     cursor_scale_w = new_cursor_state->crtc_w * 1000 / cursor_src_w;
> > +     cursor_scale_h = new_cursor_state->crtc_h * 1000 / cursor_src_h;
> >
> >       for_each_new_plane_in_state_reverse(state, underlying, new_underlying_state, i) {
> >               /* Narrow down to non-cursor planes on the same CRTC as the cursor */
> > @@ -10769,10 +10788,10 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
> >               if (!new_underlying_state->fb)
> >                       continue;
> >
> > -             underlying_scale_w = new_underlying_state->crtc_w * 1000 /
> > -                                  (new_underlying_state->src_w >> 16);
> > -             underlying_scale_h = new_underlying_state->crtc_h * 1000 /
> > -                                  (new_underlying_state->src_h >> 16);
> > +             dm_get_oriented_plane_size(new_underlying_state,
> > +                                        &underlying_src_w, &underlying_src_h);
> > +             underlying_scale_w = new_underlying_state->crtc_w * 1000 / underlying_src_w;
> > +             underlying_scale_h = new_underlying_state->crtc_h * 1000 / underlying_src_h;
> >
> >               if (cursor_scale_w != underlying_scale_w ||
> >                   cursor_scale_h != underlying_scale_h) {
> >
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] drm/amd/display: Use oriented source size when checking cursor scaling
  2021-12-02 14:54   ` Kazlauskas, Nicholas
  2021-12-02 17:11     ` Alex Deucher
@ 2021-12-02 17:47     ` Vlad Zahorodnii
  1 sibling, 0 replies; 7+ messages in thread
From: Vlad Zahorodnii @ 2021-12-02 17:47 UTC (permalink / raw)
  To: Kazlauskas, Nicholas, amd-gfx
  Cc: alexander.deucher, contact, Rodrigo.Siqueira, xaver.hugl,
	christian.koenig

On 12/2/21 16:54, Kazlauskas, Nicholas wrote:
> On 2021-12-02 7:52 a.m., Vlad Zahorodnii wrote:
>> dm_check_crtc_cursor() doesn't take into account plane transforms when
>> calculating plane scaling, this can result in false positives.
>>
>> For example, if there's an output with resolution 3840x2160 and the
>> output is rotated 90 degrees, CRTC_W and CRTC_H will be 3840 and 2160,
>> respectively, but SRC_W and SRC_H will be 2160 and 3840, respectively.
>>
>> Since the cursor plane usually has a square buffer attached to it, the
>> dm_check_crtc_cursor() will think that there's a scale factor mismatch
>> even though there isn't really.
>>
>> This fixes an issue where kwin fails to use hardware plane transforms.
>>
>> Changes since version 1:
>> - s/orientated/oriented/g
>>
>> Signed-off-by: Vlad Zahorodnii <vlad.zahorodnii@kde.org>
> 
> This looks correct to me. I guess it's also not modifying the actual 
> programming position, just the check to ensure that the cursor is going 
> to be unscaled in the correct orientation.
> 
> Would be good to have some IGT tests for these scaled cases to verify 
> atomic check pass/fail assumptions, but for now:

I'd be glad to add the tests, but I would need some assistance. Can we 
continue this conversation in the radeon irc channel (I'm zzag)?

> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> 
> Regards,
> Nicholas Kazlauskas
> 
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 35 ++++++++++++++-----
>>   1 file changed, 27 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index a3c0f2e4f4c1..c009c668fbe2 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -10736,6 +10736,24 @@ static int dm_update_plane_state(struct dc *dc,
>>       return ret;
>>   }
>> +static void dm_get_oriented_plane_size(struct drm_plane_state 
>> *plane_state,
>> +                       int *src_w, int *src_h)
>> +{
>> +    switch (plane_state->rotation & DRM_MODE_ROTATE_MASK) {
>> +    case DRM_MODE_ROTATE_90:
>> +    case DRM_MODE_ROTATE_270:
>> +        *src_w = plane_state->src_h >> 16;
>> +        *src_h = plane_state->src_w >> 16;
>> +        break;
>> +    case DRM_MODE_ROTATE_0:
>> +    case DRM_MODE_ROTATE_180:
>> +    default:
>> +        *src_w = plane_state->src_w >> 16;
>> +        *src_h = plane_state->src_h >> 16;
>> +        break;
>> +    }
>> +}
>> +
>>   static int dm_check_crtc_cursor(struct drm_atomic_state *state,
>>                   struct drm_crtc *crtc,
>>                   struct drm_crtc_state *new_crtc_state)
>> @@ -10744,6 +10762,8 @@ static int dm_check_crtc_cursor(struct 
>> drm_atomic_state *state,
>>       struct drm_plane_state *new_cursor_state, *new_underlying_state;
>>       int i;
>>       int cursor_scale_w, cursor_scale_h, underlying_scale_w, 
>> underlying_scale_h;
>> +    int cursor_src_w, cursor_src_h;
>> +    int underlying_src_w, underlying_src_h;
>>       /* On DCE and DCN there is no dedicated hardware cursor plane. 
>> We get a
>>        * cursor per pipe but it's going to inherit the scaling and
>> @@ -10755,10 +10775,9 @@ static int dm_check_crtc_cursor(struct 
>> drm_atomic_state *state,
>>           return 0;
>>       }
>> -    cursor_scale_w = new_cursor_state->crtc_w * 1000 /
>> -             (new_cursor_state->src_w >> 16);
>> -    cursor_scale_h = new_cursor_state->crtc_h * 1000 /
>> -             (new_cursor_state->src_h >> 16);
>> +    dm_get_oriented_plane_size(new_cursor_state, &cursor_src_w, 
>> &cursor_src_h);
>> +    cursor_scale_w = new_cursor_state->crtc_w * 1000 / cursor_src_w;
>> +    cursor_scale_h = new_cursor_state->crtc_h * 1000 / cursor_src_h;
>>       for_each_new_plane_in_state_reverse(state, underlying, 
>> new_underlying_state, i) {
>>           /* Narrow down to non-cursor planes on the same CRTC as the 
>> cursor */
>> @@ -10769,10 +10788,10 @@ static int dm_check_crtc_cursor(struct 
>> drm_atomic_state *state,
>>           if (!new_underlying_state->fb)
>>               continue;
>> -        underlying_scale_w = new_underlying_state->crtc_w * 1000 /
>> -                     (new_underlying_state->src_w >> 16);
>> -        underlying_scale_h = new_underlying_state->crtc_h * 1000 /
>> -                     (new_underlying_state->src_h >> 16);
>> +        dm_get_oriented_plane_size(new_underlying_state,
>> +                       &underlying_src_w, &underlying_src_h);
>> +        underlying_scale_w = new_underlying_state->crtc_w * 1000 / 
>> underlying_src_w;
>> +        underlying_scale_h = new_underlying_state->crtc_h * 1000 / 
>> underlying_src_h;
>>           if (cursor_scale_w != underlying_scale_w ||
>>               cursor_scale_h != underlying_scale_h) {
>>
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-12-02 17:47 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-02 12:16 [PATCH] drm/amd/display: Use orientated source size when checking cursor scaling Vlad Zahorodnii
2021-12-02 12:21 ` Simon Ser
2021-12-02 12:23   ` Vlad Zahorodnii
2021-12-02 12:52 ` [PATCH v2] drm/amd/display: Use oriented " Vlad Zahorodnii
2021-12-02 14:54   ` Kazlauskas, Nicholas
2021-12-02 17:11     ` Alex Deucher
2021-12-02 17:47     ` Vlad Zahorodnii

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.