-----Original Message----- From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com] Sent: Monday, January 30, 2017 6:05 AM To: Grodzovsky, Andrey Cc: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; nouveau@lists.freedesktop.org; daniel.vetter@intel.com; dc_upstream Subject: Re: [v3 PATCH 1/3] drm/atomic: Save flip flags in drm_crtct_state

Hi Andrey,

Thank you for the patch.

On Saturday 28 Jan 2017 21:26:49 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_crtc_state to be used in the low level drivers.

v2: Resending the patch since the original was broken.

v3: Save flag in crtc_state instead of plane_state

Signed-off-by: Andrey Grodzovsky Andrey.Grodzovsky@amd.com

drivers/gpu/drm/drm_atomic_helper.c | 19 +++++-------------- include/drm/drm_crtc.h | 8 +++++++- include/drm/drm_plane.h | 1 + 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index a4e5477..28065ee 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; @@ -2749,12 +2750,12 @@

static

...

int page_flip_common( return PTR_ERR(crtc_state);

crtc_state->event = event;

• crtc_state->pflip_flags = flags;

  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;

@@ -2781,10 +2782,6 @@ static int page_flip_common(

• 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 - * no provisions for this yet.

• • Returns:
  • Returns 0 on success, negative errno numbers on failure.

@@ -2800,9 +2797,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;

@@ -2810,7 +2804,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc, state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);

retry:

• ret = page_flip_common(state, crtc, fb, event);

• ret = page_flip_common(state, crtc, fb, event, flags); if (ret != 0) goto fail;

@@ -2865,9 +2859,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;

@@ -2875,7 +2866,7 @@ int drm_atomic_helper_page_flip_target( state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);

retry:

• ret = page_flip_common(state, crtc, fb, event);

• ret = page_flip_common(state, crtc, fb, event, flags); if (ret != 0) goto fail;

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5c77c3f..76457a4 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -162,10 +162,16 @@ struct drm_crtc_state { * Target vertical blank period when a page flip * should take effect. */

• u32 target_vblank;

  /**

• * @pflip_flags:
  

• *
  

• * Flip related config options
  

This isn't detailed enough. I propose something along the lines of

"DRM_MODE_PAGE_FLIP_* page flip flags, as passed to the page flip ioctl. Always zero for atomic commits that don't originate from a page flip ioctl."

You will then also need to reset the field to 0 at an appropriate point, as it's more an atomic commit transaction information than a state. Apart from that this patch looks good to me.

Thanks for your comments, i am not sure I understand why the reset is needed, for any future commit on same crtc the new state will have the field empty and if it's a flip IOCTL the field will be filled as needed in page_flip_common, otherwise it will stay empty. If the last commit on that crtc was a flip then why not keep this field with the bits that the user mode set ? Thanks, [Grodzovsky, Andrey]

...

• */
  

• u32 pflip_flags;
• /**
  • @event:
  • Optional pointer to a DRM event to signal upon completion of the

diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index db3bbde..57414ae 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -122,6 +122,7 @@ struct drm_plane_state { */ bool visible;

struct drm_atomic_state *state; };

-- Regards,

Laurent Pinchart

On 2017-01-28 09:26 PM, Andrey Grodzovsky wrote:

Swicth to use atomic helper. Start using actual user's given target_vblank value for flip instead of current value.

v3: Update for movig pflip flags to crtc_state

Change-Id: I25dae6d8c29de5d022a42aa99a18a32674b56cda 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 | 109 ++++----------------- 2 files changed, 19 insertions(+), 91 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..148780d 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

@@ -1626,7 +1549,8 @@ static void clear_unrelated_fields(struct drm_plane_state *state) static bool page_flip_needed( const struct drm_plane_state *new_state, const struct drm_plane_state *old_state,

• bool commit_surface_required)

• bool commit_surface_required,
• uint32_t pflip_flags)

{ struct drm_plane_state old_state_tmp; struct drm_plane_state new_state_tmp; @@ -1679,7 +1603,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 (pflip_flags & DRM_MODE_PAGE_FLIP_ASYNC)
  page_flip_required = true;
  

  } return page_flip_required;

@@ -2696,7 +2620,9 @@ int amdgpu_dm_atomic_commit( * 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, crtc->state->pflip_flags)
  

• 		||
  	action == DM_COMMIT_ACTION_DPMS_ON ||
  	action == DM_COMMIT_ACTION_SET) {
  

Might be good to clean up indentation to conform a bit more to kernel style. Something like the following, I think (I hope Thunderbird doesn't mangle it):

if (!page_flip_needed(plane_state, old_plane_state, true, crtc->state->pflip_flags) || action == DM_COMMIT_ACTION_DPMS_ON || action == DM_COMMIT_ACTION_SET) {

	list_for_each_entry(connector,

@@ -2760,21 +2686,22 @@ 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;

• uint32_t pflip_flags;
  
  

  if (!fb || !crtc || !crtc->state->planes_changed || !crtc->state->active) continue;

• if (page_flip_needed(plane_state, old_plane_state, false)) {
  

• pflip_flags = crtc->state->pflip_flags;
  

• if (page_flip_needed(
  

• 		plane_state, old_plane_state, false, pflip_flags)) {
  

Indentation again.

	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;
  

• 					   crtc->state->pflip_flags,
  

• 					   crtc->state->target_vblank);
  

• if (ret)
  	break;
  

  }

@@ -3138,13 +3065,15 @@ int amdgpu_dm_atomic_check(struct drm_device *dev, continue;

	action = get_dm_commit_action(crtc->state);

• 	crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
  
  /* Surfaces are created under two scenarios:
   * 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, crtc_state->pflip_flags)
  

• 			||
  

And here as well.

Otherwise patch looks good.

Harry

			action == DM_COMMIT_ACTION_DPMS_ON ||
			action == DM_COMMIT_ACTION_SET) {
		struct dc_surface *surface;