diff mbox

[3/4] drm/amd/display: Switch to using atomic_helper for flip.

Message ID 1484581498-32309-4-git-send-email-Andrey.Grodzovsky@amd.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrey Grodzovsky Jan. 16, 2017, 3:44 p.m. UTC
Change-Id: Iad3e0b9b3546e4e4dc79be9233daf4fe4dba83e0
Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
---
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_types.c    | 92 ++--------------------
 1 file changed, 6 insertions(+), 86 deletions(-)

Comments

Laurent Pinchart Jan. 16, 2017, 10:16 p.m. UTC | #1
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@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;
Michel Dänzer Jan. 18, 2017, 1:50 a.m. UTC | #2
On 17/01/17 07:16 AM, Laurent Pinchart wrote:
> On Monday 16 Jan 2017 10:44:57 Andrey Grodzovsky wrote:
>> Change-Id: Iad3e0b9b3546e4e4dc79be9233daf4fe4dba83e0
>> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@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 ?

This patch makes the amdgpu DM code use the page_flip_target hook.
drm_mode_page_flip_ioctl calls drm_crtc_vblank_get before the
page_flip_target hook.

You're right though that the fact that drm_atomic_helper_page_flip
doesn't call drm_crtc_vblank_get is a bit alarming. From the
DRM_IOCTL_MODE_PAGE_FLIP POV, drm_crtc_vblank_get must be called when
userspace calls the ioctl (either by drm_mode_page_flip_ioctl or the
page_flip hook implementation), and drm_crtc_vblank_put must be called
when the flip completes and the event is sent to userspace. How is this
supposed to be handled with atomic?

Andrey, did you check via code audit and/or testing that the vblank
reference count is still balanced after this change?


>> @@ -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.

Yeah, such whitespace-only changes should be dropped.
Laurent Pinchart Jan. 18, 2017, 12:02 p.m. UTC | #3
Hi Michel,

On Wednesday 18 Jan 2017 10:50:01 Michel Dänzer wrote:
> On 17/01/17 07:16 AM, Laurent Pinchart wrote:
> > On Monday 16 Jan 2017 10:44:57 Andrey Grodzovsky wrote:
> >> Change-Id: Iad3e0b9b3546e4e4dc79be9233daf4fe4dba83e0
> >> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@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 ?
> 
> This patch makes the amdgpu DM code use the page_flip_target hook.
> drm_mode_page_flip_ioctl calls drm_crtc_vblank_get before the
> page_flip_target hook.
> 
> You're right though that the fact that drm_atomic_helper_page_flip
> doesn't call drm_crtc_vblank_get is a bit alarming. From the
> DRM_IOCTL_MODE_PAGE_FLIP POV, drm_crtc_vblank_get must be called when
> userspace calls the ioctl (either by drm_mode_page_flip_ioctl or the
> page_flip hook implementation), and drm_crtc_vblank_put must be called
> when the flip completes and the event is sent to userspace. How is this
> supposed to be handled with atomic?

I'll let Daniel comment on that.

> Andrey, did you check via code audit and/or testing that the vblank
> reference count is still balanced after this change?
>
> >> @@ -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.
> 
> Yeah, such whitespace-only changes should be dropped.
Andrey Grodzovsky Jan. 18, 2017, 10:18 p.m. UTC | #4
> -----Original Message-----

> From: Michel Dänzer [mailto:michel@daenzer.net]

> Sent: Tuesday, January 17, 2017 8:50 PM

> To: Laurent Pinchart

> Cc: dri-devel@lists.freedesktop.org; Grodzovsky, Andrey;

> daniel.vetter@intel.com; amd-gfx@lists.freedesktop.org;

> nouveau@lists.freedesktop.org

> Subject: Re: [PATCH 3/4] drm/amd/display: Switch to using atomic_helper for

> flip.

> 

> On 17/01/17 07:16 AM, Laurent Pinchart wrote:

> > On Monday 16 Jan 2017 10:44:57 Andrey Grodzovsky wrote:

> >> Change-Id: Iad3e0b9b3546e4e4dc79be9233daf4fe4dba83e0

> >> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@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 ?

> 

> This patch makes the amdgpu DM code use the page_flip_target hook.

> drm_mode_page_flip_ioctl calls drm_crtc_vblank_get before the

> page_flip_target hook.

> 

> You're right though that the fact that drm_atomic_helper_page_flip doesn't

> call drm_crtc_vblank_get is a bit alarming. From the

> DRM_IOCTL_MODE_PAGE_FLIP POV, drm_crtc_vblank_get must be called

> when userspace calls the ioctl (either by drm_mode_page_flip_ioctl or the

> page_flip hook implementation), and drm_crtc_vblank_put must be called

> when the flip completes and the event is sent to userspace. How is this

> supposed to be handled with atomic?


First let me ask you - why we have to enable vlbank as part of pflip, 
I understand why it has to be enbbled in WAIT_FOR_VBLANK IOCTL 
but not sure about PFLIP IOCTL. 

IMHO in atomic as before in legacy page_flip, it's up to the  driver 
implementation in atomic_commit hook   to manage vblank ISR on,off.
> 

> Andrey, did you check via code audit and/or testing that the vblank

> reference count is still balanced after this change?

> 

With the page_flip_target yes, if I switch back to page_flip hook then 
vblank ISR never gets enabled on FLIP IPCTL and then I see warning in DAL's
pflip done IRQ handler from vblank_put which is obvious, which BTW
didn't impact the flips, I still was able to run glxgears in vsync mode.

> 

> >> @@ -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.

> 

> Yeah, such whitespace-only changes should be dropped.


Will remove this.
> 

> 

> --

> Earthling Michel Dänzer               |               http://www.amd.com

> Libre software enthusiast             |             Mesa and X developer
Michel Dänzer Jan. 19, 2017, 2:14 a.m. UTC | #5
On 19/01/17 07:18 AM, Grodzovsky, Andrey wrote:
>> From: Michel Dänzer [mailto:michel@daenzer.net]
>> On 17/01/17 07:16 AM, Laurent Pinchart wrote:
>>> On Monday 16 Jan 2017 10:44:57 Andrey Grodzovsky wrote:
>>>> Change-Id: Iad3e0b9b3546e4e4dc79be9233daf4fe4dba83e0
>>>> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@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 ?
>>
>> This patch makes the amdgpu DM code use the page_flip_target hook.
>> drm_mode_page_flip_ioctl calls drm_crtc_vblank_get before the
>> page_flip_target hook.
>>
>> You're right though that the fact that drm_atomic_helper_page_flip doesn't
>> call drm_crtc_vblank_get is a bit alarming. From the
>> DRM_IOCTL_MODE_PAGE_FLIP POV, drm_crtc_vblank_get must be called
>> when userspace calls the ioctl (either by drm_mode_page_flip_ioctl or the
>> page_flip hook implementation), and drm_crtc_vblank_put must be called
>> when the flip completes and the event is sent to userspace. How is this
>> supposed to be handled with atomic?
> 
> First let me ask you - why we have to enable vlbank as part of pflip, 
> I understand why it has to be enbbled in WAIT_FOR_VBLANK IOCTL 
> but not sure about PFLIP IOCTL. 

It's required for the vblank sequence counter to be accurate in the page
flip completion event sent to userspace.


> IMHO in atomic as before in legacy page_flip, it's up to the  driver 
> implementation in atomic_commit hook   to manage vblank ISR on,off.

When using the page_flip hook, it's all up to the implementation of that
hook. When using the page_flip_target hook, drm_mode_page_flip_ioctl
calls vblank_get, so the hook implementation must make sure that a
matching vblank_put call is made when the flip completes.


>> Andrey, did you check via code audit and/or testing that the vblank
>> reference count is still balanced after this change?
>>
> With the page_flip_target yes, if I switch back to page_flip hook then 
> vblank ISR never gets enabled on FLIP IPCTL and then I see warning in DAL's
> pflip done IRQ handler from vblank_put which is obvious,

For drivers using the atomic_helpers for flip, maybe there should be (or
might be already) a corresponding helper which deals with things like
this when the flip completes, so drivers don't have to call vblank_put
and such.

Anyway, it sounds like the vblank_get/puts are balanced with this patch
for now.


> which BTW didn't impact the flips, I still was able to run glxgears
> in vsync mode.

I don't think glxgears relies on the vblank sequence numbers being accurate.
diff mbox

Patch

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;