Re: [PATCH] drm/amd/display: add basic atomic check for cursor plane
Hi, On Monday, March 30, 2020 5:23 PM, Simon Ser wrote: > On Monday, March 30, 2020 5:18 PM, Kazlauskas, Nicholas > nicholas.kazlaus...@amd.com wrote: > > > On 2020-03-30 9:13 a.m., Simon Ser wrote: > > > > > On Monday, March 30, 2020 3:11 PM, Kazlauskas, Nicholas > > > nicholas.kazlaus...@amd.com wrote: > > > > > > > On 2020-03-30 9:02 a.m., Simon Ser wrote: > > > > > > > > > On Monday, March 30, 2020 2:59 PM, Kazlauskas, Nicholas > > > > > nicholas.kazlaus...@amd.com wrote: > > > > > > > > > > > We've been doing these checks for position before but I don't think > > > > > > we > > > > > > really need them. DC should be disabling the cursor when we ask for > > > > > > a > > > > > > position completely off the screen. > > > > > > I think that's better than rejecting the commit entirely at least. > > > > > > > > > > I agree DC should be disabling the cursor in this case, however that's > > > > > not yet implemented right? I think implementing this feature is > > > > > orthogonal and should be done in a separate patch. > > > > > This patch simply copies over the cursor checks in the atomic check > > > > > function. > > > > > > > > It's implemented on DCN but I don't remember if we're doing it on DCE. > > > > I guess the drop can be in a separate patch. > > > > > > > > Reviewed-by: Nicholas Kazlauskas nicholas.kazlaus...@amd.com > > > > > > Thanks for the review. I'll try to figure out whether we can drop this > > > check (from both the atomic check and the other existing check). > > > > Oh, this was actually the checks for crtc_w/crtc_h. Not the x/y, my bad. > > We probably can't drop this from here, but we can drop it from > > get_cursor_position after this patch - since it's now in the atomic check. > > Hmm, sorry I think I missed something. This patch does copy over the > x/y checks. We need to keep the w/h checks right? > > Yeah, we can probably drop get_cursor_position checks indeed. Gentle ping: any chance to get this patch merged? Thanks, Simon ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: add basic atomic check for cursor plane
On Monday, March 30, 2020 5:18 PM, Kazlauskas, Nicholas wrote: > On 2020-03-30 9:13 a.m., Simon Ser wrote: > > > On Monday, March 30, 2020 3:11 PM, Kazlauskas, Nicholas > > nicholas.kazlaus...@amd.com wrote: > > > > > On 2020-03-30 9:02 a.m., Simon Ser wrote: > > > > > > > On Monday, March 30, 2020 2:59 PM, Kazlauskas, Nicholas > > > > nicholas.kazlaus...@amd.com wrote: > > > > > > > > > We've been doing these checks for position before but I don't think we > > > > > really need them. DC should be disabling the cursor when we ask for a > > > > > position completely off the screen. > > > > > I think that's better than rejecting the commit entirely at least. > > > > > > > > I agree DC should be disabling the cursor in this case, however that's > > > > not yet implemented right? I think implementing this feature is > > > > orthogonal and should be done in a separate patch. > > > > This patch simply copies over the cursor checks in the atomic check > > > > function. > > > > > > It's implemented on DCN but I don't remember if we're doing it on DCE. > > > I guess the drop can be in a separate patch. > > > Reviewed-by: Nicholas Kazlauskas nicholas.kazlaus...@amd.com > > > > Thanks for the review. I'll try to figure out whether we can drop this > > check (from both the atomic check and the other existing check). > > Oh, this was actually the checks for crtc_w/crtc_h. Not the x/y, my bad. > > We probably can't drop this from here, but we can drop it from > get_cursor_position after this patch - since it's now in the atomic check. Hmm, sorry I think I missed something. This patch does copy over the x/y checks. We need to keep the w/h checks right? Yeah, we can probably drop get_cursor_position checks indeed. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: add basic atomic check for cursor plane
On 2020-03-30 9:13 a.m., Simon Ser wrote: On Monday, March 30, 2020 3:11 PM, Kazlauskas, Nicholas wrote: On 2020-03-30 9:02 a.m., Simon Ser wrote: On Monday, March 30, 2020 2:59 PM, Kazlauskas, Nicholas nicholas.kazlaus...@amd.com wrote: We've been doing these checks for position before but I don't think we really need them. DC should be disabling the cursor when we ask for a position completely off the screen. I think that's better than rejecting the commit entirely at least. I agree DC should be disabling the cursor in this case, however that's not yet implemented right? I think implementing this feature is orthogonal and should be done in a separate patch. This patch simply copies over the cursor checks in the atomic check function. It's implemented on DCN but I don't remember if we're doing it on DCE. I guess the drop can be in a separate patch. Reviewed-by: Nicholas Kazlauskas Thanks for the review. I'll try to figure out whether we can drop this check (from both the atomic check and the other existing check). Oh, this was actually the checks for crtc_w/crtc_h. Not the x/y, my bad. We probably can't drop this from here, but we can drop it from get_cursor_position after this patch - since it's now in the atomic check. Thanks, Nicholas Kazlauskas ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: add basic atomic check for cursor plane
On Monday, March 30, 2020 3:11 PM, Kazlauskas, Nicholas wrote: > On 2020-03-30 9:02 a.m., Simon Ser wrote: > > > On Monday, March 30, 2020 2:59 PM, Kazlauskas, Nicholas > > nicholas.kazlaus...@amd.com wrote: > > > > > We've been doing these checks for position before but I don't think we > > > really need them. DC should be disabling the cursor when we ask for a > > > position completely off the screen. > > > I think that's better than rejecting the commit entirely at least. > > > > I agree DC should be disabling the cursor in this case, however that's > > not yet implemented right? I think implementing this feature is > > orthogonal and should be done in a separate patch. > > This patch simply copies over the cursor checks in the atomic check > > function. > > It's implemented on DCN but I don't remember if we're doing it on DCE. > > I guess the drop can be in a separate patch. > > Reviewed-by: Nicholas Kazlauskas Thanks for the review. I'll try to figure out whether we can drop this check (from both the atomic check and the other existing check). ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: add basic atomic check for cursor plane
On 2020-03-30 9:02 a.m., Simon Ser wrote: On Monday, March 30, 2020 2:59 PM, Kazlauskas, Nicholas wrote: We've been doing these checks for position before but I don't think we really need them. DC should be disabling the cursor when we ask for a position completely off the screen. I think that's better than rejecting the commit entirely at least. I agree DC should be disabling the cursor in this case, however that's not yet implemented right? I think implementing this feature is orthogonal and should be done in a separate patch. This patch simply copies over the cursor checks in the atomic check function. It's implemented on DCN but I don't remember if we're doing it on DCE. I guess the drop can be in a separate patch. Reviewed-by: Nicholas Kazlauskas Regards, Nicholas Kazlauskas ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: add basic atomic check for cursor plane
On Monday, March 30, 2020 2:59 PM, Kazlauskas, Nicholas wrote: > We've been doing these checks for position before but I don't think we > really need them. DC should be disabling the cursor when we ask for a > position completely off the screen. > > I think that's better than rejecting the commit entirely at least. I agree DC should be disabling the cursor in this case, however that's not yet implemented right? I think implementing this feature is orthogonal and should be done in a separate patch. This patch simply copies over the cursor checks in the atomic check function. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: add basic atomic check for cursor plane
On 2020-03-30 5:23 a.m., Simon Ser wrote:
This patch adds a basic cursor check when an atomic test-only commit is
performed. The position and size of the cursor plane is checked.
This should fix user-space relying on atomic checks to assign buffers to
planes.
Signed-off-by: Simon Ser
Reported-by: Roman Gilg
References: https://github.com/emersion/libliftoff/issues/46
Cc: Alex Deucher
Cc: Harry Wentland
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 26 +--
1 file changed, 24 insertions(+), 2 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 f6b0b9a121fd..e1b084318ad6 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7843,6 +7843,7 @@ static int dm_update_plane_state(struct dc *dc,
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
struct dm_crtc_state *dm_new_crtc_state, *dm_old_crtc_state;
struct dm_plane_state *dm_new_plane_state, *dm_old_plane_state;
+ struct amdgpu_crtc *new_acrtc;
bool needs_reset;
int ret = 0;
@@ -7852,9 +7853,30 @@ static int dm_update_plane_state(struct dc *dc,
dm_new_plane_state = to_dm_plane_state(new_plane_state);
dm_old_plane_state = to_dm_plane_state(old_plane_state);
- /*TODO Implement atomic check for cursor plane */
- if (plane->type == DRM_PLANE_TYPE_CURSOR)
+ /*TODO Implement better atomic check for cursor plane */
+ if (plane->type == DRM_PLANE_TYPE_CURSOR) {
+ if (!enable || !new_plane_crtc ||
+ drm_atomic_plane_disabling(plane->state,
new_plane_state))
+ return 0;
+
+ new_acrtc = to_amdgpu_crtc(new_plane_crtc);
+
+ if ((new_plane_state->crtc_w > new_acrtc->max_cursor_width) ||
+ (new_plane_state->crtc_h >
new_acrtc->max_cursor_height)) {
+ DRM_DEBUG_ATOMIC("Bad cursor size %d x %d\n",
+new_plane_state->crtc_w,
new_plane_state->crtc_h);
+ return -EINVAL;
+ }
+
+ if (new_plane_state->crtc_x <= -new_acrtc->max_cursor_width ||
+ new_plane_state->crtc_y <=
-new_acrtc->max_cursor_height) {
+ DRM_DEBUG_ATOMIC("Bad cursor position %d, %d\n",
+new_plane_state->crtc_x,
new_plane_state->crtc_y);
+ return -EINVAL;
We've been doing these checks for position before but I don't think we
really need them. DC should be disabling the cursor when we ask for a
position completely off the screen.
I think that's better than rejecting the commit entirely at least.
Nicholas Kazlauskas
+ }
+
return 0;
+ }
needs_reset = should_reset_plane(state, plane, old_plane_state,
new_plane_state);
--
2.26.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
