[PATCH 0/2] drm: Add target_vblank field to drm_crtc_state

[PATCH 0/2] drm: Add target_vblank field to drm_crtc_state

[Andrey Grodzovsky]

Following the introduction of target vblank flip in
c229bfbbd drm: Add page_flip_target CRTC hook v2 the target vblank
An extra step is required to support this feature in atomic drivers.
The value has to be passed down to atomic commit to do the page flip.

Patch 1 uses drm_crtc_state as the place to hold this field.
Patch 2 is the intended usage in AMDGPU/DAL driver.

Andrey Grodzovsky (2):
  drm/core: Add target_vblank member to drm_crtc_state
  drm/amd/dal: Switch to page_flip_target hook in DAL.

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c | 8 +++++---
 include/drm/drm_crtc.h                                  | 3 +++
 2 files changed, 8 insertions(+), 3 deletions(-)

-- 
1.9.1

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

[PATCH 1/2] drm: Add target_vblank member to drm_crtc_state

[Andrey Grodzovsky]

This change allows usage of the new page_flip_target hook for
drivers implementing the atomic path.

Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
---
 include/drm/drm_crtc.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 946672f..3a7685f 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -156,6 +156,9 @@ struct drm_crtc_state {
 	struct drm_property_blob *ctm;
 	struct drm_property_blob *gamma_lut;
 
+	/* target vblank count to do a flip */
+	u32 target_vblank;
+
 	/**
 	 * @event:
 	 *
-- 
1.9.1

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

[PATCH 2/2] drm/amd/dal: Switch to page_flip_target hook in DAL.

[Andrey Grodzovsky]

This change shows the usage of proposed target_vblank field in 
drivers using the atomic code path.

Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 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 b44cd1a..eba3caa 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
@@ -1057,7 +1057,8 @@ static int dm_crtc_funcs_atomic_set_property(
 static int amdgpu_atomic_helper_page_flip(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
 				struct drm_pending_vblank_event *event,
-				uint32_t flags)
+				uint32_t flags,
+				uint32_t target_vblank)
 {
 	struct drm_plane *plane = crtc->primary;
 	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
@@ -1078,6 +1079,7 @@ static int amdgpu_atomic_helper_page_flip(struct drm_crtc *crtc,
 		goto fail;
 	}
 	crtc_state->event = event;
+	crtc_state->target_vblank = target_vblank;
 
 	plane_state = drm_atomic_get_plane_state(state, plane);
 	if (IS_ERR(plane_state)) {
@@ -1130,7 +1132,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 = amdgpu_atomic_helper_page_flip,
 	.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
@@ -2757,7 +2759,7 @@ int amdgpu_dm_atomic_commit(
 							   fb,
 							   crtc->state->event,
 							   acrtc->flip_flags,
-							   drm_crtc_vblank_count(crtc));
+							   crtc->state->target_vblank);
 			/*clean up the flags for next usage*/
 			acrtc->flip_flags = 0;
 			if (ret)
-- 
1.9.1

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

Re: [PATCH 1/2] drm: Add target_vblank member to drm_crtc_state

[Daniel Vetter]

On Sun, Jan 1, 2017 at 4:59 PM, Andrey Grodzovsky
<Andrey.Grodzovsky@amd.com> wrote:
> This change allows usage of the new page_flip_target hook for
> drivers implementing the atomic path.
>
> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Instead of patching your amdgpu version of the page_flip helper you
should patch at least the one in drm_atomic_helper..c, and then use
that one. We probably need a page_flip_target version of that, which
then opens some questions about how you would expose this for atomic
drivers and the atomic ioctl. Just adding this here in the shared
struct is somewhat pointless, you could do that in the
amdgpu_crtc_state struct too.

> ---
>  include/drm/drm_crtc.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 946672f..3a7685f 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -156,6 +156,9 @@ struct drm_crtc_state {
>         struct drm_property_blob *ctm;
>         struct drm_property_blob *gamma_lut;
>
> +       /* target vblank count to do a flip */

Please type real kerneldoc for this.

Thanks, Daniel

> +       u32 target_vblank;
> +
>         /**
>          * @event:
>          *
> --
> 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
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/atomic: Add target_vblank support in atomic helpers.

[Andrey Grodzovsky]

Allows usage of the new page_flip_target hook for
drivers implementing the atomic path.
Provides default atomic helper for the new hook.

Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 146 ++++++++++++++++++++++++++++--------
 include/drm/drm_atomic_helper.h     |   6 ++
 include/drm/drm_crtc.h              |   3 +
 3 files changed, 125 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 583f47f..5e76f90 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2733,6 +2733,59 @@ int drm_atomic_helper_resume(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
 
+static int drm_atomic_helper_page_flip_create_state(
+				struct drm_atomic_state *state,
+				struct drm_crtc *crtc,
+				struct drm_framebuffer *fb,
+				struct drm_pending_vblank_event *event)
+{
+	struct drm_plane *plane = crtc->primary;
+	struct drm_plane_state *plane_state;
+	struct drm_crtc_state *crtc_state;
+	int ret = 0;
+
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
+
+	crtc_state->event = event;
+
+	plane_state = drm_atomic_get_plane_state(state, plane);
+	if (IS_ERR(plane_state))
+		return PTR_ERR(plane_state);
+
+
+	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
+	if (ret != 0)
+		return ret;
+	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);
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static void drm_atomic_helper_page_flip_prepare_retry(
+				struct drm_atomic_state *state,
+				struct drm_plane *plane)
+{
+	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;
+}
+
 /**
  * drm_atomic_helper_page_flip - execute a legacy page flip
  * @crtc: DRM crtc
@@ -2756,8 +2809,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 {
 	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;
 
 	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
@@ -2768,35 +2819,79 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 		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);
+	ret = drm_atomic_helper_page_flip_create_state(state, crtc, fb, event);
+	if (ret != 0)
 		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_nonblocking_commit(state);
 
-	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
+fail:
+	if (ret == -EDEADLK)
+		goto backoff;
+
+	drm_atomic_state_put(state);
+	return ret;
+
+backoff:
+	drm_atomic_helper_page_flip_prepare_retry(state, plane);
+	goto retry;
+}
+EXPORT_SYMBOL(drm_atomic_helper_page_flip);
+
+/**
+ * drm_atomic_helper_page_flip - execute a legacy page flip
+ * @crtc: DRM crtc
+ * @fb: DRM framebuffer
+ * @event: optional DRM event to signal upon completion
+ * @flags: flip flags for non-vblank sync'ed updates
+ *
+ * Provides a default page flip on specific vblank implementation using
+ * the atomic driver interface.
+ *
+ * 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.
+ *
+ * Returns:
+ * Returns 0 on success, negative errno numbers on failure.
+ */
+int drm_atomic_helper_page_flip_on_target_vblank(
+				struct drm_crtc *crtc,
+				struct drm_framebuffer *fb,
+				struct drm_pending_vblank_event *event,
+				uint32_t flags,
+				uint32_t target)
+{
+	struct drm_plane *plane = crtc->primary;
+	struct drm_atomic_state *state;
+	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;
+
+	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
+
+retry:
+	ret = drm_atomic_helper_page_flip_create_state(state, crtc, fb, event);
 	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);
+	crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
+	if (WARN_ON(!crtc_state)) {
 		ret = -EINVAL;
 		goto fail;
 	}
+	crtc_state->target_vblank = target;
 
 	ret = drm_atomic_nonblocking_commit(state);
+
 fail:
 	if (ret == -EDEADLK)
 		goto backoff;
@@ -2805,19 +2900,10 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 	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;
-
+	drm_atomic_helper_page_flip_prepare_retry(state, plane);
 	goto retry;
 }
-EXPORT_SYMBOL(drm_atomic_helper_page_flip);
+EXPORT_SYMBOL(drm_atomic_helper_page_flip_on_target_vblank);
 
 /**
  * drm_atomic_helper_connector_dpms() - connector dpms helper implementation
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 7ff92b0..e8cb6b7 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -124,6 +124,12 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
 				struct drm_pending_vblank_event *event,
 				uint32_t flags);
+int drm_atomic_helper_page_flip_on_target_vblank(
+				struct drm_crtc *crtc,
+				struct drm_framebuffer *fb,
+				struct drm_pending_vblank_event *event,
+				uint32_t flags,
+				uint32_t target);
 int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
 				     int mode);
 struct drm_encoder *
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 946672f..cc926dc 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -103,6 +103,7 @@ static inline uint64_t I642U64(int64_t val)
  * @ctm: Transformation matrix
  * @gamma_lut: Lookup table for converting pixel data after the
  *	conversion matrix
+ * @target_vblank: Target vblank count to flip
  * @state: backpointer to global drm_atomic_state
  *
  * Note that the distinction between @enable and @active is rather subtile:
@@ -156,6 +157,8 @@ struct drm_crtc_state {
 	struct drm_property_blob *ctm;
 	struct drm_property_blob *gamma_lut;
 
+	u32 target_vblank;
+
 	/**
 	 * @event:
 	 *
-- 
1.9.1

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

Re: [PATCH] drm/atomic: Add target_vblank support in atomic helpers.

[Daniel Vetter]

On Tue, Jan 03, 2017 at 10:06:38AM -0500, Andrey Grodzovsky wrote:
> Allows usage of the new page_flip_target hook for
> drivers implementing the atomic path.
> Provides default atomic helper for the new hook.
> 
> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>

Please keep a per-patch changelog so that it's easier for everyone to
follow the evolution of this patch.

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 146 ++++++++++++++++++++++++++++--------
>  include/drm/drm_atomic_helper.h     |   6 ++
>  include/drm/drm_crtc.h              |   3 +
>  3 files changed, 125 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 583f47f..5e76f90 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2733,6 +2733,59 @@ int drm_atomic_helper_resume(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
>  
> +static int drm_atomic_helper_page_flip_create_state(

I'd just call this page_flip_common without the long namespace prefix
since it's static. And without the create_state name since this function
also does some basic checks.

> +				struct drm_atomic_state *state,
> +				struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb,
> +				struct drm_pending_vblank_event *event)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_plane_state *plane_state;
> +	struct drm_crtc_state *crtc_state;
> +	int ret = 0;
> +
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
> +	crtc_state->event = event;
> +
> +	plane_state = drm_atomic_get_plane_state(state, plane);
> +	if (IS_ERR(plane_state))
> +		return PTR_ERR(plane_state);
> +
> +
> +	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> +	if (ret != 0)
> +		return ret;
> +	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);
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static void drm_atomic_helper_page_flip_prepare_retry(
> +				struct drm_atomic_state *state,
> +				struct drm_plane *plane)
> +{
> +	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;

There's a lot more places where we do this trick, please either refactor
them all, or none. Personally I'd go with none since the function call is
more verbose than the assignement, and you're hiding this rather important
comment behind a function name that doesn't say what games are being
played here.

> +}
> +
>  /**
>   * drm_atomic_helper_page_flip - execute a legacy page flip
>   * @crtc: DRM crtc
> @@ -2756,8 +2809,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  {
>  	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;
>  
>  	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> @@ -2768,35 +2819,79 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  		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);
> +	ret = drm_atomic_helper_page_flip_create_state(state, crtc, fb, event);
> +	if (ret != 0)
>  		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_nonblocking_commit(state);
>  
> -	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> +fail:
> +	if (ret == -EDEADLK)
> +		goto backoff;
> +
> +	drm_atomic_state_put(state);
> +	return ret;
> +
> +backoff:
> +	drm_atomic_helper_page_flip_prepare_retry(state, plane);
> +	goto retry;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> +
> +/**
> + * drm_atomic_helper_page_flip - execute a legacy page flip
> + * @crtc: DRM crtc
> + * @fb: DRM framebuffer
> + * @event: optional DRM event to signal upon completion
> + * @flags: flip flags for non-vblank sync'ed updates
> + *
> + * Provides a default page flip on specific vblank implementation using
> + * the atomic driver interface.
> + *
> + * 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.
> + *
> + * Returns:
> + * Returns 0 on success, negative errno numbers on failure.

Kerneldoc isn't updated, just verbatim copypaste. Please also linke these
2 functions. And if you have time, would be good to sprinkle links to the
vfunc hooks for each of these default helpers (they're unfortunately
missing ...), e.g. here &drm_crtc_funcs.page_flip_target.

And please build the docs per http://blog.ffwll.ch/2016/12/howto-docs.html
and make sure it all looks pretty.

> + */
> +int drm_atomic_helper_page_flip_on_target_vblank(

Please name this helper to match the vhook it's supposed to slot into,
i.e. drm_atomic_helper_page_flip_target.

> +				struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb,
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags,
> +				uint32_t target)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_atomic_state *state;
> +	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;
> +
> +	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
> +
> +retry:
> +	ret = drm_atomic_helper_page_flip_create_state(state, crtc, fb, event);
>  	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);
> +	crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> +	if (WARN_ON(!crtc_state)) {
>  		ret = -EINVAL;
>  		goto fail;
>  	}
> +	crtc_state->target_vblank = target;
>  
>  	ret = drm_atomic_nonblocking_commit(state);
> +
>  fail:
>  	if (ret == -EDEADLK)
>  		goto backoff;
> @@ -2805,19 +2900,10 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  	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;
> -
> +	drm_atomic_helper_page_flip_prepare_retry(state, plane);
>  	goto retry;
>  }
> -EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> +EXPORT_SYMBOL(drm_atomic_helper_page_flip_on_target_vblank);
>  
>  /**
>   * drm_atomic_helper_connector_dpms() - connector dpms helper implementation
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 7ff92b0..e8cb6b7 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -124,6 +124,12 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
>  				struct drm_pending_vblank_event *event,
>  				uint32_t flags);
> +int drm_atomic_helper_page_flip_on_target_vblank(
> +				struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb,
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags,
> +				uint32_t target);
>  int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
>  				     int mode);
>  struct drm_encoder *
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 946672f..cc926dc 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -103,6 +103,7 @@ static inline uint64_t I642U64(int64_t val)
>   * @ctm: Transformation matrix
>   * @gamma_lut: Lookup table for converting pixel data after the
>   *	conversion matrix
> + * @target_vblank: Target vblank count to flip
>   * @state: backpointer to global drm_atomic_state
>   *
>   * Note that the distinction between @enable and @active is rather subtile:
> @@ -156,6 +157,8 @@ struct drm_crtc_state {
>  	struct drm_property_blob *ctm;
>  	struct drm_property_blob *gamma_lut;
>  
> +	u32 target_vblank;

The in-line style is preferred for structures, since it allows to group
comments with each member. There's also still the question of how drivers
opt-in into target_vblank (only amdgpu, and then only in your private
branch supports it). I think that should be documented in the kernel-doc.

Thanks, Daniel
> +
>  	/**
>  	 * @event:
>  	 *
> -- 
> 1.9.1
> 

-- 
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] drm/atomic: Add target_vblank support in atomic helpers.

[Grodzovsky, Andrey]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, January 04, 2017 3:44 AM
> To: Grodzovsky, Andrey
> Cc: dri-devel@lists.freedesktop.org; daniel.vetter@ffwll.ch; Deucher,
> Alexander; Daenzer, Michel; Wentland, Harry
> Subject: Re: [PATCH] drm/atomic: Add target_vblank support in atomic
> helpers.
> 
> On Tue, Jan 03, 2017 at 10:06:38AM -0500, Andrey Grodzovsky wrote:
> > Allows usage of the new page_flip_target hook for drivers implementing
> > the atomic path.
> > Provides default atomic helper for the new hook.
> >
> > Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> 
> Please keep a per-patch changelog so that it's easier for everyone to follow
> the evolution of this patch.
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 146
> ++++++++++++++++++++++++++++--------
> >  include/drm/drm_atomic_helper.h     |   6 ++
> >  include/drm/drm_crtc.h              |   3 +
> >  3 files changed, 125 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 583f47f..5e76f90 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2733,6 +2733,59 @@ int drm_atomic_helper_resume(struct
> drm_device
> > *dev,  }  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
> >
> > +static int drm_atomic_helper_page_flip_create_state(
> 
> I'd just call this page_flip_common without the long namespace prefix since
> it's static. And without the create_state name since this function also does
> some basic checks.
> 
> > +				struct drm_atomic_state *state,
> > +				struct drm_crtc *crtc,
> > +				struct drm_framebuffer *fb,
> > +				struct drm_pending_vblank_event *event) {
> > +	struct drm_plane *plane = crtc->primary;
> > +	struct drm_plane_state *plane_state;
> > +	struct drm_crtc_state *crtc_state;
> > +	int ret = 0;
> > +
> > +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > +	if (IS_ERR(crtc_state))
> > +		return PTR_ERR(crtc_state);
> > +
> > +	crtc_state->event = event;
> > +
> > +	plane_state = drm_atomic_get_plane_state(state, plane);
> > +	if (IS_ERR(plane_state))
> > +		return PTR_ERR(plane_state);
> > +
> > +
> > +	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> > +	if (ret != 0)
> > +		return ret;
> > +	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);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void drm_atomic_helper_page_flip_prepare_retry(
> > +				struct drm_atomic_state *state,
> > +				struct drm_plane *plane)
> > +{
> > +	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;
> 
> There's a lot more places where we do this trick, please either refactor them
> all, or none. Personally I'd go with none since the function call is more
> verbose than the assignement, and you're hiding this rather important
> comment behind a function name that doesn't say what games are being
> played here.
> 
> > +}
> > +
> >  /**
> >   * drm_atomic_helper_page_flip - execute a legacy page flip
> >   * @crtc: DRM crtc
> > @@ -2756,8 +2809,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc
> > *crtc,  {
> >  	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;
> >
> >  	if (flags & DRM_MODE_PAGE_FLIP_ASYNC) @@ -2768,35 +2819,79
> @@ int
> > drm_atomic_helper_page_flip(struct drm_crtc *crtc,
> >  		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);
> > +	ret = drm_atomic_helper_page_flip_create_state(state, crtc, fb,
> event);
> > +	if (ret != 0)
> >  		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_nonblocking_commit(state);
> >
> > -	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> > +fail:
> > +	if (ret == -EDEADLK)
> > +		goto backoff;
> > +
> > +	drm_atomic_state_put(state);
> > +	return ret;
> > +
> > +backoff:
> > +	drm_atomic_helper_page_flip_prepare_retry(state, plane);
> > +	goto retry;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> > +
> > +/**
> > + * drm_atomic_helper_page_flip - execute a legacy page flip
> > + * @crtc: DRM crtc
> > + * @fb: DRM framebuffer
> > + * @event: optional DRM event to signal upon completion
> > + * @flags: flip flags for non-vblank sync'ed updates
> > + *
> > + * Provides a default page flip on specific vblank implementation
> > +using
> > + * the atomic driver interface.
> > + *
> > + * 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.
> > + *
> > + * Returns:
> > + * Returns 0 on success, negative errno numbers on failure.
> 
> Kerneldoc isn't updated, just verbatim copypaste. Please also linke these
> 2 functions. And if you have time, would be good to sprinkle links to the

Not sure here, do you mean kerneldoc is not generated for the new helper function ? If so ,
Is it because @tags are missing ?

> vfunc hooks for each of these default helpers (they're unfortunately missing
> ...), e.g. here &drm_crtc_funcs.page_flip_target.

Do you mean adding kernedoc for the vfunc hook ?

Thanks, Andrey
> 
> And please build the docs per http://blog.ffwll.ch/2016/12/howto-docs.html
> and make sure it all looks pretty.
> 
> > + */
> > +int drm_atomic_helper_page_flip_on_target_vblank(
> 
> Please name this helper to match the vhook it's supposed to slot into, i.e.
> drm_atomic_helper_page_flip_target.
> 
> > +				struct drm_crtc *crtc,
> > +				struct drm_framebuffer *fb,
> > +				struct drm_pending_vblank_event *event,
> > +				uint32_t flags,
> > +				uint32_t target)
> > +{
> > +	struct drm_plane *plane = crtc->primary;
> > +	struct drm_atomic_state *state;
> > +	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;
> > +
> > +	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
> > +
> > +retry:
> > +	ret = drm_atomic_helper_page_flip_create_state(state, crtc, fb,
> > +event);
> >  	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);
> > +	crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> > +	if (WARN_ON(!crtc_state)) {
> >  		ret = -EINVAL;
> >  		goto fail;
> >  	}
> > +	crtc_state->target_vblank = target;
> >
> >  	ret = drm_atomic_nonblocking_commit(state);
> > +
> >  fail:
> >  	if (ret == -EDEADLK)
> >  		goto backoff;
> > @@ -2805,19 +2900,10 @@ int drm_atomic_helper_page_flip(struct
> drm_crtc *crtc,
> >  	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;
> > -
> > +	drm_atomic_helper_page_flip_prepare_retry(state, plane);
> >  	goto retry;
> >  }
> > -EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> > +EXPORT_SYMBOL(drm_atomic_helper_page_flip_on_target_vblank);
> >
> >  /**
> >   * drm_atomic_helper_connector_dpms() - connector dpms helper
> > implementation diff --git a/include/drm/drm_atomic_helper.h
> > b/include/drm/drm_atomic_helper.h index 7ff92b0..e8cb6b7 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -124,6 +124,12 @@ int drm_atomic_helper_page_flip(struct drm_crtc
> *crtc,
> >  				struct drm_framebuffer *fb,
> >  				struct drm_pending_vblank_event *event,
> >  				uint32_t flags);
> > +int drm_atomic_helper_page_flip_on_target_vblank(
> > +				struct drm_crtc *crtc,
> > +				struct drm_framebuffer *fb,
> > +				struct drm_pending_vblank_event *event,
> > +				uint32_t flags,
> > +				uint32_t target);
> >  int drm_atomic_helper_connector_dpms(struct drm_connector
> *connector,
> >  				     int mode);
> >  struct drm_encoder *
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index
> > 946672f..cc926dc 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -103,6 +103,7 @@ static inline uint64_t I642U64(int64_t val)
> >   * @ctm: Transformation matrix
> >   * @gamma_lut: Lookup table for converting pixel data after the
> >   *	conversion matrix
> > + * @target_vblank: Target vblank count to flip
> >   * @state: backpointer to global drm_atomic_state
> >   *
> >   * Note that the distinction between @enable and @active is rather
> subtile:
> > @@ -156,6 +157,8 @@ struct drm_crtc_state {
> >  	struct drm_property_blob *ctm;
> >  	struct drm_property_blob *gamma_lut;
> >
> > +	u32 target_vblank;
> 
> The in-line style is preferred for structures, since it allows to group comments
> with each member. There's also still the question of how drivers opt-in into
> target_vblank (only amdgpu, and then only in your private branch supports
> it). I think that should be documented in the kernel-doc.
> 
> Thanks, Daniel
> > +
> >  	/**
> >  	 * @event:
> >  	 *
> > --
> > 1.9.1
> >
> 
> --
> 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] drm/atomic: Add target_vblank support in atomic helpers.

[Daniel Vetter]

On Wed, Jan 04, 2017 at 04:36:50PM +0000, Grodzovsky, Andrey wrote:
> > > +/**
> > > + * drm_atomic_helper_page_flip - execute a legacy page flip
> > > + * @crtc: DRM crtc
> > > + * @fb: DRM framebuffer
> > > + * @event: optional DRM event to signal upon completion
> > > + * @flags: flip flags for non-vblank sync'ed updates
> > > + *
> > > + * Provides a default page flip on specific vblank implementation
> > > +using
> > > + * the atomic driver interface.
> > > + *
> > > + * 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.
> > > + *
> > > + * Returns:
> > > + * Returns 0 on success, negative errno numbers on failure.
> > 
> > Kerneldoc isn't updated, just verbatim copypaste. Please also linke these
> > 2 functions. And if you have time, would be good to sprinkle links to the
> 
> Not sure here, do you mean kerneldoc is not generated for the new helper function ? If so ,
> Is it because @tags are missing ?

You should get a warning about the missing @tag, yes. But having the exact
same text for both is pointless, so for similar functions it's good to
highlight the differences (and mabye link between the two using the
functions() syntax).
> 
> > vfunc hooks for each of these default helpers (they're unfortunately missing
> > ...), e.g. here &drm_crtc_funcs.page_flip_target.
> 
> Do you mean adding kernedoc for the vfunc hook ?

No, just linking to them.

http://blog.ffwll.ch/2016/12/howto-docs.html

and you can see the output, makes all this a bit easier to understand
probably.
-- 
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] drm/atomic: Add target_vblank support in atomic helpers (v2)

[Andrey Grodzovsky]

Allows usage of the new page_flip_target hook for drivers implementing 
the atomic path.
Provides default atomic helper for the new hook.

v2:
Update code sharing logic between exsiting and the new flip hooks.
Improve kerneldoc.

Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 133 ++++++++++++++++++++++++++++++------
 include/drm/drm_atomic_helper.h     |   6 ++
 include/drm/drm_crtc.h              |   9 +++
 3 files changed, 127 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 583f47f..a4e5477 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2733,6 +2733,44 @@ int drm_atomic_helper_resume(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
 
+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_plane *plane = crtc->primary;
+	struct drm_plane_state *plane_state;
+	struct drm_crtc_state *crtc_state;
+	int ret = 0;
+
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
+
+	crtc_state->event = event;
+
+	plane_state = drm_atomic_get_plane_state(state, plane);
+	if (IS_ERR(plane_state))
+		return PTR_ERR(plane_state);
+
+
+	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
+	if (ret != 0)
+		return ret;
+	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);
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
 /**
  * drm_atomic_helper_page_flip - execute a legacy page flip
  * @crtc: DRM crtc
@@ -2740,7 +2778,8 @@ int drm_atomic_helper_resume(struct drm_device *dev,
  * @event: optional DRM event to signal upon completion
  * @flags: flip flags for non-vblank sync'ed updates
  *
- * Provides a default page flip implementation using the atomic driver interface.
+ * Provides a default &drm_crtc_funcs.page_flip implementation
+ * using the atomic driver interface.
  *
  * 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
@@ -2748,6 +2787,9 @@ int drm_atomic_helper_resume(struct drm_device *dev,
  *
  * Returns:
  * Returns 0 on success, negative errno numbers on failure.
+ *
+ * See also:
+ * drm_atomic_helper_page_flip_target()
  */
 int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
@@ -2756,8 +2798,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 {
 	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;
 
 	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
@@ -2768,35 +2808,86 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 		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);
+	ret = page_flip_common(state, crtc, fb, event);
+	if (ret != 0)
 		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_nonblocking_commit(state);
 
-	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
+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;
+}
+EXPORT_SYMBOL(drm_atomic_helper_page_flip);
+
+/**
+ * drm_atomic_helper_page_flip_target - do page flip on target vblank period.
+ * @crtc: DRM crtc
+ * @fb: DRM framebuffer
+ * @event: optional DRM event to signal upon completion
+ * @flags: flip flags for non-vblank sync'ed updates
+ * @target: specifying the target vblank period when the flip to take effect
+ *
+ * Provides a default &drm_crtc_funcs.page_flip_target implementation.
+ * Similar to drm_atomic_helper_page_flip() with extra parameter to specify
+ * target vblank period to flip.
+ *
+ * Returns:
+ * Returns 0 on success, negative errno numbers on failure.
+ */
+int drm_atomic_helper_page_flip_target(
+				struct drm_crtc *crtc,
+				struct drm_framebuffer *fb,
+				struct drm_pending_vblank_event *event,
+				uint32_t flags,
+				uint32_t target)
+{
+	struct drm_plane *plane = crtc->primary;
+	struct drm_atomic_state *state;
+	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;
+
+	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
+
+retry:
+	ret = page_flip_common(state, crtc, fb, event);
 	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);
+	crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
+	if (WARN_ON(!crtc_state)) {
 		ret = -EINVAL;
 		goto fail;
 	}
+	crtc_state->target_vblank = target;
 
 	ret = drm_atomic_nonblocking_commit(state);
+
 fail:
 	if (ret == -EDEADLK)
 		goto backoff;
@@ -2817,7 +2908,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 
 	goto retry;
 }
-EXPORT_SYMBOL(drm_atomic_helper_page_flip);
+EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
 
 /**
  * drm_atomic_helper_connector_dpms() - connector dpms helper implementation
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 7ff92b0..b3b3abe 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -124,6 +124,12 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
 				struct drm_pending_vblank_event *event,
 				uint32_t flags);
+int drm_atomic_helper_page_flip_target(
+				struct drm_crtc *crtc,
+				struct drm_framebuffer *fb,
+				struct drm_pending_vblank_event *event,
+				uint32_t flags,
+				uint32_t target);
 int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
 				     int mode);
 struct drm_encoder *
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 946672f..5c77c3f 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -157,6 +157,15 @@ struct drm_crtc_state {
 	struct drm_property_blob *gamma_lut;
 
 	/**
+	 * @target_vblank:
+	 *
+	 * Target vertical blank period when a page flip
+	 * should take effect.
+	 */
+
+	u32 target_vblank;
+
+	/**
 	 * @event:
 	 *
 	 * Optional pointer to a DRM event to signal upon completion of the
-- 
1.9.1

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

Re: [PATCH] drm/atomic: Add target_vblank support in atomic helpers (v2)

[Daniel Vetter]

On Fri, Jan 06, 2017 at 03:39:40PM -0500, Andrey Grodzovsky wrote:
> Allows usage of the new page_flip_target hook for drivers implementing 
> the atomic path.
> Provides default atomic helper for the new hook.
> 
> v2:
> Update code sharing logic between exsiting and the new flip hooks.
> Improve kerneldoc.
> 
> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>

Looks all reasonable, I think an ack from Alex that the amd side is in
shape too, and I'll pull this into drm-misc.

Thanks, Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 133 ++++++++++++++++++++++++++++++------
>  include/drm/drm_atomic_helper.h     |   6 ++
>  include/drm/drm_crtc.h              |   9 +++
>  3 files changed, 127 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 583f47f..a4e5477 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2733,6 +2733,44 @@ int drm_atomic_helper_resume(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
>  
> +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_plane *plane = crtc->primary;
> +	struct drm_plane_state *plane_state;
> +	struct drm_crtc_state *crtc_state;
> +	int ret = 0;
> +
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
> +	crtc_state->event = event;
> +
> +	plane_state = drm_atomic_get_plane_state(state, plane);
> +	if (IS_ERR(plane_state))
> +		return PTR_ERR(plane_state);
> +
> +
> +	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> +	if (ret != 0)
> +		return ret;
> +	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);
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>  /**
>   * drm_atomic_helper_page_flip - execute a legacy page flip
>   * @crtc: DRM crtc
> @@ -2740,7 +2778,8 @@ int drm_atomic_helper_resume(struct drm_device *dev,
>   * @event: optional DRM event to signal upon completion
>   * @flags: flip flags for non-vblank sync'ed updates
>   *
> - * Provides a default page flip implementation using the atomic driver interface.
> + * Provides a default &drm_crtc_funcs.page_flip implementation
> + * using the atomic driver interface.
>   *
>   * 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
> @@ -2748,6 +2787,9 @@ int drm_atomic_helper_resume(struct drm_device *dev,
>   *
>   * Returns:
>   * Returns 0 on success, negative errno numbers on failure.
> + *
> + * See also:
> + * drm_atomic_helper_page_flip_target()
>   */
>  int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
> @@ -2756,8 +2798,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  {
>  	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;
>  
>  	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> @@ -2768,35 +2808,86 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  		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);
> +	ret = page_flip_common(state, crtc, fb, event);
> +	if (ret != 0)
>  		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_nonblocking_commit(state);
>  
> -	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> +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;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> +
> +/**
> + * drm_atomic_helper_page_flip_target - do page flip on target vblank period.
> + * @crtc: DRM crtc
> + * @fb: DRM framebuffer
> + * @event: optional DRM event to signal upon completion
> + * @flags: flip flags for non-vblank sync'ed updates
> + * @target: specifying the target vblank period when the flip to take effect
> + *
> + * Provides a default &drm_crtc_funcs.page_flip_target implementation.
> + * Similar to drm_atomic_helper_page_flip() with extra parameter to specify
> + * target vblank period to flip.
> + *
> + * Returns:
> + * Returns 0 on success, negative errno numbers on failure.
> + */
> +int drm_atomic_helper_page_flip_target(
> +				struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb,
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags,
> +				uint32_t target)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_atomic_state *state;
> +	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;
> +
> +	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
> +
> +retry:
> +	ret = page_flip_common(state, crtc, fb, event);
>  	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);
> +	crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> +	if (WARN_ON(!crtc_state)) {
>  		ret = -EINVAL;
>  		goto fail;
>  	}
> +	crtc_state->target_vblank = target;
>  
>  	ret = drm_atomic_nonblocking_commit(state);
> +
>  fail:
>  	if (ret == -EDEADLK)
>  		goto backoff;
> @@ -2817,7 +2908,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  
>  	goto retry;
>  }
> -EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> +EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
>  
>  /**
>   * drm_atomic_helper_connector_dpms() - connector dpms helper implementation
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 7ff92b0..b3b3abe 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -124,6 +124,12 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
>  				struct drm_pending_vblank_event *event,
>  				uint32_t flags);
> +int drm_atomic_helper_page_flip_target(
> +				struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb,
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags,
> +				uint32_t target);
>  int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
>  				     int mode);
>  struct drm_encoder *
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 946672f..5c77c3f 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -157,6 +157,15 @@ struct drm_crtc_state {
>  	struct drm_property_blob *gamma_lut;
>  
>  	/**
> +	 * @target_vblank:
> +	 *
> +	 * Target vertical blank period when a page flip
> +	 * should take effect.
> +	 */
> +
> +	u32 target_vblank;
> +
> +	/**
>  	 * @event:
>  	 *
>  	 * Optional pointer to a DRM event to signal upon completion of the
> -- 
> 1.9.1
> 

-- 
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] drm/atomic: Add target_vblank support in atomic helpers (v2)

[Michel Dänzer]

On 09/01/17 06:59 PM, Daniel Vetter wrote:
> On Fri, Jan 06, 2017 at 03:39:40PM -0500, Andrey Grodzovsky wrote:
>> Allows usage of the new page_flip_target hook for drivers implementing 
>> the atomic path.
>> Provides default atomic helper for the new hook.
>>
>> v2:
>> Update code sharing logic between exsiting and the new flip hooks.
>> Improve kerneldoc.
>>
>> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> 
> Looks all reasonable, I think an ack from Alex that the amd side is in
> shape too, and I'll pull this into drm-misc.

Andrey, is there an updated patch 2 adapted to current patch 1? Other
than that and some questionable indentation of parameters in function
signatures, looks good to me FWIW.


-- 
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] drm/atomic: Add target_vblank support in atomic helpers (v2)

[Grodzovsky, Andrey]

> -----Original Message-----
> From: Michel Dänzer [mailto:michel@daenzer.net]
> Sent: Wednesday, January 11, 2017 3:33 AM
> To: Daniel Vetter; Grodzovsky, Andrey
> Cc: Deucher, Alexander; dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH] drm/atomic: Add target_vblank support in atomic
> helpers (v2)
> 
> On 09/01/17 06:59 PM, Daniel Vetter wrote:
> > On Fri, Jan 06, 2017 at 03:39:40PM -0500, Andrey Grodzovsky wrote:
> >> Allows usage of the new page_flip_target hook for drivers
> >> implementing the atomic path.
> >> Provides default atomic helper for the new hook.
> >>
> >> v2:
> >> Update code sharing logic between exsiting and the new flip hooks.
> >> Improve kerneldoc.
> >>
> >> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> >
> > Looks all reasonable, I think an ack from Alex that the amd side is in
> > shape too, and I'll pull this into drm-misc.
> 
> Andrey, is there an updated patch 2 adapted to current patch 1? Other than
> that and some questionable indentation of parameters in function
> signatures, looks good to me FWIW.

We are unable to use the atomic helpers both for page_flip and page_flip_target
At their current form mostly due to DRM_MODE_PAGE_FLIP_ASYNC flag rejection they do. 
I discussed this with Daniel Vetter on IRC and suggested 
to remove the rejection but he said the precise semantics of 
atomic async flip is not clear yet and it's better to leave that out for now 
until there is a  userspace asking for it.
So I tested it by just hacking  the helper to remove the rejection.
Until that settled the original change [PATCH 2/2] drm/amd/dal: Switch to page_flip_target hook in DAL
Is what we plan to use in DAL

Andrey
> 
> 
> --
> 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] drm/atomic: Add target_vblank support in atomic helpers (v2)

[Deucher, Alexander]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Monday, January 09, 2017 4:59 AM
> To: Grodzovsky, Andrey
> Cc: dri-devel@lists.freedesktop.org; daniel.vetter@ffwll.ch; Deucher,
> Alexander; Daenzer, Michel; Wentland, Harry
> Subject: Re: [PATCH] drm/atomic: Add target_vblank support in atomic
> helpers (v2)
> 
> On Fri, Jan 06, 2017 at 03:39:40PM -0500, Andrey Grodzovsky wrote:
> > Allows usage of the new page_flip_target hook for drivers implementing
> > the atomic path.
> > Provides default atomic helper for the new hook.
> >
> > v2:
> > Update code sharing logic between exsiting and the new flip hooks.
> > Improve kerneldoc.
> >
> > Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> 
> Looks all reasonable, I think an ack from Alex that the amd side is in
> shape too, and I'll pull this into drm-misc.

I had a discussion about this with Andrey and I think we are in good shape.

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> 
> Thanks, Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 133
> ++++++++++++++++++++++++++++++------
> >  include/drm/drm_atomic_helper.h     |   6 ++
> >  include/drm/drm_crtc.h              |   9 +++
> >  3 files changed, 127 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> > index 583f47f..a4e5477 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2733,6 +2733,44 @@ int drm_atomic_helper_resume(struct
> drm_device *dev,
> >  }
> >  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
> >
> > +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_plane *plane = crtc->primary;
> > +	struct drm_plane_state *plane_state;
> > +	struct drm_crtc_state *crtc_state;
> > +	int ret = 0;
> > +
> > +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > +	if (IS_ERR(crtc_state))
> > +		return PTR_ERR(crtc_state);
> > +
> > +	crtc_state->event = event;
> > +
> > +	plane_state = drm_atomic_get_plane_state(state, plane);
> > +	if (IS_ERR(plane_state))
> > +		return PTR_ERR(plane_state);
> > +
> > +
> > +	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> > +	if (ret != 0)
> > +		return ret;
> > +	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);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  /**
> >   * drm_atomic_helper_page_flip - execute a legacy page flip
> >   * @crtc: DRM crtc
> > @@ -2740,7 +2778,8 @@ int drm_atomic_helper_resume(struct
> drm_device *dev,
> >   * @event: optional DRM event to signal upon completion
> >   * @flags: flip flags for non-vblank sync'ed updates
> >   *
> > - * Provides a default page flip implementation using the atomic driver
> interface.
> > + * Provides a default &drm_crtc_funcs.page_flip implementation
> > + * using the atomic driver interface.
> >   *
> >   * 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
> > @@ -2748,6 +2787,9 @@ int drm_atomic_helper_resume(struct
> drm_device *dev,
> >   *
> >   * Returns:
> >   * Returns 0 on success, negative errno numbers on failure.
> > + *
> > + * See also:
> > + * drm_atomic_helper_page_flip_target()
> >   */
> >  int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
> >  				struct drm_framebuffer *fb,
> > @@ -2756,8 +2798,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc
> *crtc,
> >  {
> >  	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;
> >
> >  	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> > @@ -2768,35 +2808,86 @@ int drm_atomic_helper_page_flip(struct
> drm_crtc *crtc,
> >  		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);
> > +	ret = page_flip_common(state, crtc, fb, event);
> > +	if (ret != 0)
> >  		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_nonblocking_commit(state);
> >
> > -	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> > +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;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> > +
> > +/**
> > + * drm_atomic_helper_page_flip_target - do page flip on target vblank
> period.
> > + * @crtc: DRM crtc
> > + * @fb: DRM framebuffer
> > + * @event: optional DRM event to signal upon completion
> > + * @flags: flip flags for non-vblank sync'ed updates
> > + * @target: specifying the target vblank period when the flip to take
> effect
> > + *
> > + * Provides a default &drm_crtc_funcs.page_flip_target implementation.
> > + * Similar to drm_atomic_helper_page_flip() with extra parameter to
> specify
> > + * target vblank period to flip.
> > + *
> > + * Returns:
> > + * Returns 0 on success, negative errno numbers on failure.
> > + */
> > +int drm_atomic_helper_page_flip_target(
> > +				struct drm_crtc *crtc,
> > +				struct drm_framebuffer *fb,
> > +				struct drm_pending_vblank_event *event,
> > +				uint32_t flags,
> > +				uint32_t target)
> > +{
> > +	struct drm_plane *plane = crtc->primary;
> > +	struct drm_atomic_state *state;
> > +	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;
> > +
> > +	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
> > +
> > +retry:
> > +	ret = page_flip_common(state, crtc, fb, event);
> >  	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);
> > +	crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> > +	if (WARN_ON(!crtc_state)) {
> >  		ret = -EINVAL;
> >  		goto fail;
> >  	}
> > +	crtc_state->target_vblank = target;
> >
> >  	ret = drm_atomic_nonblocking_commit(state);
> > +
> >  fail:
> >  	if (ret == -EDEADLK)
> >  		goto backoff;
> > @@ -2817,7 +2908,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc
> *crtc,
> >
> >  	goto retry;
> >  }
> > -EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> > +EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
> >
> >  /**
> >   * drm_atomic_helper_connector_dpms() - connector dpms helper
> implementation
> > diff --git a/include/drm/drm_atomic_helper.h
> b/include/drm/drm_atomic_helper.h
> > index 7ff92b0..b3b3abe 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -124,6 +124,12 @@ int drm_atomic_helper_page_flip(struct drm_crtc
> *crtc,
> >  				struct drm_framebuffer *fb,
> >  				struct drm_pending_vblank_event *event,
> >  				uint32_t flags);
> > +int drm_atomic_helper_page_flip_target(
> > +				struct drm_crtc *crtc,
> > +				struct drm_framebuffer *fb,
> > +				struct drm_pending_vblank_event *event,
> > +				uint32_t flags,
> > +				uint32_t target);
> >  int drm_atomic_helper_connector_dpms(struct drm_connector
> *connector,
> >  				     int mode);
> >  struct drm_encoder *
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 946672f..5c77c3f 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -157,6 +157,15 @@ struct drm_crtc_state {
> >  	struct drm_property_blob *gamma_lut;
> >
> >  	/**
> > +	 * @target_vblank:
> > +	 *
> > +	 * Target vertical blank period when a page flip
> > +	 * should take effect.
> > +	 */
> > +
> > +	u32 target_vblank;
> > +
> > +	/**
> >  	 * @event:
> >  	 *
> >  	 * Optional pointer to a DRM event to signal upon completion of the
> > --
> > 1.9.1
> >
> 
> --
> 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] drm/atomic: Add target_vblank support in atomic helpers (v2)

[Daniel Vetter]

On Wed, Jan 11, 2017 at 07:57:11PM +0000, Deucher, Alexander wrote:
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Monday, January 09, 2017 4:59 AM
> > To: Grodzovsky, Andrey
> > Cc: dri-devel@lists.freedesktop.org; daniel.vetter@ffwll.ch; Deucher,
> > Alexander; Daenzer, Michel; Wentland, Harry
> > Subject: Re: [PATCH] drm/atomic: Add target_vblank support in atomic
> > helpers (v2)
> > 
> > On Fri, Jan 06, 2017 at 03:39:40PM -0500, Andrey Grodzovsky wrote:
> > > Allows usage of the new page_flip_target hook for drivers implementing
> > > the atomic path.
> > > Provides default atomic helper for the new hook.
> > >
> > > v2:
> > > Update code sharing logic between exsiting and the new flip hooks.
> > > Improve kerneldoc.
> > >
> > > Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> > 
> > Looks all reasonable, I think an ack from Alex that the amd side is in
> > shape too, and I'll pull this into drm-misc.
> 
> I had a discussion about this with Andrey and I think we are in good shape.
> 
> Acked-by: Alex Deucher <alexander.deucher@amd.com>

Applied to drm-misc, thanks.
-Daniel

> 
> > 
> > Thanks, Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 133
> > ++++++++++++++++++++++++++++++------
> > >  include/drm/drm_atomic_helper.h     |   6 ++
> > >  include/drm/drm_crtc.h              |   9 +++
> > >  3 files changed, 127 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 583f47f..a4e5477 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -2733,6 +2733,44 @@ int drm_atomic_helper_resume(struct
> > drm_device *dev,
> > >  }
> > >  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
> > >
> > > +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_plane *plane = crtc->primary;
> > > +	struct drm_plane_state *plane_state;
> > > +	struct drm_crtc_state *crtc_state;
> > > +	int ret = 0;
> > > +
> > > +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > > +	if (IS_ERR(crtc_state))
> > > +		return PTR_ERR(crtc_state);
> > > +
> > > +	crtc_state->event = event;
> > > +
> > > +	plane_state = drm_atomic_get_plane_state(state, plane);
> > > +	if (IS_ERR(plane_state))
> > > +		return PTR_ERR(plane_state);
> > > +
> > > +
> > > +	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> > > +	if (ret != 0)
> > > +		return ret;
> > > +	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);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  /**
> > >   * drm_atomic_helper_page_flip - execute a legacy page flip
> > >   * @crtc: DRM crtc
> > > @@ -2740,7 +2778,8 @@ int drm_atomic_helper_resume(struct
> > drm_device *dev,
> > >   * @event: optional DRM event to signal upon completion
> > >   * @flags: flip flags for non-vblank sync'ed updates
> > >   *
> > > - * Provides a default page flip implementation using the atomic driver
> > interface.
> > > + * Provides a default &drm_crtc_funcs.page_flip implementation
> > > + * using the atomic driver interface.
> > >   *
> > >   * 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
> > > @@ -2748,6 +2787,9 @@ int drm_atomic_helper_resume(struct
> > drm_device *dev,
> > >   *
> > >   * Returns:
> > >   * Returns 0 on success, negative errno numbers on failure.
> > > + *
> > > + * See also:
> > > + * drm_atomic_helper_page_flip_target()
> > >   */
> > >  int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
> > >  				struct drm_framebuffer *fb,
> > > @@ -2756,8 +2798,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc
> > *crtc,
> > >  {
> > >  	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;
> > >
> > >  	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> > > @@ -2768,35 +2808,86 @@ int drm_atomic_helper_page_flip(struct
> > drm_crtc *crtc,
> > >  		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);
> > > +	ret = page_flip_common(state, crtc, fb, event);
> > > +	if (ret != 0)
> > >  		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_nonblocking_commit(state);
> > >
> > > -	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> > > +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;
> > > +}
> > > +EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> > > +
> > > +/**
> > > + * drm_atomic_helper_page_flip_target - do page flip on target vblank
> > period.
> > > + * @crtc: DRM crtc
> > > + * @fb: DRM framebuffer
> > > + * @event: optional DRM event to signal upon completion
> > > + * @flags: flip flags for non-vblank sync'ed updates
> > > + * @target: specifying the target vblank period when the flip to take
> > effect
> > > + *
> > > + * Provides a default &drm_crtc_funcs.page_flip_target implementation.
> > > + * Similar to drm_atomic_helper_page_flip() with extra parameter to
> > specify
> > > + * target vblank period to flip.
> > > + *
> > > + * Returns:
> > > + * Returns 0 on success, negative errno numbers on failure.
> > > + */
> > > +int drm_atomic_helper_page_flip_target(
> > > +				struct drm_crtc *crtc,
> > > +				struct drm_framebuffer *fb,
> > > +				struct drm_pending_vblank_event *event,
> > > +				uint32_t flags,
> > > +				uint32_t target)
> > > +{
> > > +	struct drm_plane *plane = crtc->primary;
> > > +	struct drm_atomic_state *state;
> > > +	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;
> > > +
> > > +	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
> > > +
> > > +retry:
> > > +	ret = page_flip_common(state, crtc, fb, event);
> > >  	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);
> > > +	crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> > > +	if (WARN_ON(!crtc_state)) {
> > >  		ret = -EINVAL;
> > >  		goto fail;
> > >  	}
> > > +	crtc_state->target_vblank = target;
> > >
> > >  	ret = drm_atomic_nonblocking_commit(state);
> > > +
> > >  fail:
> > >  	if (ret == -EDEADLK)
> > >  		goto backoff;
> > > @@ -2817,7 +2908,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc
> > *crtc,
> > >
> > >  	goto retry;
> > >  }
> > > -EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> > > +EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
> > >
> > >  /**
> > >   * drm_atomic_helper_connector_dpms() - connector dpms helper
> > implementation
> > > diff --git a/include/drm/drm_atomic_helper.h
> > b/include/drm/drm_atomic_helper.h
> > > index 7ff92b0..b3b3abe 100644
> > > --- a/include/drm/drm_atomic_helper.h
> > > +++ b/include/drm/drm_atomic_helper.h
> > > @@ -124,6 +124,12 @@ int drm_atomic_helper_page_flip(struct drm_crtc
> > *crtc,
> > >  				struct drm_framebuffer *fb,
> > >  				struct drm_pending_vblank_event *event,
> > >  				uint32_t flags);
> > > +int drm_atomic_helper_page_flip_target(
> > > +				struct drm_crtc *crtc,
> > > +				struct drm_framebuffer *fb,
> > > +				struct drm_pending_vblank_event *event,
> > > +				uint32_t flags,
> > > +				uint32_t target);
> > >  int drm_atomic_helper_connector_dpms(struct drm_connector
> > *connector,
> > >  				     int mode);
> > >  struct drm_encoder *
> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > index 946672f..5c77c3f 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -157,6 +157,15 @@ struct drm_crtc_state {
> > >  	struct drm_property_blob *gamma_lut;
> > >
> > >  	/**
> > > +	 * @target_vblank:
> > > +	 *
> > > +	 * Target vertical blank period when a page flip
> > > +	 * should take effect.
> > > +	 */
> > > +
> > > +	u32 target_vblank;
> > > +
> > > +	/**
> > >  	 * @event:
> > >  	 *
> > >  	 * Optional pointer to a DRM event to signal upon completion of the
> > > --
> > > 1.9.1
> > >
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
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] drm/atomic: Add target_vblank support in atomic helpers (v2)

[Michel Dänzer]

On 12/01/17 12:48 AM, Grodzovsky, Andrey wrote:
>> From: Michel Dänzer [mailto:michel@daenzer.net]
>> On 09/01/17 06:59 PM, Daniel Vetter wrote:
>>> On Fri, Jan 06, 2017 at 03:39:40PM -0500, Andrey Grodzovsky wrote:
>>>> Allows usage of the new page_flip_target hook for drivers
>>>> implementing the atomic path.
>>>> Provides default atomic helper for the new hook.
>>>>
>>>> v2:
>>>> Update code sharing logic between exsiting and the new flip hooks.
>>>> Improve kerneldoc.
>>>>
>>>> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
>>>
>>> Looks all reasonable, I think an ack from Alex that the amd side is in
>>> shape too, and I'll pull this into drm-misc.
>>
>> Andrey, is there an updated patch 2 adapted to current patch 1? Other than
>> that and some questionable indentation of parameters in function
>> signatures, looks good to me FWIW.
> 
> We are unable to use the atomic helpers both for page_flip and page_flip_target
> At their current form mostly due to DRM_MODE_PAGE_FLIP_ASYNC flag rejection they do. 
> I discussed this with Daniel Vetter on IRC and suggested 
> to remove the rejection but he said the precise semantics of 
> atomic async flip is not clear yet and it's better to leave that out for now 
> until there is a  userspace asking for it.
> So I tested it by just hacking  the helper to remove the rejection.
> Until that settled the original change [PATCH 2/2] drm/amd/dal: Switch to page_flip_target hook in DAL
> Is what we plan to use in DAL

IIRC Daniel suggested (on IRC?) to use the helper for non-async flips
and the current DC code for async flips. Is that feasible?


-- 
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] drm/atomic: Add target_vblank support in atomic helpers (v2)

[Daniel Vetter]

On Thu, Jan 12, 2017 at 9:28 AM, Michel Dänzer <michel@daenzer.net> wrote:
> On 12/01/17 12:48 AM, Grodzovsky, Andrey wrote:
>>> From: Michel Dänzer [mailto:michel@daenzer.net]
>>> On 09/01/17 06:59 PM, Daniel Vetter wrote:
>>>> On Fri, Jan 06, 2017 at 03:39:40PM -0500, Andrey Grodzovsky wrote:
>>>>> Allows usage of the new page_flip_target hook for drivers
>>>>> implementing the atomic path.
>>>>> Provides default atomic helper for the new hook.
>>>>>
>>>>> v2:
>>>>> Update code sharing logic between exsiting and the new flip hooks.
>>>>> Improve kerneldoc.
>>>>>
>>>>> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
>>>>
>>>> Looks all reasonable, I think an ack from Alex that the amd side is in
>>>> shape too, and I'll pull this into drm-misc.
>>>
>>> Andrey, is there an updated patch 2 adapted to current patch 1? Other than
>>> that and some questionable indentation of parameters in function
>>> signatures, looks good to me FWIW.
>>
>> We are unable to use the atomic helpers both for page_flip and page_flip_target
>> At their current form mostly due to DRM_MODE_PAGE_FLIP_ASYNC flag rejection they do.
>> I discussed this with Daniel Vetter on IRC and suggested
>> to remove the rejection but he said the precise semantics of
>> atomic async flip is not clear yet and it's better to leave that out for now
>> until there is a  userspace asking for it.
>> So I tested it by just hacking  the helper to remove the rejection.
>> Until that settled the original change [PATCH 2/2] drm/amd/dal: Switch to page_flip_target hook in DAL
>> Is what we plan to use in DAL
>
> IIRC Daniel suggested (on IRC?) to use the helper for non-async flips
> and the current DC code for async flips. Is that feasible?

We do have some async flip flag reserved for atomic, so we could route
it through. But since atm there's no one asking for async flips on the
atomic ioctl I'm a bit wary for fear of ending up with ill-defined
semantics. But I guess if we do this for legacy pageflips only, and
make sure we do still reject async flips submitted through the atomic
ioctl that should be all fine. And it should allow amdgpu to entirely
get rid of the legacy flip path, which would be nice.

Once we have that we could even use it for cursor plane updates
(through the legacy ioctl, for drivers with universal planes), for
those drivers that support it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] drm/atomic: Add target_vblank support in atomic helpers (v2)

[Grodzovsky, Andrey]

> -----Original Message-----
> From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Thursday, January 12, 2017 3:51 AM
> To: Michel Dänzer
> Cc: Grodzovsky, Andrey; Deucher, Alexander; dri-
> devel@lists.freedesktop.org
> Subject: Re: [PATCH] drm/atomic: Add target_vblank support in atomic
> helpers (v2)
> 
> On Thu, Jan 12, 2017 at 9:28 AM, Michel Dänzer <michel@daenzer.net>
> wrote:
> > On 12/01/17 12:48 AM, Grodzovsky, Andrey wrote:
> >>> From: Michel Dänzer [mailto:michel@daenzer.net] On 09/01/17 06:59
> >>> PM, Daniel Vetter wrote:
> >>>> On Fri, Jan 06, 2017 at 03:39:40PM -0500, Andrey Grodzovsky wrote:
> >>>>> Allows usage of the new page_flip_target hook for drivers
> >>>>> implementing the atomic path.
> >>>>> Provides default atomic helper for the new hook.
> >>>>>
> >>>>> v2:
> >>>>> Update code sharing logic between exsiting and the new flip hooks.
> >>>>> Improve kerneldoc.
> >>>>>
> >>>>> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> >>>>
> >>>> Looks all reasonable, I think an ack from Alex that the amd side is
> >>>> in shape too, and I'll pull this into drm-misc.
> >>>
> >>> Andrey, is there an updated patch 2 adapted to current patch 1?
> >>> Other than that and some questionable indentation of parameters in
> >>> function signatures, looks good to me FWIW.
> >>
> >> We are unable to use the atomic helpers both for page_flip and
> >> page_flip_target At their current form mostly due to
> DRM_MODE_PAGE_FLIP_ASYNC flag rejection they do.
> >> I discussed this with Daniel Vetter on IRC and suggested to remove
> >> the rejection but he said the precise semantics of atomic async flip
> >> is not clear yet and it's better to leave that out for now until
> >> there is a  userspace asking for it.
> >> So I tested it by just hacking  the helper to remove the rejection.
> >> Until that settled the original change [PATCH 2/2] drm/amd/dal:
> >> Switch to page_flip_target hook in DAL Is what we plan to use in DAL
> >
> > IIRC Daniel suggested (on IRC?) to use the helper for non-async flips
> > and the current DC code for async flips. Is that feasible?
> 
> We do have some async flip flag reserved for atomic, so we could route it
> through. But since atm there's no one asking for async flips on the atomic
> ioctl I'm a bit wary for fear of ending up with ill-defined semantics. But I guess
> if we do this for legacy pageflips only, and make sure we do still reject async
> flips submitted through the atomic ioctl that should be all fine. And it should
> allow amdgpu to entirely get rid of the legacy flip path, which would be nice.
> 
> Once we have that we could even use it for cursor plane updates (through
> the legacy ioctl, for drivers with universal planes), for those drivers that
> support it.
> -Daniel

So are we ok with a follow-up patch removing the ASYNC_FLIP restriction in the legacy IOCTL
+ adding the drm_mode_crtc_page_flip_target.flags to drm_crtc_state ? In that case as I said before,
at least for DAL we could drop our own page_flip hook and use the avaialbe helpers.

Andrey
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Add target_vblank support in atomic helpers (v2)

[Daniel Vetter]

On Thu, Jan 12, 2017 at 06:18:20PM +0000, Grodzovsky, Andrey wrote:
> > -----Original Message-----
> > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> > Daniel Vetter
> > Sent: Thursday, January 12, 2017 3:51 AM
> > To: Michel Dänzer
> > Cc: Grodzovsky, Andrey; Deucher, Alexander; dri-
> > devel@lists.freedesktop.org
> > Subject: Re: [PATCH] drm/atomic: Add target_vblank support in atomic
> > helpers (v2)
> > 
> > On Thu, Jan 12, 2017 at 9:28 AM, Michel Dänzer <michel@daenzer.net>
> > wrote:
> > > On 12/01/17 12:48 AM, Grodzovsky, Andrey wrote:
> > >>> From: Michel Dänzer [mailto:michel@daenzer.net] On 09/01/17 06:59
> > >>> PM, Daniel Vetter wrote:
> > >>>> On Fri, Jan 06, 2017 at 03:39:40PM -0500, Andrey Grodzovsky wrote:
> > >>>>> Allows usage of the new page_flip_target hook for drivers
> > >>>>> implementing the atomic path.
> > >>>>> Provides default atomic helper for the new hook.
> > >>>>>
> > >>>>> v2:
> > >>>>> Update code sharing logic between exsiting and the new flip hooks.
> > >>>>> Improve kerneldoc.
> > >>>>>
> > >>>>> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> > >>>>
> > >>>> Looks all reasonable, I think an ack from Alex that the amd side is
> > >>>> in shape too, and I'll pull this into drm-misc.
> > >>>
> > >>> Andrey, is there an updated patch 2 adapted to current patch 1?
> > >>> Other than that and some questionable indentation of parameters in
> > >>> function signatures, looks good to me FWIW.
> > >>
> > >> We are unable to use the atomic helpers both for page_flip and
> > >> page_flip_target At their current form mostly due to
> > DRM_MODE_PAGE_FLIP_ASYNC flag rejection they do.
> > >> I discussed this with Daniel Vetter on IRC and suggested to remove
> > >> the rejection but he said the precise semantics of atomic async flip
> > >> is not clear yet and it's better to leave that out for now until
> > >> there is a  userspace asking for it.
> > >> So I tested it by just hacking  the helper to remove the rejection.
> > >> Until that settled the original change [PATCH 2/2] drm/amd/dal:
> > >> Switch to page_flip_target hook in DAL Is what we plan to use in DAL
> > >
> > > IIRC Daniel suggested (on IRC?) to use the helper for non-async flips
> > > and the current DC code for async flips. Is that feasible?
> > 
> > We do have some async flip flag reserved for atomic, so we could route it
> > through. But since atm there's no one asking for async flips on the atomic
> > ioctl I'm a bit wary for fear of ending up with ill-defined semantics. But I guess
> > if we do this for legacy pageflips only, and make sure we do still reject async
> > flips submitted through the atomic ioctl that should be all fine. And it should
> > allow amdgpu to entirely get rid of the legacy flip path, which would be nice.
> > 
> > Once we have that we could even use it for cursor plane updates (through
> > the legacy ioctl, for drivers with universal planes), for those drivers that
> > support it.
> > -Daniel
> 
> So are we ok with a follow-up patch removing the ASYNC_FLIP restriction in the legacy IOCTL
> + adding the drm_mode_crtc_page_flip_target.flags to drm_crtc_state ? In that case as I said before,
> at least for DAL we could drop our own page_flip hook and use the avaialbe helpers.

Yeah, reconsidering all I think that'd be rather reasonable approach.
-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