[PATCH 14/17] drm/atomic-helper: implement ->page_flip
On Thu, Nov 06, 2014 at 12:43:34PM -0500, Sean Paul wrote:
> On Sun, Nov 02, 2014 at 02:19:27PM +0100, Daniel Vetter wrote:
> > + 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);
> > + if (ret == -EDEADLK)
> > + goto backoff;
> > + else
> > + goto fail;
>
> This function would benefit from the error cleanup you did in the previous
> patch (ie, doing the -EDEADLK check in fail and punting to backoff there).
v6 in in this subthread has that already. Can you please check that this
one's good?
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
[Intel-gfx] [PATCH 14/17] drm/atomic-helper: implement ->page_flip
On Thu, Nov 6, 2014 at 1:13 PM, Daniel Vetter wrote:
>
> On Thu, Nov 06, 2014 at 12:43:34PM -0500, Sean Paul wrote:
> > On Sun, Nov 02, 2014 at 02:19:27PM +0100, Daniel Vetter wrote:
> > > + 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);
> > > + if (ret == -EDEADLK)
> > > + goto backoff;
> > > + else
> > > + goto fail;
> >
> > This function would benefit from the error cleanup you did in the previous
> > patch (ie, doing the -EDEADLK check in fail and punting to backoff there).
>
> v6 in in this subthread has that already. Can you please check that this
> one's good?
>
Yep, looks good.
Sean
>
> Thanks, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[PATCH 14/17] drm/atomic-helper: implement ->page_flip
On Sun, Nov 02, 2014 at 02:19:27PM +0100, Daniel Vetter wrote:
> Currently there is no way to implement async flips using atomic, that
> essentially requires us to be able to cancel pending requests
> mid-flight.
>
> To be able to do that (and I guess we want this since vblank synced
> updates whic opportunistically cancel still pending updates seem to be
s/whic/which/
> wanted) we'd need to add a mandatory cancellation mode. Depending upon
> the exact semantics we decide upon that could mean that userspace will
> not get completion events, or will get them all stacked up.
>
> So reject async updates for now. Also async updates usually means not
> vblank synced at all, and I guess for drivers which want to support
> this they should simply add a special pageflip handler (since usually
> you need a special flip cmd to achieve this). That kind of async flip
> is pretty much exclusively just used for games and benchmarks where
> dropping just one frame means you'll get a headshot or something bad
> like that ... And so slight amounts of tearing is acceptable.
>
> v2: Fixup kerneldoc, reported by Paulo.
>
> v3: Use the set_crtc_for_plane function to assign the crtc, since
> otherwise the book-keeping is off.
>
> v4: Update crtc->primary->fb since ->page_flip is the only driver
> callback where the core won't do this itself. We might want to fix
> this inconsistency eventually.
>
> Cc: Paulo Zanoni
> Signed-off-by: Daniel Vetter
Minor nit regarding error handing, either way:
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 90
> +
> include/drm/drm_atomic_helper.h | 5 +++
> 2 files changed, 95 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 92ae34bde44d..70bd67cf86e3 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1617,3 +1617,93 @@ backoff:
> goto retry;
> }
> EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
> +
> +/**
> + * 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 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(struct drm_crtc *crtc,
> + struct drm_framebuffer *fb,
> + struct drm_pending_vblank_event *event,
> + uint32_t flags)
> +{
> + struct drm_plane *plane = crtc->primary;
> + struct drm_atomic_state *state;
> + struct drm_plane_state *plane_state;
> + struct drm_crtc_state *crtc_state;
> + int ret = 0;
> +
> + 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:
> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> + if (IS_ERR(crtc_state)) {
> + ret = PTR_ERR(crtc_state);
> + if (ret == -EDEADLK)
> + goto backoff;
> + else
> + goto fail;
This function would benefit from the error cleanup you did in the previous
patch (ie, doing the -EDEADLK check in fail and punting to backoff there).
> + }
> + crtc_state->event = event;
> +
> + plane_state = drm_atomic_get_plane_state(state, plane);
> + if (IS_ERR(plane_state)) {
> + ret = PTR_ERR(plane_state);
> + if (ret == -EDEADLK)
> + goto backoff;
> + else
> + goto fail;
> + }
> +
> + drm_atomic_set_crtc_for_plane(plane_state, crtc);
> + plane_state->fb = fb;
> +
> + ret = drm_atomic_async_commit(state);
> + if (ret == -EDEADLK)
> + goto backoff;
> +
> + /* Driver takes ownership of state on successful async commit. */
> + if (ret == 0) {
> + /* TODO: ->page_flip is the only driver callback where the core
> + * doesn't update plane->fb. For now patch it up here. */
> + plane->fb = plane->state->fb;
> +
> + return 0;
> + }
> +
> +fail:
> + drm_atomic_state_free(state);
> +
> + return ret;
> +backoff:
> + drm_atomic_legacy_backoff(state);
> + drm_atomic_state_clear(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);
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 8cd6fe7a48e5..28a2f3a815fd 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -69,5 +69,10 @@ int drm_atomic_helper_plane_set_property(struct drm_plane
>
[PATCH 14/17] drm/atomic-helper: implement ->page_flip
Currently there is no way to implement async flips using atomic, that
essentially requires us to be able to cancel pending requests
mid-flight.
To be able to do that (and I guess we want this since vblank synced
updates whic opportunistically cancel still pending updates seem to be
wanted) we'd need to add a mandatory cancellation mode. Depending upon
the exact semantics we decide upon that could mean that userspace will
not get completion events, or will get them all stacked up.
So reject async updates for now. Also async updates usually means not
vblank synced at all, and I guess for drivers which want to support
this they should simply add a special pageflip handler (since usually
you need a special flip cmd to achieve this). That kind of async flip
is pretty much exclusively just used for games and benchmarks where
dropping just one frame means you'll get a headshot or something bad
like that ... And so slight amounts of tearing is acceptable.
v2: Fixup kerneldoc, reported by Paulo.
v3: Use the set_crtc_for_plane function to assign the crtc, since
otherwise the book-keeping is off.
v4: Update crtc->primary->fb since ->page_flip is the only driver
callback where the core won't do this itself. We might want to fix
this inconsistency eventually.
Cc: Paulo Zanoni
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_atomic_helper.c | 90 +
include/drm/drm_atomic_helper.h | 5 +++
2 files changed, 95 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 92ae34bde44d..70bd67cf86e3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1617,3 +1617,93 @@ backoff:
goto retry;
}
EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
+
+/**
+ * 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 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(struct drm_crtc *crtc,
+ struct drm_framebuffer *fb,
+ struct drm_pending_vblank_event *event,
+ uint32_t flags)
+{
+ struct drm_plane *plane = crtc->primary;
+ struct drm_atomic_state *state;
+ struct drm_plane_state *plane_state;
+ struct drm_crtc_state *crtc_state;
+ int ret = 0;
+
+ 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:
+ crtc_state = drm_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(crtc_state)) {
+ ret = PTR_ERR(crtc_state);
+ if (ret == -EDEADLK)
+ goto backoff;
+ else
+ 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);
+ if (ret == -EDEADLK)
+ goto backoff;
+ else
+ goto fail;
+ }
+
+ drm_atomic_set_crtc_for_plane(plane_state, crtc);
+ plane_state->fb = fb;
+
+ ret = drm_atomic_async_commit(state);
+ if (ret == -EDEADLK)
+ goto backoff;
+
+ /* Driver takes ownership of state on successful async commit. */
+ if (ret == 0) {
+ /* TODO: ->page_flip is the only driver callback where the core
+* doesn't update plane->fb. For now patch it up here. */
+ plane->fb = plane->state->fb;
+
+ return 0;
+ }
+
+fail:
+ drm_atomic_state_free(state);
+
+ return ret;
+backoff:
+ drm_atomic_legacy_backoff(state);
+ drm_atomic_state_clear(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);
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 8cd6fe7a48e5..28a2f3a815fd 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -69,5 +69,10 @@ int drm_atomic_helper_plane_set_property(struct drm_plane
*plane,
int
