[Intel-gfx] [PATCH 08/17] drm/plane-helper: transitional atomic plane helpers

[Sean Paul]

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 <przanoni at gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>

Just a few nits to prove that I read the whole thing ;-).

Reviewed-by: Sean Paul <seanpaul at chromium.org>


> ---
>  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 <drm/drmP.h>
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_rect.h>
> -#include <drm/drm_plane_helper.h>
> +#include <drm/drm_crtc_helper.h>
>
>  #define SUBPIXEL_MASK 0xffff
>
> @@ -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: framebuffer to flip onto plane
> + * @crtc_x: x offset of primary plane on crtc
> + * @crtc_y: y offset of primary plane on crtc
> + * @crtc_w: width of primary plane rectangle on crtc
> + * @crtc_h: height of primary plane rectangle on crtc
> + * @src_x: x offset of @fb for panning
> + * @src_y: y offset of @fb for panning
> + * @src_w: width of source rectangle in @fb
> + * @src_h: height of source rectangle in @fb
> + *
> + * Provides a default plane update handler using the atomic plane update
> + * functions. It is fully left to the driver to check plane constraints and
> + * handle corner-cases like a fully occluded or otherwise invisible plane.
> + *
> + * This is useful for piecewise transitioning of a driver to the atomic helpers.
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
> +    struct drm_framebuffer *fb,
> +    int crtc_x, int crtc_y,
> +    unsigned int crtc_w, unsigned int crtc_h,
> +    uint32_t src_x, uint32_t src_y,
> +    uint32_t src_w, uint32_t src_h)
> +{
> + struct drm_plane_state *plane_state;
> +
> + if (plane->funcs->atomic_duplicate_state)
> + plane_state = plane->funcs->atomic_duplicate_state(plane);
> + else if (plane->state)
> + plane_state = kmemdup(plane->state, sizeof(*plane_state),
> +      GFP_KERNEL);
> + else
> + plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
> + if (!plane_state)
> + return -ENOMEM;
> +
> + plane_state->crtc = crtc;
> + plane_state->fb = fb;
> + plane_state->crtc_x = crtc_x;
> + plane_state->crtc_y = crtc_y;
> + plane_state->crtc_h = crtc_h;
> + plane_state->crtc_w = crtc_w;
> + plane_state->src_x = src_x;
> + plane_state->src_y = src_y;
> + plane_state->src_h = src_h;
> + plane_state->src_w = src_w;
> +
> + return plane_commit(plane, plane_state, plane->fb);
> +}
> +EXPORT_SYMBOL(drm_plane_helper_update);
> +
> +/**
> + * drm_plane_helper_disable() - Helper for primary plane disable
> + * @plane: plane to disable
> + *
> + * Provides a default plane disable handler using the atomic plane update
> + * functions. It is fully left to the driver to check plane constraints and
> + * handle corner-cases like a fully occluded or otherwise invisible plane.
> + *
> + * This is useful for piecewise transitioning of a driver to the atomic helpers.
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_plane_helper_disable(struct drm_plane *plane)
> +{
> + struct drm_plane_state *plane_state;
> +
> + /* crtc helpers love to call disable functions for already disabled hw
> + * functions. So cope with that. */
> + if (!plane->crtc)
> + return 0;
> +
> + if (plane->funcs->atomic_duplicate_state)
> + plane_state = plane->funcs->atomic_duplicate_state(plane);
> + else if (plane->state)
> + plane_state = kmemdup(plane->state, sizeof(*plane_state),
> +      GFP_KERNEL);
> + else
> + plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
> + if (!plane_state)
> + return -ENOMEM;
> +
> + plane_state->crtc = NULL;
> + plane_state->fb = NULL;
> +
> + return plane_commit(plane, plane_state, plane->fb);
> +}
> +EXPORT_SYMBOL(drm_plane_helper_disable);
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index c2146a7d91fb..8a0709704af0 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -94,4 +94,12 @@ extern struct drm_plane *drm_primary_helper_create_plane(struct drm_device *dev,
>   int num_formats);
>
>
> +int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
> +    struct drm_framebuffer *fb,
> +    int crtc_x, int crtc_y,
> +    unsigned int crtc_w, unsigned int crtc_h,
> +    uint32_t src_x, uint32_t src_y,
> +    uint32_t src_w, uint32_t src_h);
> +int drm_plane_helper_disable(struct drm_plane *plane);
> +
>  #endif
> --
> 2.1.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel