We currently don't support dirty rectangles on hardware rotated modes. So, if a user is using hardware rotated modes with PSR-SU enabled, use PSR-SU FFU for all rotated planes (including cursor planes).
Cc: stable@vger.kernel.org Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2952 Fixes: 30ebe41582d1 ("drm/amd/display: add FB_DAMAGE_CLIPS support") Signed-off-by: Hamza Mahfooz hamza.mahfooz@amd.com --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++ drivers/gpu/drm/amd/display/dc/dc_hw_types.h | 1 + drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c | 12 ++++++++++-- .../gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c | 3 ++- 4 files changed, 17 insertions(+), 3 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 c146dc9cba92..79f8102d2601 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5208,6 +5208,7 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, bool bb_changed; bool fb_changed; u32 i = 0; + *dirty_regions_changed = false;
/* @@ -5217,6 +5218,9 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, if (plane->type == DRM_PLANE_TYPE_CURSOR) return;
+ if (new_plane_state->rotation != DRM_MODE_ROTATE_0) + goto ffu; + num_clips = drm_plane_get_damage_clips_count(new_plane_state); clips = drm_plane_get_damage_clips(new_plane_state);
diff --git a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h index 9649934ea186..e2a3aa8812df 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h +++ b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h @@ -465,6 +465,7 @@ struct dc_cursor_mi_param { struct fixed31_32 v_scale_ratio; enum dc_rotation_angle rotation; bool mirror; + struct dc_stream_state *stream; };
/* IPP related types */ diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c index 139cf31d2e45..89c3bf0fe0c9 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c @@ -1077,8 +1077,16 @@ void hubp2_cursor_set_position( if (src_y_offset < 0) src_y_offset = 0; /* Save necessary cursor info x, y position. w, h is saved in attribute func. */ - hubp->cur_rect.x = src_x_offset + param->viewport.x; - hubp->cur_rect.y = src_y_offset + param->viewport.y; + if (param->stream->link->psr_settings.psr_version >= DC_PSR_VERSION_SU_1 && + param->rotation != ROTATION_ANGLE_0) { + hubp->cur_rect.x = 0; + hubp->cur_rect.y = 0; + hubp->cur_rect.w = param->stream->timing.h_addressable; + hubp->cur_rect.h = param->stream->timing.v_addressable; + } else { + hubp->cur_rect.x = src_x_offset + param->viewport.x; + hubp->cur_rect.y = src_y_offset + param->viewport.y; + } }
void hubp2_clk_cntl(struct hubp *hubp, bool enable) diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c index 2b8b8366538e..ce5613a76267 100644 --- a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c +++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c @@ -3417,7 +3417,8 @@ void dcn10_set_cursor_position(struct pipe_ctx *pipe_ctx) .h_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.horz, .v_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.vert, .rotation = pipe_ctx->plane_state->rotation, - .mirror = pipe_ctx->plane_state->horizontal_mirror + .mirror = pipe_ctx->plane_state->horizontal_mirror, + .stream = pipe_ctx->stream }; bool pipe_split_on = false; bool odm_combine_on = (pipe_ctx->next_odm_pipe != NULL) ||
On 12/5/2023 14:17, Hamza Mahfooz wrote:
We currently don't support dirty rectangles on hardware rotated modes. So, if a user is using hardware rotated modes with PSR-SU enabled, use PSR-SU FFU for all rotated planes (including cursor planes).
Here is the email for the original reporter to give an attribution tag.
Reported-by: Kai-Heng Feng kai.heng.feng@canonical.com
Cc: stable@vger.kernel.org Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2952 Fixes: 30ebe41582d1 ("drm/amd/display: add FB_DAMAGE_CLIPS support") Signed-off-by: Hamza Mahfooz hamza.mahfooz@amd.com
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++ drivers/gpu/drm/amd/display/dc/dc_hw_types.h | 1 + drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c | 12 ++++++++++-- .../gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c | 3 ++- 4 files changed, 17 insertions(+), 3 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 c146dc9cba92..79f8102d2601 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5208,6 +5208,7 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, bool bb_changed; bool fb_changed; u32 i = 0;
Looks like a spurious newline here.
*dirty_regions_changed = false; /* @@ -5217,6 +5218,9 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, if (plane->type == DRM_PLANE_TYPE_CURSOR) return;
- if (new_plane_state->rotation != DRM_MODE_ROTATE_0)
goto ffu;
I noticed that the original report was specifically on 180°. Since you're also covering 90° and 270° with this check it sounds like it's actually problematic on those too?
num_clips = drm_plane_get_damage_clips_count(new_plane_state); clips = drm_plane_get_damage_clips(new_plane_state); diff --git a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h index 9649934ea186..e2a3aa8812df 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h +++ b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h @@ -465,6 +465,7 @@ struct dc_cursor_mi_param { struct fixed31_32 v_scale_ratio; enum dc_rotation_angle rotation; bool mirror;
- struct dc_stream_state *stream; };
/* IPP related types */ diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c index 139cf31d2e45..89c3bf0fe0c9 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c @@ -1077,8 +1077,16 @@ void hubp2_cursor_set_position( if (src_y_offset < 0) src_y_offset = 0; /* Save necessary cursor info x, y position. w, h is saved in attribute func. */
- hubp->cur_rect.x = src_x_offset + param->viewport.x;
- hubp->cur_rect.y = src_y_offset + param->viewport.y;
- if (param->stream->link->psr_settings.psr_version >= DC_PSR_VERSION_SU_1 &&
param->rotation != ROTATION_ANGLE_0) {
Ditto on above about 90° and 270°.
hubp->cur_rect.x = 0;hubp->cur_rect.y = 0;hubp->cur_rect.w = param->stream->timing.h_addressable;hubp->cur_rect.h = param->stream->timing.v_addressable;- } else {
hubp->cur_rect.x = src_x_offset + param->viewport.x;hubp->cur_rect.y = src_y_offset + param->viewport.y;- } }
void hubp2_clk_cntl(struct hubp *hubp, bool enable) diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c index 2b8b8366538e..ce5613a76267 100644 --- a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c +++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c @@ -3417,7 +3417,8 @@ void dcn10_set_cursor_position(struct pipe_ctx *pipe_ctx) .h_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.horz, .v_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.vert, .rotation = pipe_ctx->plane_state->rotation,
.mirror = pipe_ctx->plane_state->horizontal_mirror
.mirror = pipe_ctx->plane_state->horizontal_mirror,.stream = pipe_ctx->stream
As a nit; I think it's worth leaving a harmless trailing ',' so that there is less ping pong in the future when adding new members to a struct.
}; bool pipe_split_on = false; bool odm_combine_on = (pipe_ctx->next_odm_pipe != NULL) ||
On Wed, Dec 6, 2023 at 4:29 AM Mario Limonciello mario.limonciello@amd.com wrote:
On 12/5/2023 14:17, Hamza Mahfooz wrote:
We currently don't support dirty rectangles on hardware rotated modes. So, if a user is using hardware rotated modes with PSR-SU enabled, use PSR-SU FFU for all rotated planes (including cursor planes).
Here is the email for the original reporter to give an attribution tag.
Reported-by: Kai-Heng Feng kai.heng.feng@canonical.com
For this particular issue, Tested-by: Kai-Heng Feng kai.heng.feng@canonical.com
Cc: stable@vger.kernel.org Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2952 Fixes: 30ebe41582d1 ("drm/amd/display: add FB_DAMAGE_CLIPS support") Signed-off-by: Hamza Mahfooz hamza.mahfooz@amd.com
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++ drivers/gpu/drm/amd/display/dc/dc_hw_types.h | 1 + drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c | 12 ++++++++++-- .../gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c | 3 ++- 4 files changed, 17 insertions(+), 3 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 c146dc9cba92..79f8102d2601 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5208,6 +5208,7 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, bool bb_changed; bool fb_changed; u32 i = 0;
Looks like a spurious newline here.
*dirty_regions_changed = false; /*@@ -5217,6 +5218,9 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, if (plane->type == DRM_PLANE_TYPE_CURSOR) return;
if (new_plane_state->rotation != DRM_MODE_ROTATE_0)goto ffu;I noticed that the original report was specifically on 180°. Since you're also covering 90° and 270° with this check it sounds like it's actually problematic on those too?
90 & 270 are problematic too. But from what I observed the issue is much more than just cursors.
Kai-Heng
num_clips = drm_plane_get_damage_clips_count(new_plane_state); clips = drm_plane_get_damage_clips(new_plane_state);diff --git a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h index 9649934ea186..e2a3aa8812df 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h +++ b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h @@ -465,6 +465,7 @@ struct dc_cursor_mi_param { struct fixed31_32 v_scale_ratio; enum dc_rotation_angle rotation; bool mirror;
struct dc_stream_state *stream;};
/* IPP related types */
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c index 139cf31d2e45..89c3bf0fe0c9 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c @@ -1077,8 +1077,16 @@ void hubp2_cursor_set_position( if (src_y_offset < 0) src_y_offset = 0; /* Save necessary cursor info x, y position. w, h is saved in attribute func. */
hubp->cur_rect.x = src_x_offset + param->viewport.x;hubp->cur_rect.y = src_y_offset + param->viewport.y;
if (param->stream->link->psr_settings.psr_version >= DC_PSR_VERSION_SU_1 &¶m->rotation != ROTATION_ANGLE_0) {Ditto on above about 90° and 270°.
hubp->cur_rect.x = 0;hubp->cur_rect.y = 0;hubp->cur_rect.w = param->stream->timing.h_addressable;hubp->cur_rect.h = param->stream->timing.v_addressable;} else {hubp->cur_rect.x = src_x_offset + param->viewport.x;hubp->cur_rect.y = src_y_offset + param->viewport.y;}}
void hubp2_clk_cntl(struct hubp *hubp, bool enable)
diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c index 2b8b8366538e..ce5613a76267 100644 --- a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c +++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c @@ -3417,7 +3417,8 @@ void dcn10_set_cursor_position(struct pipe_ctx *pipe_ctx) .h_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.horz, .v_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.vert, .rotation = pipe_ctx->plane_state->rotation,
.mirror = pipe_ctx->plane_state->horizontal_mirror
.mirror = pipe_ctx->plane_state->horizontal_mirror,.stream = pipe_ctx->streamAs a nit; I think it's worth leaving a harmless trailing ',' so that there is less ping pong in the future when adding new members to a struct.
}; bool pipe_split_on = false; bool odm_combine_on = (pipe_ctx->next_odm_pipe != NULL) ||
On 12/6/2023 19:23, Kai-Heng Feng wrote:
On Wed, Dec 6, 2023 at 4:29 AM Mario Limonciello mario.limonciello@amd.com wrote:
On 12/5/2023 14:17, Hamza Mahfooz wrote:
We currently don't support dirty rectangles on hardware rotated modes. So, if a user is using hardware rotated modes with PSR-SU enabled, use PSR-SU FFU for all rotated planes (including cursor planes).
Here is the email for the original reporter to give an attribution tag.
Reported-by: Kai-Heng Feng kai.heng.feng@canonical.com
For this particular issue, Tested-by: Kai-Heng Feng kai.heng.feng@canonical.com
Can you confirm what kernel base you tested issue against?
I ask because Bin Li (+CC) also tested it against 6.1 based LTS kernel but ran into problems.
I wonder if it's because of other dependency patches. If that's the case it would be good to call them out in the Cc: @stable as dependencies so when Greg or Sasha backport this 6.1 doesn't get broken.
Bin,
Could you run ./scripts/decode_stacktrace.sh on your kernel trace to give us a specific line number on the issue you hit?
Thanks!
Cc: stable@vger.kernel.org Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2952 Fixes: 30ebe41582d1 ("drm/amd/display: add FB_DAMAGE_CLIPS support") Signed-off-by: Hamza Mahfooz hamza.mahfooz@amd.com
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++ drivers/gpu/drm/amd/display/dc/dc_hw_types.h | 1 + drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c | 12 ++++++++++-- .../gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c | 3 ++- 4 files changed, 17 insertions(+), 3 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 c146dc9cba92..79f8102d2601 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5208,6 +5208,7 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, bool bb_changed; bool fb_changed; u32 i = 0;
Looks like a spurious newline here.
*dirty_regions_changed = false; /*@@ -5217,6 +5218,9 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, if (plane->type == DRM_PLANE_TYPE_CURSOR) return;
if (new_plane_state->rotation != DRM_MODE_ROTATE_0)goto ffu;I noticed that the original report was specifically on 180°. Since you're also covering 90° and 270° with this check it sounds like it's actually problematic on those too?
90 & 270 are problematic too. But from what I observed the issue is much more than just cursors.
Got it; thanks.
Kai-Heng
num_clips = drm_plane_get_damage_clips_count(new_plane_state); clips = drm_plane_get_damage_clips(new_plane_state);diff --git a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h index 9649934ea186..e2a3aa8812df 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h +++ b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h @@ -465,6 +465,7 @@ struct dc_cursor_mi_param { struct fixed31_32 v_scale_ratio; enum dc_rotation_angle rotation; bool mirror;
struct dc_stream_state *stream;};
/* IPP related types */
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c index 139cf31d2e45..89c3bf0fe0c9 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c @@ -1077,8 +1077,16 @@ void hubp2_cursor_set_position( if (src_y_offset < 0) src_y_offset = 0; /* Save necessary cursor info x, y position. w, h is saved in attribute func. */
hubp->cur_rect.x = src_x_offset + param->viewport.x;hubp->cur_rect.y = src_y_offset + param->viewport.y;
if (param->stream->link->psr_settings.psr_version >= DC_PSR_VERSION_SU_1 &¶m->rotation != ROTATION_ANGLE_0) {Ditto on above about 90° and 270°.
hubp->cur_rect.x = 0;hubp->cur_rect.y = 0;hubp->cur_rect.w = param->stream->timing.h_addressable;hubp->cur_rect.h = param->stream->timing.v_addressable;} else {hubp->cur_rect.x = src_x_offset + param->viewport.x;hubp->cur_rect.y = src_y_offset + param->viewport.y;}}
void hubp2_clk_cntl(struct hubp *hubp, bool enable)
diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c index 2b8b8366538e..ce5613a76267 100644 --- a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c +++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c @@ -3417,7 +3417,8 @@ void dcn10_set_cursor_position(struct pipe_ctx *pipe_ctx) .h_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.horz, .v_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.vert, .rotation = pipe_ctx->plane_state->rotation,
.mirror = pipe_ctx->plane_state->horizontal_mirror
.mirror = pipe_ctx->plane_state->horizontal_mirror,.stream = pipe_ctx->streamAs a nit; I think it's worth leaving a harmless trailing ',' so that there is less ping pong in the future when adding new members to a struct.
}; bool pipe_split_on = false; bool odm_combine_on = (pipe_ctx->next_odm_pipe != NULL) ||
On Thu, Dec 7, 2023 at 9:57 AM Mario Limonciello mario.limonciello@amd.com wrote:
On 12/6/2023 19:23, Kai-Heng Feng wrote:
On Wed, Dec 6, 2023 at 4:29 AM Mario Limonciello mario.limonciello@amd.com wrote:
On 12/5/2023 14:17, Hamza Mahfooz wrote:
We currently don't support dirty rectangles on hardware rotated modes. So, if a user is using hardware rotated modes with PSR-SU enabled, use PSR-SU FFU for all rotated planes (including cursor planes).
Here is the email for the original reporter to give an attribution tag.
Reported-by: Kai-Heng Feng kai.heng.feng@canonical.com
For this particular issue, Tested-by: Kai-Heng Feng kai.heng.feng@canonical.com
Can you confirm what kernel base you tested issue against?
I ask because Bin Li (+CC) also tested it against 6.1 based LTS kernel but ran into problems.
The patch was tested against ADSN.
I wonder if it's because of other dependency patches. If that's the case it would be good to call them out in the Cc: @stable as dependencies so when Greg or Sasha backport this 6.1 doesn't get broken.
Probably. I haven't really tested any older kernel series.
Kai-Heng
Bin,
Could you run ./scripts/decode_stacktrace.sh on your kernel trace to give us a specific line number on the issue you hit?
Thanks!
Cc: stable@vger.kernel.org Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2952 Fixes: 30ebe41582d1 ("drm/amd/display: add FB_DAMAGE_CLIPS support") Signed-off-by: Hamza Mahfooz hamza.mahfooz@amd.com
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++ drivers/gpu/drm/amd/display/dc/dc_hw_types.h | 1 + drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c | 12 ++++++++++-- .../gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c | 3 ++- 4 files changed, 17 insertions(+), 3 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 c146dc9cba92..79f8102d2601 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5208,6 +5208,7 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, bool bb_changed; bool fb_changed; u32 i = 0;
Looks like a spurious newline here.
*dirty_regions_changed = false; /*@@ -5217,6 +5218,9 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, if (plane->type == DRM_PLANE_TYPE_CURSOR) return;
if (new_plane_state->rotation != DRM_MODE_ROTATE_0)goto ffu;I noticed that the original report was specifically on 180°. Since you're also covering 90° and 270° with this check it sounds like it's actually problematic on those too?
90 & 270 are problematic too. But from what I observed the issue is much more than just cursors.
Got it; thanks.
Kai-Heng
num_clips = drm_plane_get_damage_clips_count(new_plane_state); clips = drm_plane_get_damage_clips(new_plane_state);diff --git a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h index 9649934ea186..e2a3aa8812df 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h +++ b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h @@ -465,6 +465,7 @@ struct dc_cursor_mi_param { struct fixed31_32 v_scale_ratio; enum dc_rotation_angle rotation; bool mirror;
struct dc_stream_state *stream;};
/* IPP related types */
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c index 139cf31d2e45..89c3bf0fe0c9 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c @@ -1077,8 +1077,16 @@ void hubp2_cursor_set_position( if (src_y_offset < 0) src_y_offset = 0; /* Save necessary cursor info x, y position. w, h is saved in attribute func. */
hubp->cur_rect.x = src_x_offset + param->viewport.x;hubp->cur_rect.y = src_y_offset + param->viewport.y;
if (param->stream->link->psr_settings.psr_version >= DC_PSR_VERSION_SU_1 &¶m->rotation != ROTATION_ANGLE_0) {Ditto on above about 90° and 270°.
hubp->cur_rect.x = 0;hubp->cur_rect.y = 0;hubp->cur_rect.w = param->stream->timing.h_addressable;hubp->cur_rect.h = param->stream->timing.v_addressable;} else {hubp->cur_rect.x = src_x_offset + param->viewport.x;hubp->cur_rect.y = src_y_offset + param->viewport.y;}}
void hubp2_clk_cntl(struct hubp *hubp, bool enable)
diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c index 2b8b8366538e..ce5613a76267 100644 --- a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c +++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c @@ -3417,7 +3417,8 @@ void dcn10_set_cursor_position(struct pipe_ctx *pipe_ctx) .h_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.horz, .v_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.vert, .rotation = pipe_ctx->plane_state->rotation,
.mirror = pipe_ctx->plane_state->horizontal_mirror
.mirror = pipe_ctx->plane_state->horizontal_mirror,.stream = pipe_ctx->streamAs a nit; I think it's worth leaving a harmless trailing ',' so that there is less ping pong in the future when adding new members to a struct.
}; bool pipe_split_on = false; bool odm_combine_on = (pipe_ctx->next_odm_pipe != NULL) ||
On 12/6/2023 20:07, Kai-Heng Feng wrote:
On Thu, Dec 7, 2023 at 9:57 AM Mario Limonciello mario.limonciello@amd.com wrote:
On 12/6/2023 19:23, Kai-Heng Feng wrote:
On Wed, Dec 6, 2023 at 4:29 AM Mario Limonciello mario.limonciello@amd.com wrote:
On 12/5/2023 14:17, Hamza Mahfooz wrote:
We currently don't support dirty rectangles on hardware rotated modes. So, if a user is using hardware rotated modes with PSR-SU enabled, use PSR-SU FFU for all rotated planes (including cursor planes).
Here is the email for the original reporter to give an attribution tag.
Reported-by: Kai-Heng Feng kai.heng.feng@canonical.com
For this particular issue, Tested-by: Kai-Heng Feng kai.heng.feng@canonical.com
Can you confirm what kernel base you tested issue against?
I ask because Bin Li (+CC) also tested it against 6.1 based LTS kernel but ran into problems.
The patch was tested against ADSN.
I wonder if it's because of other dependency patches. If that's the case it would be good to call them out in the Cc: @stable as dependencies so when Greg or Sasha backport this 6.1 doesn't get broken.
Probably. I haven't really tested any older kernel series.
Since you've got a good environment to test it and reproduce it would you mind double checking it against 6.7-rc, 6.5 and 6.1 trees? If we don't have confidence it works on the older trees I think we'll need to drop the stable tag.
Kai-Heng
Bin,
Could you run ./scripts/decode_stacktrace.sh on your kernel trace to give us a specific line number on the issue you hit?
Thanks!
Cc: stable@vger.kernel.org Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2952 Fixes: 30ebe41582d1 ("drm/amd/display: add FB_DAMAGE_CLIPS support") Signed-off-by: Hamza Mahfooz hamza.mahfooz@amd.com
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++ drivers/gpu/drm/amd/display/dc/dc_hw_types.h | 1 + drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c | 12 ++++++++++-- .../gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c | 3 ++- 4 files changed, 17 insertions(+), 3 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 c146dc9cba92..79f8102d2601 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5208,6 +5208,7 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, bool bb_changed; bool fb_changed; u32 i = 0;
Looks like a spurious newline here.
*dirty_regions_changed = false; /*@@ -5217,6 +5218,9 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, if (plane->type == DRM_PLANE_TYPE_CURSOR) return;
if (new_plane_state->rotation != DRM_MODE_ROTATE_0)goto ffu;I noticed that the original report was specifically on 180°. Since you're also covering 90° and 270° with this check it sounds like it's actually problematic on those too?
90 & 270 are problematic too. But from what I observed the issue is much more than just cursors.
Got it; thanks.
Kai-Heng
num_clips = drm_plane_get_damage_clips_count(new_plane_state); clips = drm_plane_get_damage_clips(new_plane_state);diff --git a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h index 9649934ea186..e2a3aa8812df 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h +++ b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h @@ -465,6 +465,7 @@ struct dc_cursor_mi_param { struct fixed31_32 v_scale_ratio; enum dc_rotation_angle rotation; bool mirror;
struct dc_stream_state *stream;};
/* IPP related types */
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c index 139cf31d2e45..89c3bf0fe0c9 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c @@ -1077,8 +1077,16 @@ void hubp2_cursor_set_position( if (src_y_offset < 0) src_y_offset = 0; /* Save necessary cursor info x, y position. w, h is saved in attribute func. */
hubp->cur_rect.x = src_x_offset + param->viewport.x;hubp->cur_rect.y = src_y_offset + param->viewport.y;
if (param->stream->link->psr_settings.psr_version >= DC_PSR_VERSION_SU_1 &¶m->rotation != ROTATION_ANGLE_0) {Ditto on above about 90° and 270°.
hubp->cur_rect.x = 0;hubp->cur_rect.y = 0;hubp->cur_rect.w = param->stream->timing.h_addressable;hubp->cur_rect.h = param->stream->timing.v_addressable;} else {hubp->cur_rect.x = src_x_offset + param->viewport.x;hubp->cur_rect.y = src_y_offset + param->viewport.y;}}
void hubp2_clk_cntl(struct hubp *hubp, bool enable)
diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c index 2b8b8366538e..ce5613a76267 100644 --- a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c +++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c @@ -3417,7 +3417,8 @@ void dcn10_set_cursor_position(struct pipe_ctx *pipe_ctx) .h_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.horz, .v_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.vert, .rotation = pipe_ctx->plane_state->rotation,
.mirror = pipe_ctx->plane_state->horizontal_mirror
.mirror = pipe_ctx->plane_state->horizontal_mirror,.stream = pipe_ctx->streamAs a nit; I think it's worth leaving a harmless trailing ',' so that there is less ping pong in the future when adding new members to a struct.
}; bool pipe_split_on = false; bool odm_combine_on = (pipe_ctx->next_odm_pipe != NULL) ||
On Thu, Dec 7, 2023 at 10:10 AM Mario Limonciello mario.limonciello@amd.com wrote:
On 12/6/2023 20:07, Kai-Heng Feng wrote:
On Thu, Dec 7, 2023 at 9:57 AM Mario Limonciello mario.limonciello@amd.com wrote:
On 12/6/2023 19:23, Kai-Heng Feng wrote:
On Wed, Dec 6, 2023 at 4:29 AM Mario Limonciello mario.limonciello@amd.com wrote:
On 12/5/2023 14:17, Hamza Mahfooz wrote:
We currently don't support dirty rectangles on hardware rotated modes. So, if a user is using hardware rotated modes with PSR-SU enabled, use PSR-SU FFU for all rotated planes (including cursor planes).
Here is the email for the original reporter to give an attribution tag.
Reported-by: Kai-Heng Feng kai.heng.feng@canonical.com
For this particular issue, Tested-by: Kai-Heng Feng kai.heng.feng@canonical.com
Can you confirm what kernel base you tested issue against?
I ask because Bin Li (+CC) also tested it against 6.1 based LTS kernel but ran into problems.
The patch was tested against ADSN.
I wonder if it's because of other dependency patches. If that's the case it would be good to call them out in the Cc: @stable as dependencies so when Greg or Sasha backport this 6.1 doesn't get broken.
Probably. I haven't really tested any older kernel series.
Since you've got a good environment to test it and reproduce it would you mind double checking it against 6.7-rc, 6.5 and 6.1 trees? If we don't have confidence it works on the older trees I think we'll need to drop the stable tag.
Not seeing issues here when the patch is applied against 6.5 and 6.1 (which needs to resolve a minor conflict).
I am not sure what happened for Bin's case.
Kai-Heng
Kai-Heng
Bin,
Could you run ./scripts/decode_stacktrace.sh on your kernel trace to give us a specific line number on the issue you hit?
Thanks!
Cc: stable@vger.kernel.org Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2952 Fixes: 30ebe41582d1 ("drm/amd/display: add FB_DAMAGE_CLIPS support") Signed-off-by: Hamza Mahfooz hamza.mahfooz@amd.com
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++ drivers/gpu/drm/amd/display/dc/dc_hw_types.h | 1 + drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c | 12 ++++++++++-- .../gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c | 3 ++- 4 files changed, 17 insertions(+), 3 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 c146dc9cba92..79f8102d2601 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5208,6 +5208,7 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, bool bb_changed; bool fb_changed; u32 i = 0;
Looks like a spurious newline here.
*dirty_regions_changed = false; /*@@ -5217,6 +5218,9 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, if (plane->type == DRM_PLANE_TYPE_CURSOR) return;
if (new_plane_state->rotation != DRM_MODE_ROTATE_0)goto ffu;I noticed that the original report was specifically on 180°. Since you're also covering 90° and 270° with this check it sounds like it's actually problematic on those too?
90 & 270 are problematic too. But from what I observed the issue is much more than just cursors.
Got it; thanks.
Kai-Heng
num_clips = drm_plane_get_damage_clips_count(new_plane_state); clips = drm_plane_get_damage_clips(new_plane_state);diff --git a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h index 9649934ea186..e2a3aa8812df 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h +++ b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h @@ -465,6 +465,7 @@ struct dc_cursor_mi_param { struct fixed31_32 v_scale_ratio; enum dc_rotation_angle rotation; bool mirror;
struct dc_stream_state *stream;};
/* IPP related types */
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c index 139cf31d2e45..89c3bf0fe0c9 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c @@ -1077,8 +1077,16 @@ void hubp2_cursor_set_position( if (src_y_offset < 0) src_y_offset = 0; /* Save necessary cursor info x, y position. w, h is saved in attribute func. */
hubp->cur_rect.x = src_x_offset + param->viewport.x;hubp->cur_rect.y = src_y_offset + param->viewport.y;
if (param->stream->link->psr_settings.psr_version >= DC_PSR_VERSION_SU_1 &¶m->rotation != ROTATION_ANGLE_0) {Ditto on above about 90° and 270°.
hubp->cur_rect.x = 0;hubp->cur_rect.y = 0;hubp->cur_rect.w = param->stream->timing.h_addressable;hubp->cur_rect.h = param->stream->timing.v_addressable;} else {hubp->cur_rect.x = src_x_offset + param->viewport.x;hubp->cur_rect.y = src_y_offset + param->viewport.y;}}
void hubp2_clk_cntl(struct hubp *hubp, bool enable)
diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c index 2b8b8366538e..ce5613a76267 100644 --- a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c +++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c @@ -3417,7 +3417,8 @@ void dcn10_set_cursor_position(struct pipe_ctx *pipe_ctx) .h_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.horz, .v_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.vert, .rotation = pipe_ctx->plane_state->rotation,
.mirror = pipe_ctx->plane_state->horizontal_mirror
.mirror = pipe_ctx->plane_state->horizontal_mirror,.stream = pipe_ctx->streamAs a nit; I think it's worth leaving a harmless trailing ',' so that there is less ping pong in the future when adding new members to a struct.
}; bool pipe_split_on = false; bool odm_combine_on = (pipe_ctx->next_odm_pipe != NULL) ||
On 12/5/23 15:29, Mario Limonciello wrote:
On 12/5/2023 14:17, Hamza Mahfooz wrote:
We currently don't support dirty rectangles on hardware rotated modes. So, if a user is using hardware rotated modes with PSR-SU enabled, use PSR-SU FFU for all rotated planes (including cursor planes).
Here is the email for the original reporter to give an attribution tag.
Reported-by: Kai-Heng Feng kai.heng.feng@canonical.com
Cc: stable@vger.kernel.org Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2952 Fixes: 30ebe41582d1 ("drm/amd/display: add FB_DAMAGE_CLIPS support") Signed-off-by: Hamza Mahfooz hamza.mahfooz@amd.com
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++ drivers/gpu/drm/amd/display/dc/dc_hw_types.h | 1 + drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c | 12 ++++++++++-- .../gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c | 3 ++- 4 files changed, 17 insertions(+), 3 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 c146dc9cba92..79f8102d2601 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5208,6 +5208,7 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, bool bb_changed; bool fb_changed; u32 i = 0;
Looks like a spurious newline here.
*dirty_regions_changed = false; /* @@ -5217,6 +5218,9 @@ static void fill_dc_dirty_rects(struct drm_plane *plane, if (plane->type == DRM_PLANE_TYPE_CURSOR) return; + if (new_plane_state->rotation != DRM_MODE_ROTATE_0) + goto ffu;
I noticed that the original report was specifically on 180°. Since you're also covering 90° and 270° with this check it sounds like it's actually problematic on those too?
Ya it's problematic for 90 and 270 as well, though most mainstream compositors don't use hardware rotation for those cases under any circumstances. So, I doubt that many people would encounter this issue in the wild for them.
num_clips = drm_plane_get_damage_clips_count(new_plane_state); clips = drm_plane_get_damage_clips(new_plane_state); diff --git a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h index 9649934ea186..e2a3aa8812df 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_hw_types.h +++ b/drivers/gpu/drm/amd/display/dc/dc_hw_types.h @@ -465,6 +465,7 @@ struct dc_cursor_mi_param { struct fixed31_32 v_scale_ratio; enum dc_rotation_angle rotation; bool mirror; + struct dc_stream_state *stream; }; /* IPP related types */ diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c index 139cf31d2e45..89c3bf0fe0c9 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c @@ -1077,8 +1077,16 @@ void hubp2_cursor_set_position( if (src_y_offset < 0) src_y_offset = 0; /* Save necessary cursor info x, y position. w, h is saved in attribute func. */ - hubp->cur_rect.x = src_x_offset + param->viewport.x; - hubp->cur_rect.y = src_y_offset + param->viewport.y; + if (param->stream->link->psr_settings.psr_version >= DC_PSR_VERSION_SU_1 && + param->rotation != ROTATION_ANGLE_0) {
Ditto on above about 90° and 270°.
+ hubp->cur_rect.x = 0; + hubp->cur_rect.y = 0; + hubp->cur_rect.w = param->stream->timing.h_addressable; + hubp->cur_rect.h = param->stream->timing.v_addressable; + } else { + hubp->cur_rect.x = src_x_offset + param->viewport.x; + hubp->cur_rect.y = src_y_offset + param->viewport.y; + } } void hubp2_clk_cntl(struct hubp *hubp, bool enable) diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c index 2b8b8366538e..ce5613a76267 100644 --- a/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c +++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn10/dcn10_hwseq.c @@ -3417,7 +3417,8 @@ void dcn10_set_cursor_position(struct pipe_ctx *pipe_ctx) .h_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.horz, .v_scale_ratio = pipe_ctx->plane_res.scl_data.ratios.vert, .rotation = pipe_ctx->plane_state->rotation, - .mirror = pipe_ctx->plane_state->horizontal_mirror + .mirror = pipe_ctx->plane_state->horizontal_mirror, + .stream = pipe_ctx->stream
As a nit; I think it's worth leaving a harmless trailing ',' so that there is less ping pong in the future when adding new members to a struct.
}; bool pipe_split_on = false; bool odm_combine_on = (pipe_ctx->next_odm_pipe != NULL) ||
linux-stable-mirror@lists.linaro.org
-
Hamza Mahfooz -
Kai-Heng Feng -
Mario Limonciello