[PATCH 08/17] drm/plane-helper: transitional atomic plane helpers
On Wed, Nov 5, 2014 at 5:45 PM, Sean Paul wrote:
>> + /* There's no other way to figure out whether the crtc is running. */
>> + ret = drm_crtc_vblank_get(crtc[i]);
>> + if (ret == 0) {
>> + drm_crtc_wait_one_vblank(crtc[i]);
>> + drm_crtc_vblank_put(crtc[i]);
>> + }
>
> This will be good motivation for driver writers to convert to universal
> planes!
The problem here is actually dpms. Our current dpms interface sucks
badly, since from generic code it's completely impossible to figure
out whether the pipe is still on or somehow disabled due to dpms. I'm
pondering ideas and designs right now, but my current dpms plan for
atomic includes a fix for this. So with that I'd replace this hack
with crtc->state->active check and add a WARN_ON to make sure the
drm_vblank_get doesn't fail when the pipe is supposed to be on.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH 08/17] drm/plane-helper: transitional atomic plane helpers
On Sun, Nov 02, 2014 at 02:19:21PM +0100, Daniel Vetter wrote:
> Converting a driver to the atomic interface can be a daunting
> undertaking. One of the prerequisites is to have full universal planes
> support.
>
> To make that transition a bit easier this pathc provides plane helpers
s/pathc/patch/
> which use the new atomic helper callbacks just only for the plane
> changes. This way the plane update functionality can be tested without
> being forced to convert everything at once.
>
> Of course a real atomic update capable driver will implement the
> all plane properties through the atomic interface, so these helpers
> are mostly transitional. But they can be used to enable proper
> universal plane support, especially once the crtc helpers have also
> been adapted.
>
> v2: Use ->atomic_duplicate_state if available.
>
> v3: Don't forget to call ->atomic_destroy_state if available.
>
> v4: Fixup kerneldoc, reported by Paulo.
>
> v5: Extract a common plane_commit helper and fix some bugs in the
> plane_state setup of the plane_disable implementation.
>
> v6: Fix issues with the cleanup of the old fb. Since transitional
> helpers can be mixed we need to assume that the old fb has been set up
> by a legacy path (e.g. set_config or page_flip when the primary plane
> is converted to use these functions already). Hence pass an additional
> old_fb parameter to plane_commit to do that cleanup work correctly.
>
> v7:
> - Fix spurious WARNING (crtc helpers really love to disable stuff
> harder) and fix array index bonghits.
> - Correctly handle the lack of plane->state object, necessary for
> transitional use.
> - Don't indicate failure if drm_vblank_get doesn't work - that's
> expected when the pipe is in dpms off mode.
>
> Cc: Paulo Zanoni
> Signed-off-by: Daniel Vetter
Just a few nits to prove that I read the whole thing ;-).
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_plane_helper.c | 172
> -
> include/drm/drm_plane_helper.h | 8 ++
> 2 files changed, 179 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> b/drivers/gpu/drm/drm_plane_helper.c
> index 827ec1a3040b..45aa8c98e3fb 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -27,7 +27,7 @@
> #include
> #include
> #include
> -#include
> +#include
>
> #define SUBPIXEL_MASK 0x
>
> @@ -369,3 +369,173 @@ int drm_crtc_init(struct drm_device *dev, struct
> drm_crtc *crtc,
> return drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs);
> }
> EXPORT_SYMBOL(drm_crtc_init);
> +
> +static int
> +plane_commit(struct drm_plane *plane, struct drm_plane_state *plane_state,
> + struct drm_framebuffer *old_fb)
> +{
> + struct drm_plane_helper_funcs *plane_funcs;
> + struct drm_crtc *crtc[2];
> + struct drm_crtc_helper_funcs *crtc_funcs[2];
> + int i, ret = 0;
> +
> + plane_funcs = plane->helper_private;
> +
> + /* Since this is a transitional helper we can't assume that plane->state
> + * is always valid. Hence we need to use plane->crtc instead of
> + * plane->state->crtc as the old crtc. */
> + crtc[0] = plane->crtc;
> + crtc[1] = crtc[0] != plane_state->crtc ? plane_state->crtc : NULL;
> +
> + for (i = 0; i < 2; i++)
> + crtc_funcs[i] = crtc[i] ? crtc[i]->helper_private : NULL;
> +
> + if (plane_funcs->atomic_check) {
> + ret = plane_funcs->atomic_check(plane, plane_state);
> + if (ret)
> + goto fail;
> + }
> +
> + if (plane_funcs->prepare_fb && plane_state->fb) {
> + ret = plane_funcs->prepare_fb(plane, plane_state->fb);
> + if (ret)
> + goto fail;
> + }
> +
> + /* Point of no return, commit sw state. */
> + swap(plane->state, plane_state);
> +
> + for (i = 0; i < 2; i++) {
> + if (crtc_funcs[i] && crtc_funcs[i]->atomic_begin)
> + crtc_funcs[i]->atomic_begin(crtc[i]);
> + }
> +
> + plane_funcs->atomic_update(plane);
> +
> + for (i = 0; i < 2; i++) {
> + if (crtc_funcs[i] && crtc_funcs[i]->atomic_flush)
> + crtc_funcs[i]->atomic_flush(crtc[i]);
> + }
> +
> + for (i = 0; i < 2; i++) {
> + if (!crtc[i])
> + continue;
> +
> + /* There's no other way to figure out whether the crtc is running. */
> + ret = drm_crtc_vblank_get(crtc[i]);
> + if (ret == 0) {
> + drm_crtc_wait_one_vblank(crtc[i]);
> + drm_crtc_vblank_put(crtc[i]);
> + }
This will be good motivation for driver writers to convert to universal
planes!
> +
> + ret = 0;
> + }
> +
> + if (plane_funcs->cleanup_fb && old_fb)
> + plane_funcs->cleanup_fb(plane, old_fb);
> +fail:
minor nit: Might be better to rename this "out", so it's clear that it's
intended to run even in the successful case.
> + if (plane_state) {
> + if (plane->funcs->atomic_destroy_state)
> + plane->funcs->atomic_destroy_state(plane, plane_state);
> + else
> + kfree(plane_state);
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * drm_plane_helper_update() - Helper for primary plane update
> + * @plane: plane object to update
> + * @crtc: owning CRTC of owning plane
> + * @fb:
[PATCH 08/17] drm/plane-helper: transitional atomic plane helpers
Converting a driver to the atomic interface can be a daunting
undertaking. One of the prerequisites is to have full universal planes
support.
To make that transition a bit easier this pathc provides plane helpers
which use the new atomic helper callbacks just only for the plane
changes. This way the plane update functionality can be tested without
being forced to convert everything at once.
Of course a real atomic update capable driver will implement the
all plane properties through the atomic interface, so these helpers
are mostly transitional. But they can be used to enable proper
universal plane support, especially once the crtc helpers have also
been adapted.
v2: Use ->atomic_duplicate_state if available.
v3: Don't forget to call ->atomic_destroy_state if available.
v4: Fixup kerneldoc, reported by Paulo.
v5: Extract a common plane_commit helper and fix some bugs in the
plane_state setup of the plane_disable implementation.
v6: Fix issues with the cleanup of the old fb. Since transitional
helpers can be mixed we need to assume that the old fb has been set up
by a legacy path (e.g. set_config or page_flip when the primary plane
is converted to use these functions already). Hence pass an additional
old_fb parameter to plane_commit to do that cleanup work correctly.
v7:
- Fix spurious WARNING (crtc helpers really love to disable stuff
harder) and fix array index bonghits.
- Correctly handle the lack of plane->state object, necessary for
transitional use.
- Don't indicate failure if drm_vblank_get doesn't work - that's
expected when the pipe is in dpms off mode.
Cc: Paulo Zanoni
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_plane_helper.c | 172 -
include/drm/drm_plane_helper.h | 8 ++
2 files changed, 179 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_plane_helper.c
b/drivers/gpu/drm/drm_plane_helper.c
index 827ec1a3040b..45aa8c98e3fb 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -27,7 +27,7 @@
#include
#include
#include
-#include
+#include
#define SUBPIXEL_MASK 0x
@@ -369,3 +369,173 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc
*crtc,
return drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs);
}
EXPORT_SYMBOL(drm_crtc_init);
+
+static int
+plane_commit(struct drm_plane *plane, struct drm_plane_state *plane_state,
+struct drm_framebuffer *old_fb)
+{
+ struct drm_plane_helper_funcs *plane_funcs;
+ struct drm_crtc *crtc[2];
+ struct drm_crtc_helper_funcs *crtc_funcs[2];
+ int i, ret = 0;
+
+ plane_funcs = plane->helper_private;
+
+ /* Since this is a transitional helper we can't assume that plane->state
+* is always valid. Hence we need to use plane->crtc instead of
+* plane->state->crtc as the old crtc. */
+ crtc[0] = plane->crtc;
+ crtc[1] = crtc[0] != plane_state->crtc ? plane_state->crtc : NULL;
+
+ for (i = 0; i < 2; i++)
+ crtc_funcs[i] = crtc[i] ? crtc[i]->helper_private : NULL;
+
+ if (plane_funcs->atomic_check) {
+ ret = plane_funcs->atomic_check(plane, plane_state);
+ if (ret)
+ goto fail;
+ }
+
+ if (plane_funcs->prepare_fb && plane_state->fb) {
+ ret = plane_funcs->prepare_fb(plane, plane_state->fb);
+ if (ret)
+ goto fail;
+ }
+
+ /* Point of no return, commit sw state. */
+ swap(plane->state, plane_state);
+
+ for (i = 0; i < 2; i++) {
+ if (crtc_funcs[i] && crtc_funcs[i]->atomic_begin)
+ crtc_funcs[i]->atomic_begin(crtc[i]);
+ }
+
+ plane_funcs->atomic_update(plane);
+
+ for (i = 0; i < 2; i++) {
+ if (crtc_funcs[i] && crtc_funcs[i]->atomic_flush)
+ crtc_funcs[i]->atomic_flush(crtc[i]);
+ }
+
+ for (i = 0; i < 2; i++) {
+ if (!crtc[i])
+ continue;
+
+ /* There's no other way to figure out whether the crtc is
running. */
+ ret = drm_crtc_vblank_get(crtc[i]);
+ if (ret == 0) {
+ drm_crtc_wait_one_vblank(crtc[i]);
+ drm_crtc_vblank_put(crtc[i]);
+ }
+
+ ret = 0;
+ }
+
+ if (plane_funcs->cleanup_fb && old_fb)
+ plane_funcs->cleanup_fb(plane, old_fb);
+fail:
+ if (plane_state) {
+ if (plane->funcs->atomic_destroy_state)
+ plane->funcs->atomic_destroy_state(plane, plane_state);
+ else
+ kfree(plane_state);
+ }
+
+ return ret;
+}
+
+/**
+ * drm_plane_helper_update() - Helper for primary plane update
+ * @plane: plane object to update
+ * @crtc: owning CRTC of owning plane
+ * @fb: framebuffer to flip onto plane
+ * @crtc_x: x
