Nouveau - Jan 2017 - [PATCH 3/4] drm/amd/display: Switch to using atomic_helper for flip.

This series is a folow-up on 
https://patchwork.kernel.org/patch/9501787/

The first patch makes changes to atomic helpers
to allow for drives with ASYNC flip support to use them.
Patches 2 and 3 are to use this in AMDGPU/DC and
patch 4 is possible cleanup in nouveau/kms who seems
to have the duplicate the helper as we did to support
ASYNC flips. 

Andrey Grodzovsky (4):
  drm/atomic: Save flip flags in drm_plane_state
  drm/amdgpu: Remove flip_flag from amdgpu_crtc
  drm/amd/display: Switch to using atomic_helper for flip.
  drm/nouveau/kms/nv50: Switch to using atomic helper for flip.

 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h           |  1 -
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_types.c    | 92 ++--------------------
 drivers/gpu/drm/drm_atomic_helper.c                | 10 +--
 drivers/gpu/drm/nouveau/nv50_display.c             | 77 ++----------------
 include/drm/drm_plane.h                            |  8 ++
 5 files changed, 22 insertions(+), 166 deletions(-)

-- 
1.9.1

Allows using atomic flip helpers for drivers
using ASYNC flip.
Remove ASYNC_FLIP restriction in helpers and
caches the page flip flags in drm_plane_state
to be used in the low level drivers.

Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 10 +++-------
 include/drm/drm_plane.h             |  8 ++++++++
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index a4e5477..f83dc43 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2737,7 +2737,8 @@ static int page_flip_common(
 				struct drm_atomic_state *state,
 				struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
-				struct drm_pending_vblank_event *event)
+				struct drm_pending_vblank_event *event,
+				uint32_t flags)
 {
 	struct drm_plane *plane = crtc->primary;
 	struct drm_plane_state *plane_state;
@@ -2754,6 +2755,7 @@ static int page_flip_common(
 	if (IS_ERR(plane_state))
 		return PTR_ERR(plane_state);
 
+	plane_state->pflip_flags = flags;
 
 	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
 	if (ret != 0)
@@ -2800,9 +2802,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 	struct drm_atomic_state *state;
 	int ret = 0;
 
-	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
-		return -EINVAL;
-
 	state = drm_atomic_state_alloc(plane->dev);
 	if (!state)
 		return -ENOMEM;
@@ -2865,9 +2864,6 @@ int drm_atomic_helper_page_flip_target(
 	struct drm_crtc_state *crtc_state;
 	int ret = 0;
 
-	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
-		return -EINVAL;
-
 	state = drm_atomic_state_alloc(plane->dev);
 	if (!state)
 		return -ENOMEM;
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index db3bbde..86d8ffc 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -122,6 +122,14 @@ struct drm_plane_state {
 	 */
 	bool visible;
 
+
+	/**
+	 * @pflip_flags:
+	 *
+	 * Flip related config options
+	 */
+	u32 pflip_flags;
+
 	struct drm_atomic_state *state;
 };
 
-- 
1.9.1

Follwing introduction of pflip_flags in drm_plane_state
this is not needed anymore.

Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index 4c0a86e..3ff3c14 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -443,7 +443,6 @@ struct amdgpu_crtc {
 	enum amdgpu_interrupt_state vsync_timer_enabled;
 
 	int otg_inst;
-	uint32_t flip_flags;
 	/* After Set Mode target will be non-NULL */
 	struct dc_target *target;
 };
-- 
1.9.1

Change-Id: Iad3e0b9b3546e4e4dc79be9233daf4fe4dba83e0
Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
---
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_types.c    | 92 ++--------------------
 1 file changed, 6 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
index a443b70..d4664bf 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
@@ -1060,83 +1060,6 @@ static int dm_crtc_funcs_atomic_set_property(
 	return 0;
 }
 
-
-static int amdgpu_atomic_helper_page_flip(struct drm_crtc *crtc,
-				struct drm_framebuffer *fb,
-				struct drm_pending_vblank_event *event,
-				uint32_t flags)
-{
-	struct drm_plane *plane = crtc->primary;
-	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
-	struct drm_atomic_state *state;
-	struct drm_plane_state *plane_state;
-	struct drm_crtc_state *crtc_state;
-	int ret = 0;
-
-	state = drm_atomic_state_alloc(plane->dev);
-	if (!state)
-		return -ENOMEM;
-
-	ret = drm_crtc_vblank_get(crtc);
-	if (ret)
-		return ret;
-
-	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
-retry:
-	crtc_state = drm_atomic_get_crtc_state(state, crtc);
-	if (IS_ERR(crtc_state)) {
-		ret = PTR_ERR(crtc_state);
-		goto fail;
-	}
-	crtc_state->event = event;
-
-	plane_state = drm_atomic_get_plane_state(state, plane);
-	if (IS_ERR(plane_state)) {
-		ret = PTR_ERR(plane_state);
-		goto fail;
-	}
-
-	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
-	if (ret != 0)
-		goto fail;
-	drm_atomic_set_fb_for_plane(plane_state, fb);
-
-	/* Make sure we don't accidentally do a full modeset. */
-	state->allow_modeset = false;
-	if (!crtc_state->active) {
-		DRM_DEBUG_ATOMIC("[CRTC:%d] disabled, rejecting legacy flip\n",
-				 crtc->base.id);
-		ret = -EINVAL;
-		goto fail;
-	}
-	acrtc->flip_flags = flags;
-
-	ret = drm_atomic_nonblocking_commit(state);
-
-fail:
-	if (ret == -EDEADLK)
-		goto backoff;
-
-	if (ret)
-		drm_crtc_vblank_put(crtc);
-
-	drm_atomic_state_put(state);
-
-	return ret;
-backoff:
-	drm_atomic_state_clear(state);
-	drm_atomic_legacy_backoff(state);
-
-	/*
-	 * Someone might have exchanged the framebuffer while we dropped locks
-	 * in the backoff code. We need to fix up the fb refcount tracking the
-	 * core does for us.
-	 */
-	plane->old_fb = plane->fb;
-
-	goto retry;
-}
-
 /* Implemented only the options currently availible for the driver */
 static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
 	.reset = drm_atomic_helper_crtc_reset,
@@ -1145,7 +1068,7 @@ static int amdgpu_atomic_helper_page_flip(struct drm_crtc
*crtc,
 	.destroy = amdgpu_dm_crtc_destroy,
 	.gamma_set = amdgpu_dm_atomic_crtc_gamma_set,
 	.set_config = drm_atomic_helper_set_config,
-	.page_flip = amdgpu_atomic_helper_page_flip,
+	.page_flip_target = drm_atomic_helper_page_flip_target,
 	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 	.atomic_set_property = dm_crtc_funcs_atomic_set_property
@@ -1679,7 +1602,7 @@ static bool page_flip_needed(
 				    sizeof(old_state_tmp)) == 0 ? true:false;
 	if (new_state->crtc && page_flip_required == false) {
 		acrtc_new = to_amdgpu_crtc(new_state->crtc);
-		if (acrtc_new->flip_flags & DRM_MODE_PAGE_FLIP_ASYNC)
+		if (new_state->pflip_flags & DRM_MODE_PAGE_FLIP_ASYNC)
 			page_flip_required = true;
 	}
 	return page_flip_required;
@@ -2760,7 +2683,6 @@ int amdgpu_dm_atomic_commit(
 	for_each_plane_in_state(state, plane, old_plane_state, i) {
 		struct drm_plane_state *plane_state = plane->state;
 		struct drm_crtc *crtc = plane_state->crtc;
-		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 		struct drm_framebuffer *fb = plane_state->fb;
 
 		if (!fb || !crtc || !crtc->state->planes_changed ||
@@ -2771,10 +2693,9 @@ int amdgpu_dm_atomic_commit(
 			ret = amdgpu_crtc_page_flip_target(crtc,
 							   fb,
 							   crtc->state->event,
-							   acrtc->flip_flags,
-							   drm_crtc_vblank_count(crtc));
-			/*clean up the flags for next usage*/
-			acrtc->flip_flags = 0;
+							   plane_state->pflip_flags,
+							   crtc->state->target_vblank);
+
 			if (ret)
 				break;
 		}
@@ -3143,8 +3064,7 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
 			 * 1. This commit is not a page flip.
 			 * 2. This commit is a page flip, and targets are created.
 			 */
-			if (!page_flip_needed(plane_state, old_plane_state,
-					      true) ||
+			if (!page_flip_needed(plane_state, old_plane_state, true) ||
 					action == DM_COMMIT_ACTION_DPMS_ON ||
 					action == DM_COMMIT_ACTION_SET) {
 				struct dc_surface *surface;
-- 
1.9.1

Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
---
 drivers/gpu/drm/nouveau/nv50_display.c | 77 +++-------------------------------
 1 file changed, 5 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nv50_display.c
b/drivers/gpu/drm/nouveau/nv50_display.c
index 2c2c645..419e00c 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -846,6 +846,10 @@ struct nv50_wndw_func {
 	asyw->image.w = fb->base.width;
 	asyw->image.h = fb->base.height;
 	asyw->image.kind = (fb->nvbo->tile_flags & 0x0000ff00) >>
8;
+
+	asyw->interval +			asyw->state.pflip_flags &
DRM_MODE_PAGE_FLIP_ASYNC ? 0 : 1;
+
 	if (asyw->image.kind) {
 		asyw->image.layout = 0;
 		if (drm->device.info.chipset >= 0xc0)
@@ -2221,77 +2225,6 @@ struct nv50_base {
 	.atomic_check = nv50_head_atomic_check,
 };
 
-/* This is identical to the version in the atomic helpers, except that
- * it supports non-vblanked ("async") page flips.
- */
-static int
-nv50_head_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
-		    struct drm_pending_vblank_event *event, u32 flags)
-{
-	struct drm_plane *plane = crtc->primary;
-	struct drm_atomic_state *state;
-	struct drm_plane_state *plane_state;
-	struct drm_crtc_state *crtc_state;
-	int ret = 0;
-
-	state = drm_atomic_state_alloc(plane->dev);
-	if (!state)
-		return -ENOMEM;
-
-	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
-retry:
-	crtc_state = drm_atomic_get_crtc_state(state, crtc);
-	if (IS_ERR(crtc_state)) {
-		ret = PTR_ERR(crtc_state);
-		goto fail;
-	}
-	crtc_state->event = event;
-
-	plane_state = drm_atomic_get_plane_state(state, plane);
-	if (IS_ERR(plane_state)) {
-		ret = PTR_ERR(plane_state);
-		goto fail;
-	}
-
-	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
-	if (ret != 0)
-		goto fail;
-	drm_atomic_set_fb_for_plane(plane_state, fb);
-
-	/* Make sure we don't accidentally do a full modeset. */
-	state->allow_modeset = false;
-	if (!crtc_state->active) {
-		DRM_DEBUG_ATOMIC("[CRTC:%d] disabled, rejecting legacy flip\n",
-				 crtc->base.id);
-		ret = -EINVAL;
-		goto fail;
-	}
-
-	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
-		nv50_wndw_atom(plane_state)->interval = 0;
-
-	ret = drm_atomic_nonblocking_commit(state);
-fail:
-	if (ret == -EDEADLK)
-		goto backoff;
-
-	drm_atomic_state_put(state);
-	return ret;
-
-backoff:
-	drm_atomic_state_clear(state);
-	drm_atomic_legacy_backoff(state);
-
-	/*
-	 * Someone might have exchanged the framebuffer while we dropped locks
-	 * in the backoff code. We need to fix up the fb refcount tracking the
-	 * core does for us.
-	 */
-	plane->old_fb = plane->fb;
-
-	goto retry;
-}
-
 static int
 nv50_head_gamma_set(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b,
 		    uint32_t size)
@@ -2386,7 +2319,7 @@ struct nv50_base {
 	.gamma_set = nv50_head_gamma_set,
 	.destroy = nv50_head_destroy,
 	.set_config = drm_atomic_helper_set_config,
-	.page_flip = nv50_head_page_flip,
+	.page_flip = drm_atomic_helper_page_flip,
 	.set_property = drm_atomic_helper_crtc_set_property,
 	.atomic_duplicate_state = nv50_head_atomic_duplicate_state,
 	.atomic_destroy_state = nv50_head_atomic_destroy_state,
-- 
1.9.1

Hi Andrey,

2017-01-16 Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>:
> Allows using atomic flip helpers for drivers
> using ASYNC flip.
> Remove ASYNC_FLIP restriction in helpers and
> caches the page flip flags in drm_plane_state
> to be used in the low level drivers.
> 
> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 10 +++-------
>  include/drm/drm_plane.h             |  8 ++++++++
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
> index a4e5477..f83dc43 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2737,7 +2737,8 @@ static int page_flip_common(
>  				struct drm_atomic_state *state,
>  				struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
> -				struct drm_pending_vblank_event *event)
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags)
Did you build this patch? It is changing the signature of
page_flip_common() but no changes to the callers.

Gustavo

Hi Andrey,

Thank you for the patches.

On Monday 16 Jan 2017 10:44:54 Andrey Grodzovsky wrote:
> This series is a folow-up on
> https://patchwork.kernel.org/patch/9501787/
> 
> The first patch makes changes to atomic helpers
> to allow for drives with ASYNC flip support to use them.
> Patches 2 and 3 are to use this in AMDGPU/DC and
> patch 4 is possible cleanup in nouveau/kms who seems
> to have the duplicate the helper as we did to support
> ASYNC flips.
I have my doubts regarding this. I'd much rather see userspace moving to the
atomic API instead of extending support for legacy APIs.
> Andrey Grodzovsky (4):
>   drm/atomic: Save flip flags in drm_plane_state
>   drm/amdgpu: Remove flip_flag from amdgpu_crtc
>   drm/amd/display: Switch to using atomic_helper for flip.
>   drm/nouveau/kms/nv50: Switch to using atomic helper for flip.
> 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h           |  1 -
>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_types.c    | 92
-------------------
>  drivers/gpu/drm/drm_atomic_helper.c                | 10 +--
>  drivers/gpu/drm/nouveau/nv50_display.c             | 77 ++----------------
>  include/drm/drm_plane.h                            |  8 ++
>  5 files changed, 22 insertions(+), 166 deletions(-)
-- 
Regards,

Laurent Pinchart

On 2017-01-16 03:39 PM, Laurent Pinchart wrote:
> Hi Andrey,
>
> Thank you for the patches.
>
> On Monday 16 Jan 2017 10:44:54 Andrey Grodzovsky wrote:
>> This series is a folow-up on
>> https://patchwork.kernel.org/patch/9501787/
>>
>> The first patch makes changes to atomic helpers
>> to allow for drives with ASYNC flip support to use them.
>> Patches 2 and 3 are to use this in AMDGPU/DC and
>> patch 4 is possible cleanup in nouveau/kms who seems
>> to have the duplicate the helper as we did to support
>> ASYNC flips.
>
> I have my doubts regarding this. I'd much rather see userspace moving
to the
> atomic API instead of extending support for legacy APIs.
>
This change is not about introducing the async flag but cleaning up the 
legacy helpers to make sure drivers that currently use it through the 
legacy IOCTLs can benefit from the helpers and not have to roll their own.

If the problem is with the pflip_flags, wouldn't drivers still need that 
after moving userspace to the atomic IOCTL?

I don't disagree with you on having userspace move to atomic but I don't
expect to see all userspace drivers move to atomic in the next couple 
months. Why not clean this up in the meantime?

Harry
>> Andrey Grodzovsky (4):
>>   drm/atomic: Save flip flags in drm_plane_state
>>   drm/amdgpu: Remove flip_flag from amdgpu_crtc
>>   drm/amd/display: Switch to using atomic_helper for flip.
>>   drm/nouveau/kms/nv50: Switch to using atomic helper for flip.
>>
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h           |  1 -
>>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_types.c    | 92
-------------------
>>  drivers/gpu/drm/drm_atomic_helper.c                | 10 +--
>>  drivers/gpu/drm/nouveau/nv50_display.c             | 77
++----------------
>>  include/drm/drm_plane.h                            |  8 ++
>>  5 files changed, 22 insertions(+), 166 deletions(-)
>

Hi Andrey,

Thank you for the patch.

On Monday 16 Jan 2017 10:44:56 Andrey Grodzovsky wrote:
> Follwing introduction of pflip_flags in drm_plane_state
> this is not needed anymore.
> 
> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h index 4c0a86e..3ff3c14 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -443,7 +443,6 @@ struct amdgpu_crtc {
>  	enum amdgpu_interrupt_state vsync_timer_enabled;
> 
>  	int otg_inst;
> -	uint32_t flip_flags;
This breaks compilation of the amdgpu driver. You should squash this patch 
with 3/4.
>  	/* After Set Mode target will be non-NULL */
>  	struct dc_target *target;
>  };
-- 
Regards,

Laurent Pinchart

Hi Andrey,

Thank you for the patch.

On Monday 16 Jan 2017 10:44:57 Andrey Grodzovsky wrote:
> Change-Id: Iad3e0b9b3546e4e4dc79be9233daf4fe4dba83e0
> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
> ---
>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_types.c    | 92 ++----------------
>  1 file changed, 6 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c index
> a443b70..d4664bf 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
> @@ -1060,83 +1060,6 @@ static int dm_crtc_funcs_atomic_set_property(
>  	return 0;
>  }
> 
> -
> -static int amdgpu_atomic_helper_page_flip(struct drm_crtc *crtc,
> -				struct drm_framebuffer *fb,
> -				struct drm_pending_vblank_event *event,
> -				uint32_t flags)
> -{
> -	struct drm_plane *plane = crtc->primary;
> -	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> -	struct drm_atomic_state *state;
> -	struct drm_plane_state *plane_state;
> -	struct drm_crtc_state *crtc_state;
> -	int ret = 0;
> -
> -	state = drm_atomic_state_alloc(plane->dev);
> -	if (!state)
> -		return -ENOMEM;
> -
> -	ret = drm_crtc_vblank_get(crtc);
The DRM core's atomic page flip helper doesn't get/put vblank. Have you 
double-checked that removing them isn't a problem ?
> -	if (ret)
> -		return ret;
> -
> -	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
> -retry:
> -	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> -	if (IS_ERR(crtc_state)) {
> -		ret = PTR_ERR(crtc_state);
> -		goto fail;
> -	}
> -	crtc_state->event = event;
> -
> -	plane_state = drm_atomic_get_plane_state(state, plane);
> -	if (IS_ERR(plane_state)) {
> -		ret = PTR_ERR(plane_state);
> -		goto fail;
> -	}
> -
> -	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> -	if (ret != 0)
> -		goto fail;
> -	drm_atomic_set_fb_for_plane(plane_state, fb);
> -
> -	/* Make sure we don't accidentally do a full modeset. */
> -	state->allow_modeset = false;
> -	if (!crtc_state->active) {
> -		DRM_DEBUG_ATOMIC("[CRTC:%d] disabled, rejecting legacy 
flip\n",
> -				 crtc->base.id);
> -		ret = -EINVAL;
> -		goto fail;
> -	}
> -	acrtc->flip_flags = flags;
> -
> -	ret = drm_atomic_nonblocking_commit(state);
> -
> -fail:
> -	if (ret == -EDEADLK)
> -		goto backoff;
> -
> -	if (ret)
> -		drm_crtc_vblank_put(crtc);
> -
> -	drm_atomic_state_put(state);
> -
> -	return ret;
> -backoff:
> -	drm_atomic_state_clear(state);
> -	drm_atomic_legacy_backoff(state);
> -
> -	/*
> -	 * Someone might have exchanged the framebuffer while we dropped locks
> -	 * in the backoff code. We need to fix up the fb refcount tracking the
> -	 * core does for us.
> -	 */
> -	plane->old_fb = plane->fb;
> -
> -	goto retry;
> -}
> -
>  /* Implemented only the options currently availible for the driver */
>  static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
>  	.reset = drm_atomic_helper_crtc_reset,
> @@ -1145,7 +1068,7 @@ static int amdgpu_atomic_helper_page_flip(struct
> drm_crtc *crtc, .destroy = amdgpu_dm_crtc_destroy,
>  	.gamma_set = amdgpu_dm_atomic_crtc_gamma_set,
>  	.set_config = drm_atomic_helper_set_config,
> -	.page_flip = amdgpu_atomic_helper_page_flip,
> +	.page_flip_target = drm_atomic_helper_page_flip_target,
>  	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>  	.atomic_set_property = dm_crtc_funcs_atomic_set_property
> @@ -1679,7 +1602,7 @@ static bool page_flip_needed(
>  				    sizeof(old_state_tmp)) == 0 ? true:false;
>  	if (new_state->crtc && page_flip_required == false) {
>  		acrtc_new = to_amdgpu_crtc(new_state->crtc);
> -		if (acrtc_new->flip_flags & DRM_MODE_PAGE_FLIP_ASYNC)
> +		if (new_state->pflip_flags & DRM_MODE_PAGE_FLIP_ASYNC)
>  			page_flip_required = true;
>  	}
>  	return page_flip_required;
> @@ -2760,7 +2683,6 @@ int amdgpu_dm_atomic_commit(
>  	for_each_plane_in_state(state, plane, old_plane_state, i) {
>  		struct drm_plane_state *plane_state = plane->state;
>  		struct drm_crtc *crtc = plane_state->crtc;
> -		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>  		struct drm_framebuffer *fb = plane_state->fb;
> 
>  		if (!fb || !crtc || !crtc->state->planes_changed ||
> @@ -2771,10 +2693,9 @@ int amdgpu_dm_atomic_commit(
>  			ret = amdgpu_crtc_page_flip_target(crtc,
>  							   fb,
>  							   crtc->state->event,
> -							   acrtc->flip_flags,
> -							   
drm_crtc_vblank_count(crtc));
> -			/*clean up the flags for next usage*/
> -			acrtc->flip_flags = 0;
> +							   plane_state-
>pflip_flags,
> +							   crtc->state-
>target_vblank);
> +
>  			if (ret)
>  				break;
>  		}
> @@ -3143,8 +3064,7 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
>  			 * 1. This commit is not a page flip.
>  			 * 2. This commit is a page flip, and targets are 
created.
>  			 */
> -			if (!page_flip_needed(plane_state, old_plane_state,
> -					      true) ||
> +			if (!page_flip_needed(plane_state, old_plane_state, 
true) ||

This seems to be an unrelated change.
>  					action == DM_COMMIT_ACTION_DPMS_ON ||
>  					action == DM_COMMIT_ACTION_SET) {
>  				struct dc_surface *surface;
-- 
Regards,

Laurent Pinchart

Hi Andrey,

Thank you for the patch.

On Monday 16 Jan 2017 10:44:55 Andrey Grodzovsky wrote:
> Allows using atomic flip helpers for drivers
> using ASYNC flip.
> Remove ASYNC_FLIP restriction in helpers and
> caches the page flip flags in drm_plane_state
> to be used in the low level drivers.
> 
> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 10 +++-------
>  include/drm/drm_plane.h             |  8 ++++++++
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index a4e5477..f83dc43 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2737,7 +2737,8 @@ static int page_flip_common(
>  				struct drm_atomic_state *state,
>  				struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
> -				struct drm_pending_vblank_event *event)
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags)
>  {
>  	struct drm_plane *plane = crtc->primary;
>  	struct drm_plane_state *plane_state;
> @@ -2754,6 +2755,7 @@ static int page_flip_common(
>  	if (IS_ERR(plane_state))
>  		return PTR_ERR(plane_state);
> 
> +	plane_state->pflip_flags = flags;
> 
>  	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
>  	if (ret != 0)
> @@ -2800,9 +2802,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc
*crtc,
> struct drm_atomic_state *state;
>  	int ret = 0;
> 
> -	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> -		return -EINVAL;
> -
With this change all drivers using the helper will not reject that async flag, 
even if they don't implement support for async page flip. You need to either
patch them all to reject the flag, or implement async page flip support for 
all of them (preferable in the helpers, and then remove the

 * Note that for now so called async page flips (i.e. updates which are not
 * synchronized to vblank) are not supported, since the atomic interfaces have
 * no provisions for this yet.

comment).
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> @@ -2865,9 +2864,6 @@ int drm_atomic_helper_page_flip_target(
>  	struct drm_crtc_state *crtc_state;
>  	int ret = 0;
> 
> -	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> -		return -EINVAL;
> -
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index db3bbde..86d8ffc 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -122,6 +122,14 @@ struct drm_plane_state {
>  	 */
>  	bool visible;
> 
> +
> +	/**
> +	 * @pflip_flags:
> +	 *
> +	 * Flip related config options
> +	 */
> +	u32 pflip_flags;
> +
>  	struct drm_atomic_state *state;
>  };
-- 
Regards,

Laurent Pinchart

On Mon, Jan 16, 2017 at 10:44:55AM -0500, Andrey Grodzovsky
wrote:
> Allows using atomic flip helpers for drivers
> using ASYNC flip.
> Remove ASYNC_FLIP restriction in helpers and
> caches the page flip flags in drm_plane_state
> to be used in the low level drivers.
> 
> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 10 +++-------
>  include/drm/drm_plane.h             |  8 ++++++++
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
> index a4e5477..f83dc43 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2737,7 +2737,8 @@ static int page_flip_common(
>  				struct drm_atomic_state *state,
>  				struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
> -				struct drm_pending_vblank_event *event)
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags)
>  {
>  	struct drm_plane *plane = crtc->primary;
>  	struct drm_plane_state *plane_state;
> @@ -2754,6 +2755,7 @@ static int page_flip_common(
>  	if (IS_ERR(plane_state))
>  		return PTR_ERR(plane_state);
>  
> +	plane_state->pflip_flags = flags;
"pflip" looks off. Better just spell it out.

These flags need to be reset in duplicate_state otherwise they leak into
subsequent operations. This is why I don't really like the concept of
"state" containing flags and stuff that are not really part of the
state but rather part of the operation of moving from the old state to
the new state. But maybe we don't have a better place for this sort of
stuff? I have suggested at some point that we might rename drm_atomic_state
to drm_atomic_transaction or something. Stuffing the flags (or just a bool
perhaps?) in there might be less confusing.
>  
>  	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
>  	if (ret != 0)
> @@ -2800,9 +2802,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc
*crtc,
>  	struct drm_atomic_state *state;
>  	int ret = 0;
>  
> -	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> -		return -EINVAL;
> -
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> @@ -2865,9 +2864,6 @@ int drm_atomic_helper_page_flip_target(
>  	struct drm_crtc_state *crtc_state;
>  	int ret = 0;
>  
> -	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> -		return -EINVAL;
> -
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index db3bbde..86d8ffc 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -122,6 +122,14 @@ struct drm_plane_state {
>  	 */
>  	bool visible;
>  
> +
> +	/**
> +	 * @pflip_flags:
> +	 *
> +	 * Flip related config options
> +	 */
> +	u32 pflip_flags;
> +
>  	struct drm_atomic_state *state;
>  };
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- 
Ville Syrjälä
Intel OTC

On Mon, Jan 16, 2017 at 10:44:55AM -0500, Andrey Grodzovsky
wrote:
> Allows using atomic flip helpers for drivers
> using ASYNC flip.
> Remove ASYNC_FLIP restriction in helpers and
> caches the page flip flags in drm_plane_state
> to be used in the low level drivers.
> 
> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
It's mostly guesswork, but I think we should have the flip flags in the
crtc, not in each plane. Similar to how we move the event from planes to
crtc.
-Daniel
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 10 +++-------
>  include/drm/drm_plane.h             |  8 ++++++++
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
> index a4e5477..f83dc43 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2737,7 +2737,8 @@ static int page_flip_common(
>  				struct drm_atomic_state *state,
>  				struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
> -				struct drm_pending_vblank_event *event)
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags)
>  {
>  	struct drm_plane *plane = crtc->primary;
>  	struct drm_plane_state *plane_state;
> @@ -2754,6 +2755,7 @@ static int page_flip_common(
>  	if (IS_ERR(plane_state))
>  		return PTR_ERR(plane_state);
>  
> +	plane_state->pflip_flags = flags;
>  
>  	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
>  	if (ret != 0)
> @@ -2800,9 +2802,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc
*crtc,
>  	struct drm_atomic_state *state;
>  	int ret = 0;
>  
> -	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> -		return -EINVAL;
> -
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> @@ -2865,9 +2864,6 @@ int drm_atomic_helper_page_flip_target(
>  	struct drm_crtc_state *crtc_state;
>  	int ret = 0;
>  
> -	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> -		return -EINVAL;
> -
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index db3bbde..86d8ffc 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -122,6 +122,14 @@ struct drm_plane_state {
>  	 */
>  	bool visible;
>  
> +
> +	/**
> +	 * @pflip_flags:
> +	 *
> +	 * Flip related config options
> +	 */
> +	u32 pflip_flags;
> +
>  	struct drm_atomic_state *state;
>  };
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch