[PATCH 0/4] Allow ASYNC flip with atomic helpers.

[PATCH 0/4] Allow ASYNC flip with atomic helpers.

[Andrey Grodzovsky]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state

[Andrey Grodzovsky]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state

[Gustavo Padovan]

Hi Andrey,

2017-01-16 Andrey Grodzovsky <Andrey.Grodzovsky@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@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

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

RE: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state

[Grodzovsky, Andrey]

> -----Original Message-----
> From: Gustavo Padovan [mailto:gustavo@padovan.org]
> Sent: Monday, January 16, 2017 3:22 PM
> To: Grodzovsky, Andrey
> Cc: dri-devel@lists.freedesktop.org; nouveau@lists.freedesktop.org; amd-
> gfx@lists.freedesktop.org; Deucher, Alexander; daniel.vetter@intel.com
> Subject: Re: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state
> 
> Hi Andrey,
> 
> 2017-01-16 Andrey Grodzovsky <Andrey.Grodzovsky@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@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

Thanks for spotting this, I am afraid I've sent not the final version of the patch. 
I will resend the latest version later today.

Thanks
Andrey

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state

[Daniel Vetter]

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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state

[Cheng, Tony]

> -----Original Message-----
> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf
> Of Daniel Vetter
> Sent: Monday, January 23, 2017 3:55 AM
> To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>;
> nouveau@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org; daniel.vetter@intel.com
> Subject: Re: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state
> 
> 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@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

What does ASYNC flip mean?  HW flip as soon as possible and result in tearing on screen?  If so I could imaging some use case where you have some UI control/menu overlay on top, and some game running on a underlay plane, and the game want to be able to flip as soon as possible.  Or Daniel do you think ASYNC property will apply to all planes in CRTC?

> 
> > ---
> >  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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state

[Grodzovsky, Andrey]

> -----Original Message-----
> From: Cheng, Tony
> Sent: Monday, January 23, 2017 2:49 PM
> To: Daniel Vetter; Grodzovsky, Andrey
> Cc: Deucher, Alexander; nouveau@lists.freedesktop.org; amd-
> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> daniel.vetter@intel.com; dc_upstream
> Subject: RE: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state
> 
> 
> 
> > -----Original Message-----
> > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On
> > Behalf Of Daniel Vetter
> > Sent: Monday, January 23, 2017 3:55 AM
> > To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>;
> > nouveau@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; dri-
> > devel@lists.freedesktop.org; daniel.vetter@intel.com
> > Subject: Re: [PATCH 1/4] drm/atomic: Save flip flags in
> > drm_plane_state
> >
> > 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@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
> 
Do you mean here crtc or crtc_state ?

> What does ASYNC flip mean?  HW flip as soon as possible and result in tearing
> on screen?  If so I could imaging some use case where you have some UI
> control/menu overlay on top, and some game running on a underlay plane,
> and the game want to be able to flip as soon as possible.  Or Daniel do you
> think ASYNC property will apply to all planes in CRTC?
> 
Also the client code both in nouveau and AMD/DAL iterates over new planes and their new states
so it seems to me easier to retrieve the parameter from the plane_state and not adding loop
to find matching crtc or its  state, especially if it's before drm_atomic_helper_swap_state is called,
 when crtc->state still points to the old state ...
[Grodzovsky, Andrey] 
> >
> > > ---
> > >  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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state

[Daniel Vetter]

On Thu, Jan 26, 2017 at 05:01:01AM +0000, Grodzovsky, Andrey wrote:
> 
> 
> > -----Original Message-----
> > From: Cheng, Tony
> > Sent: Monday, January 23, 2017 2:49 PM
> > To: Daniel Vetter; Grodzovsky, Andrey
> > Cc: Deucher, Alexander; nouveau@lists.freedesktop.org; amd-
> > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> > daniel.vetter@intel.com; dc_upstream
> > Subject: RE: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On
> > > Behalf Of Daniel Vetter
> > > Sent: Monday, January 23, 2017 3:55 AM
> > > To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
> > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>;
> > > nouveau@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; dri-
> > > devel@lists.freedesktop.org; daniel.vetter@intel.com
> > > Subject: Re: [PATCH 1/4] drm/atomic: Save flip flags in
> > > drm_plane_state
> > >
> > > 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@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
> > 
> Do you mean here crtc or crtc_state ?

Yes.

> > What does ASYNC flip mean?  HW flip as soon as possible and result in tearing
> > on screen?  If so I could imaging some use case where you have some UI
> > control/menu overlay on top, and some game running on a underlay plane,
> > and the game want to be able to flip as soon as possible.  Or Daniel do you
> > think ASYNC property will apply to all planes in CRTC?
> > 
> Also the client code both in nouveau and AMD/DAL iterates over new planes and their new states
> so it seems to me easier to retrieve the parameter from the plane_state and not adding loop
> to find matching crtc or its  state, especially if it's before drm_atomic_helper_swap_state is called,
>  when crtc->state still points to the old state ...

It's pretty easy to go from plane_state to crtc_state. No need to loop at
all. And the implementation shouldn't justify where we put it, the
important part is the semantics.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state

[Daniel Vetter]

On Mon, Jan 23, 2017 at 07:48:54PM +0000, Cheng, Tony wrote:
> 
> 
> > -----Original Message-----
> > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf
> > Of Daniel Vetter
> > Sent: Monday, January 23, 2017 3:55 AM
> > To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>;
> > nouveau@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; dri-
> > devel@lists.freedesktop.org; daniel.vetter@intel.com
> > Subject: Re: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state
> > 
> > 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@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
> 
> What does ASYNC flip mean?  HW flip as soon as possible and result in
> tearing on screen?  If so I could imaging some use case where you have
> some UI control/menu overlay on top, and some game running on a underlay
> plane, and the game want to be able to flip as soon as possible.  Or
> Daniel do you think ASYNC property will apply to all planes in CRTC?

Those kind of questions are exactly why I think we should wait with
exposing async through the atomic ioctl until someone needs it. And yes
async means "as fast as possible, with tearing".
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state

[Grodzovsky, Andrey]

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: Monday, January 16, 2017 5:18 PM
> To: dri-devel@lists.freedesktop.org
> Cc: Grodzovsky, Andrey; nouveau@lists.freedesktop.org; amd-
> gfx@lists.freedesktop.org; Deucher, Alexander; daniel.vetter@intel.com
> Subject: Re: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state
> 
> 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@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

Please check drm_mode_page_flip_ioctl, one of the checks in the beginning is

if ((page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC) && !dev->mode_config.async_page_flip)
		return -EINVAL;

We in DC explicitly set dev->mode_config.async_page_flip = true, any driver which is 
Not supporting ASYNC flip will fail the IOCTL at this point.
Same in drm_mode_atomic_ioctl
> 
>  * 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).

Thanks, that a good catch, will remove.

Andrey
> 
> >  	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

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

Re: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state

[Laurent Pinchart]

Hi Andrey,

On Tuesday 17 Jan 2017 04:03:11 Grodzovsky, Andrey wrote:
> On Monday, January 16, 2017 5:18 PM Laurent Pinchart wrote:
> > 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@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
> 
> Please check drm_mode_page_flip_ioctl, one of the checks in the beginning is
> 
> if ((page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC) &&
> !dev->mode_config.async_page_flip) return -EINVAL;

I think you're right. Sorry for the noise.

> We in DC explicitly set dev->mode_config.async_page_flip = true, any driver
> which is Not supporting ASYNC flip will fail the IOCTL at this point.
> Same in drm_mode_atomic_ioctl
> 
> >  * 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).
> 
> Thanks, that a good catch, will remove.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state

[Ville Syrjälä]

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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/amdgpu: Remove flip_flag from amdgpu_crtc

[Andrey Grodzovsky]

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

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

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

Re: [PATCH 2/4] drm/amdgpu: Remove flip_flag from amdgpu_crtc

[Laurent Pinchart]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andrey Grodzovsky]

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);
-	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

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

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

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Grodzovsky, Andrey]

> -----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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH v3 2/3] drm/amd/display: Switch to using atomic_helper for flip.

[Andrey Grodzovsky]

v3: Remove white space change.

Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h           |  1 -
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_types.c    | 89 ++--------------------
 2 files changed, 5 insertions(+), 85 deletions(-)

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;
 };
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..70e01ad 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;
 		}
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/nouveau/kms/nv50: Switch to using atomic helper for flip.

[Andrey Grodzovsky]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/4] Allow ASYNC flip with atomic helpers.

[Laurent Pinchart]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/4] Allow ASYNC flip with atomic helpers.

[Harry Wentland]

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(-)
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 0/4] Allow ASYNC flip with atomic helpers.

[Laurent Pinchart]

Hi Harry,

On Monday 16 Jan 2017 16:13:39 Harry Wentland wrote:
> On 2017-01-16 03:39 PM, Laurent Pinchart wrote:
> > 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?

If this patch series was just about moving common driver code into the core, 
sure, but it goes beyond that. Or, actually, it needs to go beyond that, but 
doesn't yet. Removing the DRM_MODE_PAGE_FLIP_ASYNC test in patch 1/4 means 
that the DRM core will not reject async page flips anymore, for any driver 
that uses the helper. You thus need to either patch all drivers that use the 
helper to reject the flag, or implement the feature in the drivers (and 
preferably in the helpers then). The current version of this patch series will 
make all existing users of the helpers accept async page flips without 
actually implementing them.

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/4] Allow ASYNC flip with atomic helpers.

[Daniel Vetter]

On Tue, Jan 17, 2017 at 12:14:24AM +0200, Laurent Pinchart wrote:
> Hi Harry,
> 
> On Monday 16 Jan 2017 16:13:39 Harry Wentland wrote:
> > On 2017-01-16 03:39 PM, Laurent Pinchart wrote:
> > > 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?
> 
> If this patch series was just about moving common driver code into the core, 
> sure, but it goes beyond that. Or, actually, it needs to go beyond that, but 
> doesn't yet. Removing the DRM_MODE_PAGE_FLIP_ASYNC test in patch 1/4 means 
> that the DRM core will not reject async page flips anymore, for any driver 
> that uses the helper. You thus need to either patch all drivers that use the 
> helper to reject the flag, or implement the feature in the drivers (and 
> preferably in the helpers then). The current version of this patch series will 
> make all existing users of the helpers accept async page flips without 
> actually implementing them.

"How do we check for this driver feature" was one of the opens I raised
here on irc, even before Andrey started typing :-) Definitely needs to be
addressed. What's imo optional is exposing async flips through the atomic
ioctl, since atm no atomic userspace seems to be in need of async flips
yet. And the semantics of multi-plane async flips are tricky to define
properly, so definitely want that userspace validation. That's why I think
just doing the compat stuff is ok.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx