[PATCH 1/2] drm/amdgpu: Switch error code when bo pin fails. (v3)

[PATCH 1/2] drm/amdgpu: Switch error code when bo pin fails. (v3)

[Alex Deucher]

From: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>

Switching to -ENOMEM resolves a hang in case the driver
uses this function in atomic code path.

v2: Update commit message.
v3:
Remove forcing -EINVAL in case of amdgpu_bo_pin fail
since there is no apparent reason for not just passing
through the the return value.

Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Reviewed-by Harry Wentland <harry.wentland@amd.com>

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 48a3f67..4986340 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -189,7 +189,6 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
 
 	r = amdgpu_bo_pin(new_abo, AMDGPU_GEM_DOMAIN_VRAM, &base);
 	if (unlikely(r != 0)) {
-		r = -EINVAL;
 		DRM_ERROR("failed to pin new abo buffer before flip\n");
 		goto unreserve;
 	}
-- 
2.5.5

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdgpu: Refactor flip into prepare submit and submit. (v2)

[Alex Deucher]

From: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>

Make pflip atomic friendly. Split the fuinction into
whatever can fail part and the actual flip submit part.
Call the pre-submit function before atomic states
are swapped so in case of error we can fail the
IOCTL.

v2:
Update due to  target_vblank code change.
Fix identetation.
Change return type for amdgpu_crtc_submit_flip to void

Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
Reviewed-by Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 147 ++++++++++++++++++++++------
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h    |  17 ++++
 2 files changed, 135 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 4986340..d0ad619 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -138,10 +138,56 @@ static void amdgpu_unpin_work_func(struct work_struct *__work)
 	kfree(work);
 }
 
-int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
-				 struct drm_framebuffer *fb,
-				 struct drm_pending_vblank_event *event,
-				 uint32_t page_flip_flags, uint32_t target)
+
+static void amdgpu_flip_work_cleanup(struct amdgpu_flip_work *work)
+{
+	int i;
+
+	amdgpu_bo_unref(&work->old_abo);
+	dma_fence_put(work->excl);
+	for (i = 0; i < work->shared_count; ++i)
+		dma_fence_put(work->shared[i]);
+	kfree(work->shared);
+	kfree(work);
+}
+
+static void amdgpu_flip_cleanup_unreserve(
+		struct amdgpu_flip_work *work,
+		struct amdgpu_bo *new_abo)
+{
+	amdgpu_bo_unreserve(new_abo);
+	amdgpu_flip_work_cleanup(work);
+}
+
+static void amdgpu_flip_cleanup_unpin(
+		struct amdgpu_flip_work *work,
+		struct amdgpu_bo *new_abo)
+{
+	if (unlikely(amdgpu_bo_unpin(new_abo) != 0))
+		DRM_ERROR("failed to unpin new abo in error path\n");
+	amdgpu_flip_cleanup_unreserve(work, new_abo);
+}
+
+void amdgpu_crtc_cleanup_flip_ctx(
+		struct amdgpu_flip_work *work,
+		struct amdgpu_bo *new_abo)
+{
+	if (unlikely(amdgpu_bo_reserve(new_abo, false) != 0)) {
+		DRM_ERROR("failed to reserve new abo in error path\n");
+		amdgpu_flip_work_cleanup(work);
+		return;
+	}
+	amdgpu_flip_cleanup_unpin(work, new_abo);
+}
+
+int amdgpu_crtc_prepare_flip(
+				struct drm_crtc *crtc,
+				struct drm_framebuffer *fb,
+				struct drm_pending_vblank_event *event,
+				uint32_t page_flip_flags,
+				uint32_t target,
+				struct amdgpu_flip_work **work_p,
+				struct amdgpu_bo **new_abo_p)
 {
 	struct drm_device *dev = crtc->dev;
 	struct amdgpu_device *adev = dev->dev_private;
@@ -154,7 +200,7 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
 	unsigned long flags;
 	u64 tiling_flags;
 	u64 base;
-	int i, r;
+	int r;
 
 	work = kzalloc(sizeof *work, GFP_KERNEL);
 	if (work == NULL)
@@ -215,41 +261,84 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
 		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
 		r = -EBUSY;
 		goto pflip_cleanup;
-	}
 
-	amdgpu_crtc->pflip_status = AMDGPU_FLIP_PENDING;
-	amdgpu_crtc->pflip_works = work;
+	}
+	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
 
+	*work_p = work;
+	*new_abo_p = new_abo;
 
-	DRM_DEBUG_DRIVER("crtc:%d[%p], pflip_stat:AMDGPU_FLIP_PENDING, work: %p,\n",
-					 amdgpu_crtc->crtc_id, amdgpu_crtc, work);
-	/* update crtc fb */
-	crtc->primary->fb = fb;
-	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
-	amdgpu_flip_work_func(&work->flip_work.work);
 	return 0;
 
 pflip_cleanup:
-	if (unlikely(amdgpu_bo_reserve(new_abo, false) != 0)) {
-		DRM_ERROR("failed to reserve new abo in error path\n");
-		goto cleanup;
-	}
+	amdgpu_crtc_cleanup_flip_ctx(work, new_abo);
+	return r;
+
 unpin:
-	if (unlikely(amdgpu_bo_unpin(new_abo) != 0)) {
-		DRM_ERROR("failed to unpin new abo in error path\n");
-	}
+	amdgpu_flip_cleanup_unpin(work, new_abo);
+	return r;
+
 unreserve:
-	amdgpu_bo_unreserve(new_abo);
+	amdgpu_flip_cleanup_unreserve(work, new_abo);
+	return r;
 
 cleanup:
-	amdgpu_bo_unref(&work->old_abo);
-	dma_fence_put(work->excl);
-	for (i = 0; i < work->shared_count; ++i)
-		dma_fence_put(work->shared[i]);
-	kfree(work->shared);
-	kfree(work);
-
+	amdgpu_flip_work_cleanup(work);
 	return r;
+
+}
+
+void amdgpu_crtc_submit_flip(
+		struct drm_crtc *crtc,
+		struct drm_framebuffer *fb,
+		struct amdgpu_flip_work *work,
+		struct amdgpu_bo *new_abo)
+{
+	unsigned long flags;
+	struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
+
+	spin_lock_irqsave(&crtc->dev->event_lock, flags);
+	amdgpu_crtc->pflip_status = AMDGPU_FLIP_PENDING;
+	amdgpu_crtc->pflip_works = work;
+
+	/* update crtc fb */
+	crtc->primary->fb = fb;
+	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+
+	DRM_DEBUG_DRIVER(
+			"crtc:%d[%p], pflip_stat:AMDGPU_FLIP_PENDING, work: %p,\n",
+			amdgpu_crtc->crtc_id, amdgpu_crtc, work);
+
+	amdgpu_flip_work_func(&work->flip_work.work);
+}
+
+int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
+			  struct drm_framebuffer *fb,
+			  struct drm_pending_vblank_event *event,
+			  uint32_t page_flip_flags,
+			  uint32_t target)
+{
+	struct amdgpu_bo *new_abo;
+	struct amdgpu_flip_work *work;
+	int r = amdgpu_crtc_prepare_flip(
+			crtc,
+			fb,
+			event,
+			page_flip_flags,
+			target,
+			&work,
+			&new_abo);
+
+	if (r)
+		return r;
+
+	amdgpu_crtc_submit_flip(
+			crtc,
+			fb,
+			work,
+			new_abo);
+
+	return 0;
 }
 
 int amdgpu_crtc_set_config(struct drm_mode_set *set)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index b603467..b6d3bc3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -595,6 +595,23 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
 				 struct drm_framebuffer *fb,
 				 struct drm_pending_vblank_event *event,
 				 uint32_t page_flip_flags, uint32_t target);
+void amdgpu_crtc_cleanup_flip_ctx(
+		struct amdgpu_flip_work *work,
+		struct amdgpu_bo *new_abo);
+int amdgpu_crtc_prepare_flip(struct drm_crtc *crtc,
+			  struct drm_framebuffer *fb,
+			  struct drm_pending_vblank_event *event,
+			  uint32_t page_flip_flags,
+			  uint32_t target,
+			  struct amdgpu_flip_work **work,
+			  struct amdgpu_bo **new_abo);
+
+void amdgpu_crtc_submit_flip(
+		struct drm_crtc *crtc,
+		struct drm_framebuffer *fb,
+		struct amdgpu_flip_work *work,
+		struct amdgpu_bo *new_abo);
+
 extern const struct drm_mode_config_funcs amdgpu_mode_funcs;
 
 #endif
-- 
2.5.5

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Refactor flip into prepare submit and submit. (v2)

[Christian König]

Am 13.01.2017 um 19:26 schrieb Alex Deucher:
> From: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
>
> Make pflip atomic friendly. Split the fuinction into
> whatever can fail part and the actual flip submit part.
> Call the pre-submit function before atomic states
> are swapped so in case of error we can fail the
> IOCTL.
>
> v2:
> Update due to  target_vblank code change.
> Fix identetation.
> Change return type for amdgpu_crtc_submit_flip to void
>
> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> Reviewed-by Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

The coding style on a couple of functions headers is incorrect, here for 
example:
> void amdgpu_crtc_cleanup_flip_ctx(
> 		struct amdgpu_flip_work *work,
> 		struct amdgpu_bo *new_abo)

That should look like:
> void amdgpu_crtc_cleanup_flip_ctx(struct amdgpu_flip_work *work,
>                                   struct amdgpu_bo *new_abo)

Regards,
Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: Switch error code when bo pin fails. (v3)

[Christian König]

Am 13.01.2017 um 19:26 schrieb Alex Deucher:
> From: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
>
> Switching to -ENOMEM resolves a hang in case the driver
> uses this function in atomic code path.
>
> v2: Update commit message.
> v3:
> Remove forcing -EINVAL in case of amdgpu_bo_pin fail
> since there is no apparent reason for not just passing
> through the the return value.
>
> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Reviewed-by Harry Wentland <harry.wentland@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>.

>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index 48a3f67..4986340 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -189,7 +189,6 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
>   
>   	r = amdgpu_bo_pin(new_abo, AMDGPU_GEM_DOMAIN_VRAM, &base);
>   	if (unlikely(r != 0)) {
> -		r = -EINVAL;
>   		DRM_ERROR("failed to pin new abo buffer before flip\n");
>   		goto unreserve;
>   	}


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx