Re: [PATCH] drm/amd/display: ensure async flips are only accepted for fast updates

2023-08-04 Thread Harry Wentland 



On 2023-08-04 11:56, Hamza Mahfooz wrote:
> We should be checking to see if async flips are supported in
> amdgpu_dm_atomic_check() (i.e. not dm_crtc_helper_atomic_check()). Also,
> async flipping isn't supported if a plane's framebuffer changes memory
> domains during an atomic commit. So, move the check from
> dm_crtc_helper_atomic_check() to amdgpu_dm_atomic_check() and check if
> the memory domain has changed in amdgpu_dm_atomic_check().
> 
> Cc: sta...@vger.kernel.org
> Fixes: 3f86b60691e6 ("drm/amd/display: only accept async flips for fast 
> updates")
> Tested-by: Marcus Seyfarth 
> Signed-off-by: Hamza Mahfooz 
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 25 ---
>  .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c| 12 -
>  2 files changed, 21 insertions(+), 16 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 32fb551862b0..e561d99b3f40 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -8086,7 +8086,8 @@ static void amdgpu_dm_commit_planes(struct 
> drm_atomic_state *state,
>* fast updates.
>*/
>   if (crtc->state->async_flip &&
> - acrtc_state->update_type != UPDATE_TYPE_FAST)
> + (acrtc_state->update_type != UPDATE_TYPE_FAST ||
> +  get_mem_type(old_plane_state->fb) != get_mem_type(fb)))
>   drm_warn_once(state->dev,
> "[PLANE:%d:%s] async flip with non-fast 
> update\n",
> plane->base.id, plane->name);
> @@ -10050,12 +10051,18 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>  
>   /* Remove exiting planes if they are modified */
>   for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, 
> new_plane_state, i) {
> + if (old_plane_state->fb && new_plane_state->fb &&
> + get_mem_type(old_plane_state->fb) !=
> + get_mem_type(new_plane_state->fb))
> + lock_and_validation_needed = true;
> +
>   ret = dm_update_plane_state(dc, state, plane,
>   old_plane_state,
>   new_plane_state,
>   false,
>   _and_validation_needed,
>   _top_most_overlay);
> +

nit: extraneous newline

>   if (ret) {
>   DRM_DEBUG_DRIVER("dm_update_plane_state() failed\n");
>   goto fail;
> @@ -10069,6 +10076,7 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>  new_crtc_state,
>  false,
>  _and_validation_needed);
> +

nit: extraneous newline

>   if (ret) {
>   DRM_DEBUG_DRIVER("DISABLE: dm_update_crtc_state() 
> failed\n");
>   goto fail;
> @@ -10297,9 +10305,18 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>   struct dm_crtc_state *dm_new_crtc_state =
>   to_dm_crtc_state(new_crtc_state);
>  
> - dm_new_crtc_state->update_type = lock_and_validation_needed ?
> -  UPDATE_TYPE_FULL :
> -  UPDATE_TYPE_FAST;
> + /*
> +  * Only allow async flips for fast updates that don't change
> +  * the FB pitch, the DCC state, rotation, etc.
> +  */
> + if (new_crtc_state->async_flip && lock_and_validation_needed) {
> + drm_dbg_atomic(crtc->dev,
> +"[CRTC:%d:%s] async flips are only 
> supported for fast updates\n",
> +crtc->base.id, crtc->name);
> + ret = -EINVAL;
> + goto fail;
> + } else

nit: Add braces here as well

> + dm_new_crtc_state->update_type = UPDATE_TYPE_FAST;

If async_flip is false you'll be setting update_type to FAST here
uncoditionally.

You'll still need the lock_and_validation check here, i.e. this:

>   dm_new_crtc_state->update_type = lock_and_validation_needed ?
>UPDATE_TYPE_FULL :
>UPDATE_TYPE_FAST;

Harry

>   }
>  
>   /* Must be success */
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> index 30d4c6fd95f5..440fc0869a34 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> +++

[PATCH] drm/amd/display: ensure async flips are only accepted for fast updates

2023-08-04 Thread Hamza Mahfooz 
We should be checking to see if async flips are supported in
amdgpu_dm_atomic_check() (i.e. not dm_crtc_helper_atomic_check()). Also,
async flipping isn't supported if a plane's framebuffer changes memory
domains during an atomic commit. So, move the check from
dm_crtc_helper_atomic_check() to amdgpu_dm_atomic_check() and check if
the memory domain has changed in amdgpu_dm_atomic_check().

Cc: sta...@vger.kernel.org
Fixes: 3f86b60691e6 ("drm/amd/display: only accept async flips for fast 
updates")
Tested-by: Marcus Seyfarth 
Signed-off-by: Hamza Mahfooz 
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 25 ---
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c| 12 -
 2 files changed, 21 insertions(+), 16 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 32fb551862b0..e561d99b3f40 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8086,7 +8086,8 @@ static void amdgpu_dm_commit_planes(struct 
drm_atomic_state *state,
 * fast updates.
 */
if (crtc->state->async_flip &&
-   acrtc_state->update_type != UPDATE_TYPE_FAST)
+   (acrtc_state->update_type != UPDATE_TYPE_FAST ||
+get_mem_type(old_plane_state->fb) != get_mem_type(fb)))
drm_warn_once(state->dev,
  "[PLANE:%d:%s] async flip with non-fast 
update\n",
  plane->base.id, plane->name);
@@ -10050,12 +10051,18 @@ static int amdgpu_dm_atomic_check(struct drm_device 
*dev,
 
/* Remove exiting planes if they are modified */
for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, 
new_plane_state, i) {
+   if (old_plane_state->fb && new_plane_state->fb &&
+   get_mem_type(old_plane_state->fb) !=
+   get_mem_type(new_plane_state->fb))
+   lock_and_validation_needed = true;
+
ret = dm_update_plane_state(dc, state, plane,
old_plane_state,
new_plane_state,
false,
_and_validation_needed,
_top_most_overlay);
+
if (ret) {
DRM_DEBUG_DRIVER("dm_update_plane_state() failed\n");
goto fail;
@@ -10069,6 +10076,7 @@ static int amdgpu_dm_atomic_check(struct drm_device 
*dev,
   new_crtc_state,
   false,
   _and_validation_needed);
+
if (ret) {
DRM_DEBUG_DRIVER("DISABLE: dm_update_crtc_state() 
failed\n");
goto fail;
@@ -10297,9 +10305,18 @@ static int amdgpu_dm_atomic_check(struct drm_device 
*dev,
struct dm_crtc_state *dm_new_crtc_state =
to_dm_crtc_state(new_crtc_state);
 
-   dm_new_crtc_state->update_type = lock_and_validation_needed ?
-UPDATE_TYPE_FULL :
-UPDATE_TYPE_FAST;
+   /*
+* Only allow async flips for fast updates that don't change
+* the FB pitch, the DCC state, rotation, etc.
+*/
+   if (new_crtc_state->async_flip && lock_and_validation_needed) {
+   drm_dbg_atomic(crtc->dev,
+  "[CRTC:%d:%s] async flips are only 
supported for fast updates\n",
+  crtc->base.id, crtc->name);
+   ret = -EINVAL;
+   goto fail;
+   } else
+   dm_new_crtc_state->update_type = UPDATE_TYPE_FAST;
}
 
/* Must be success */
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
index 30d4c6fd95f5..440fc0869a34 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
@@ -398,18 +398,6 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc 
*crtc,
return -EINVAL;
}
 
-   /*
-* Only allow async flips for fast updates that don't change the FB
-* pitch, the DCC state, rotation, etc.
-*/
-   if (crtc_state->async_flip &&
-   dm_crtc_state->update_type != UPDATE_TYPE_FAST) {
-   drm_dbg_atomic(crtc->dev,
-  "[CRTC:%d:%s] async flips are only supported for 
fast updates\n",
-