Re: [PATCH 07/17] drm: Add atomic/plane helpers

[Sean Paul <seanpaul@chromium.org>]

On Sun, Nov 02, 2014 at 02:19:20PM +0100, Daniel Vetter wrote:
> This is the first cut of atomic helper code. As-is it's only useful to
> implement a pure atomic interface for plane updates.
>
> Later patches will integrate this with the crtc helpers so that full
> atomic updates are possible. We also need a pile of helpers to aid
> drivers in transitioning from the legacy world to the shiny new atomic
> age. Finally we need helpers to implement legacy ioctls on top of the
> atomic interface.
>
> The design of the overall helpers<->driver interaction is fairly
> simple, but has an unfortunate large interface:
>
> - We have ->atomic_check callbacks for crtcs and planes. The idea is
>   that connectors don't need any checking, and if they do they can
>   adjust the relevant crtc driver-private state. So no connector hooks
>   should be needed. Also the crtc helpers integration will do the
>   ->best_encoder checks, so no need for that.
>
> - Framebuffer pinning needs to be done before we can commit to the hw
>   state. This is especially important for async updates where we must
>   pin all buffers before returning to userspace, so that really only
>   hw failures can happen in the asynchronous worker.
>
>   Hence we add ->prepare_fb and ->cleanup_fb hooks for this resources
>   management.
>
> - The actual atomic plane commit can't fail (except hw woes), so has
>   void return type. It has three stages:
>   1. Prepare all affected crtcs with crtc->atomic_begin. Drivers can
>      use this to unset the GO bit or similar latches to prevent plane
>      updates.
>   2. Update plane state by looping over all changed planes and calling
>      plane->atomic_update. Presuming the hardware is sane and has GO
>      bits drivers can simply bash the state into the hardware in this
>      function. Other drivers might use this to precompute hw state for
>      the final step.
>   3. Finally latch the update for the next vblank with
>      crtc->atomic_flush. Note that this function doesn't need to wait
>      for the vblank to happen even for the synchronous case.
>
> v2: Clear drm_<obj>_state->state to NULL when swapping in state.
>
> v3: Add TODO that we don't short-circuit plane updates for now. Likely
> no one will care.
>
> v4: Squash in a bit of polish that somehow landed in the wrong (later)
> patche.
>
> v5: Integrate atomic functions into the drm docbook and fixup the
> kerneldoc.
>
> v6: Fixup fixup patch squashing fumble.
>
> v7: Don't touch the legacy plane state plane->fb and plane->crtc. This
> is only used by the legacy ioctl code in the drm core, and that code
> already takes care of updating the pointers in all relevant cases.
> This is in stark contrast to connector->encoder->crtc links on the
> modeset side, which we still need to set since the core doesn't touch
> them.
>
> Also some more kerneldoc polish.
>
> v8: Drop outdated comment.
>
> v9: Handle the state->state pointer correctly: Only clearing the
> ->state pointer when assigning the state to the kms object isn't good
> enough. We also need to re-link the swapped out state into the
> drm_atomic_state structure.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>


Minor nit re: docbook. Otherwise,

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

> ---
>  Documentation/DocBook/drm.tmpl      |   4 +
>  drivers/gpu/drm/Makefile            |   2 +-
>  drivers/gpu/drm/drm_atomic_helper.c | 355 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |  44 +++++
>  include/drm/drm_crtc.h              |   6 +
>  include/drm/drm_crtc_helper.h       |   6 +
>  include/drm/drm_plane_helper.h      |  22 +++
>  7 files changed, 438 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_atomic_helper.c
>  create mode 100644 include/drm/drm_atomic_helper.h
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 89829ae58e97..ea0ef43b19e1 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -996,6 +996,10 @@ int max_width, max_height;</synopsis>
>  !Edrivers/gpu/drm/drm_modes.c
>      </sect2>
>      <sect2>
> +      <title>Atomic Mode Setting Function Reference</title>
> +!Edrivers/gpu/drm/drm_atomic.c
> +    </sect2>
> +    <sect2>

This change should probably be in the previous patch.


>        <title>Frame Buffer Creation</title>
>        <synopsis>struct drm_framebuffer *(*fb_create)(struct drm_device *dev,
>       struct drm_file *file_priv,
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 2e89cd50c14f..96338e349a24 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -24,7 +24,7 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
>  drm-$(CONFIG_OF) += drm_of.o
>
>  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> - drm_plane_helper.o drm_dp_mst_topology.o
> + drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o
>  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  drm_kms_helper-$(CONFIG_DRM_KMS_FB_HELPER) += drm_fb_helper.o
>  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> new file mode 100644
> index 000000000000..55a8eb2678b0
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -0,0 +1,355 @@
> +/*
> + * Copyright (C) 2014 Red Hat
> + * Copyright (C) 2014 Intel Corp.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + * Rob Clark <robdclark@gmail.com>
> + * Daniel Vetter <daniel.vetter@ffwll.ch>
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +
> +static void
> +drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
> + struct drm_plane_state *plane_state,
> + struct drm_plane *plane)
> +{
> + struct drm_crtc_state *crtc_state;
> +
> + if (plane->state->crtc) {
> + crtc_state = state->crtc_states[drm_crtc_index(plane->crtc)];
> +
> + if (WARN_ON(!crtc_state))
> + return;
> +
> + crtc_state->planes_changed = true;
> + }
> +
> + if (plane_state->crtc) {
> + crtc_state =
> + state->crtc_states[drm_crtc_index(plane_state->crtc)];
> +
> + if (WARN_ON(!crtc_state))
> + return;
> +
> + crtc_state->planes_changed = true;
> + }
> +}
> +
> +/**
> + * drm_atomic_helper_check - validate state object
> + * @dev: DRM device
> + * @state: the driver state object
> + *
> + * Check the state object to see if the requested state is physically possible.
> + * Only crtcs and planes have check callbacks, so for any additional (global)
> + * checking that a driver needs it can simply wrap that around this function.
> + * Drivers without such needs can directly use this as their ->atomic_check()
> + * callback.
> + *
> + * RETURNS
> + * Zero for success or -errno
> + */
> +int drm_atomic_helper_check(struct drm_device *dev,
> +    struct drm_atomic_state *state)
> +{
> + int nplanes = dev->mode_config.num_total_plane;
> + int ncrtcs = dev->mode_config.num_crtc;
> + int i, ret = 0;
> +
> + for (i = 0; i < nplanes; i++) {
> + struct drm_plane_helper_funcs *funcs;
> + struct drm_plane *plane = state->planes[i];
> + struct drm_plane_state *plane_state = state->plane_states[i];
> +
> + if (!plane)
> + continue;
> +
> + funcs = plane->helper_private;
> +
> + drm_atomic_helper_plane_changed(state, plane_state, plane);
> +
> + if (!funcs || !funcs->atomic_check)
> + continue;
> +
> + ret = funcs->atomic_check(plane, plane_state);
> + if (ret) {
> + DRM_DEBUG_KMS("[PLANE:%d] atomic check failed\n",
> +      plane->base.id);
> + return ret;
> + }
> + }
> +
> + for (i = 0; i < ncrtcs; i++) {
> + struct drm_crtc_helper_funcs *funcs;
> + struct drm_crtc *crtc = state->crtcs[i];
> +
> + if (!crtc)
> + continue;
> +
> + funcs = crtc->helper_private;
> +
> + if (!funcs || !funcs->atomic_check)
> + continue;
> +
> + ret = funcs->atomic_check(crtc, state->crtc_states[i]);
> + if (ret) {
> + DRM_DEBUG_KMS("[CRTC:%d] atomic check failed\n",
> +      crtc->base.id);
> + return ret;
> + }
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_check);
> +
> +/**
> + * drm_atomic_helper_prepare_planes - prepare plane resources after commit
> + * @dev: DRM device
> + * @state: atomic state object with old state structures
> + *
> + * This function prepares plane state, specifically framebuffers, for the new
> + * configuration. If any failure is encountered this function will call
> + * ->cleanup_fb on any already successfully prepared framebuffer.
> + *
> + * Returns:
> + * 0 on success, negative error code on failure.
> + */
> +int drm_atomic_helper_prepare_planes(struct drm_device *dev,
> +     struct drm_atomic_state *state)
> +{
> + int nplanes = dev->mode_config.num_total_plane;
> + int ret, i;
> +
> + for (i = 0; i < nplanes; i++) {
> + struct drm_plane_helper_funcs *funcs;
> + struct drm_plane *plane = state->planes[i];
> + struct drm_framebuffer *fb;
> +
> + if (!plane)
> + continue;
> +
> + funcs = plane->helper_private;
> +
> + fb = state->plane_states[i]->fb;
> +
> + if (fb && funcs->prepare_fb) {
> + ret = funcs->prepare_fb(plane, fb);
> + if (ret)
> + goto fail;
> + }
> + }
> +
> + return 0;
> +
> +fail:
> + for (i--; i >= 0; i--) {
> + struct drm_plane_helper_funcs *funcs;
> + struct drm_plane *plane = state->planes[i];
> + struct drm_framebuffer *fb;
> +
> + if (!plane)
> + continue;
> +
> + funcs = plane->helper_private;
> +
> + fb = state->plane_states[i]->fb;
> +
> + if (fb && funcs->cleanup_fb)
> + funcs->cleanup_fb(plane, fb);
> +
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
> +
> +/**
> + * drm_atomic_helper_commit_planes - commit plane state
> + * @dev: DRM device
> + * @state: atomic state
> + *
> + * This function commits the new plane state using the plane and atomic helper
> + * functions for planes and crtcs. It assumes that the atomic state has already
> + * been pushed into the relevant object state pointers, since this step can no
> + * longer fail.
> + *
> + * It still requires the global state object @state to know which planes and
> + * crtcs need to be updated though.
> + */
> +void drm_atomic_helper_commit_planes(struct drm_device *dev,
> +     struct drm_atomic_state *state)
> +{
> + int nplanes = dev->mode_config.num_total_plane;
> + int ncrtcs = dev->mode_config.num_crtc;
> + int i;
> +
> + for (i = 0; i < ncrtcs; i++) {
> + struct drm_crtc_helper_funcs *funcs;
> + struct drm_crtc *crtc = state->crtcs[i];
> +
> + if (!crtc)
> + continue;
> +
> + funcs = crtc->helper_private;
> +
> + if (!funcs || !funcs->atomic_begin)
> + continue;
> +
> + funcs->atomic_begin(crtc);
> + }
> +
> + for (i = 0; i < nplanes; i++) {
> + struct drm_plane_helper_funcs *funcs;
> + struct drm_plane *plane = state->planes[i];
> +
> + if (!plane)
> + continue;
> +
> + funcs = plane->helper_private;
> +
> + if (!funcs || !funcs->atomic_update)
> + continue;
> +
> + funcs->atomic_update(plane);
> + }
> +
> + for (i = 0; i < ncrtcs; i++) {
> + struct drm_crtc_helper_funcs *funcs;
> + struct drm_crtc *crtc = state->crtcs[i];
> +
> + if (!crtc)
> + continue;
> +
> + funcs = crtc->helper_private;
> +
> + if (!funcs || !funcs->atomic_flush)
> + continue;
> +
> + funcs->atomic_flush(crtc);
> + }
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_commit_planes);
> +
> +/**
> + * drm_atomic_helper_cleanup_planes - cleanup plane resources after commit
> + * @dev: DRM device
> + * @old_state: atomic state object with old state structures
> + *
> + * This function cleans up plane state, specifically framebuffers, from the old
> + * configuration. Hence the old configuration must be perserved in @old_state to
> + * be able to call this function.
> + *
> + * This function must also be called on the new state when the atomic update
> + * fails at any point after calling drm_atomic_helper_prepare_planes().
> + */
> +void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
> +      struct drm_atomic_state *old_state)
> +{
> + int nplanes = dev->mode_config.num_total_plane;
> + int i;
> +
> + for (i = 0; i < nplanes; i++) {
> + struct drm_plane_helper_funcs *funcs;
> + struct drm_plane *plane = old_state->planes[i];
> + struct drm_framebuffer *old_fb;
> +
> + if (!plane)
> + continue;
> +
> + funcs = plane->helper_private;
> +
> + old_fb = old_state->plane_states[i]->fb;
> +
> + if (old_fb && funcs->cleanup_fb)
> + funcs->cleanup_fb(plane, old_fb);
> + }
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
> +
> +/**
> + * drm_atomic_helper_swap_state - store atomic state into current sw state
> + * @dev: DRM device
> + * @state: atomic state
> + *
> + * This function stores the atomic state into the current state pointers in all
> + * driver objects. It should be called after all failing steps have been done
> + * and succeeded, but before the actual hardware state is committed.
> + *
> + * For cleanup and error recovery the current state for all changed objects will
> + * be swaped into @state.
> + *
> + * With that sequence it fits perfectly into the plane prepare/cleanup sequence:
> + *
> + * 1. Call drm_atomic_helper_prepare_planes() with the staged atomic state.
> + *
> + * 2. Do any other steps that might fail.
> + *
> + * 3. Put the staged state into the current state pointers with this function.
> + *
> + * 4. Actually commit the hardware state.
> + *
> + * 5. Call drm_atomic_helper_cleanup_planes with @state, which since step 3
> + * contains the old state. Also do any other cleanup required with that state.
> + */
> +void drm_atomic_helper_swap_state(struct drm_device *dev,
> +  struct drm_atomic_state *state)
> +{
> + int i;
> +
> + for (i = 0; i < dev->mode_config.num_connector; i++) {
> + struct drm_connector *connector = state->connectors[i];
> +
> + if (!connector)
> + continue;
> +
> + connector->state->state = state;
> + swap(state->connector_states[i], connector->state);
> + connector->state->state = NULL;
> + }
> +
> + for (i = 0; i < dev->mode_config.num_crtc; i++) {
> + struct drm_crtc *crtc = state->crtcs[i];
> +
> + if (!crtc)
> + continue;
> +
> + crtc->state->state = state;
> + swap(state->crtc_states[i], crtc->state);
> + crtc->state->state = NULL;
> + }
> +
> + for (i = 0; i < dev->mode_config.num_total_plane; i++) {
> + struct drm_plane *plane = state->planes[i];
> +
> + if (!plane)
> + continue;
> +
> + plane->state->state = state;
> + swap(state->plane_states[i], plane->state);
> + plane->state->state = NULL;
> + }
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_swap_state);
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> new file mode 100644
> index 000000000000..79938c62e7ad
> --- /dev/null
> +++ b/include/drm/drm_atomic_helper.h
> @@ -0,0 +1,44 @@
> +/*
> + * Copyright (C) 2014 Red Hat
> + * Copyright (C) 2014 Intel Corp.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + * Rob Clark <robdclark@gmail.com>
> + * Daniel Vetter <daniel.vetter@ffwll.ch>
> + */
> +
> +#ifndef DRM_ATOMIC_HELPER_H_
> +#define DRM_ATOMIC_HELPER_H_
> +
> +int drm_atomic_helper_check(struct drm_device *dev,
> +    struct drm_atomic_state *state);
> +
> +int drm_atomic_helper_prepare_planes(struct drm_device *dev,
> +     struct drm_atomic_state *state);
> +void drm_atomic_helper_commit_planes(struct drm_device *dev,
> +     struct drm_atomic_state *state);
> +void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
> +      struct drm_atomic_state *old_state);
> +
> +void drm_atomic_helper_swap_state(struct drm_device *dev,
> +  struct drm_atomic_state *state);
> +
> +#endif /* DRM_ATOMIC_HELPER_H_ */
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c3ce5b36da5f..d0068b7af678 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -229,6 +229,7 @@ struct drm_atomic_state;
>  /**
>   * struct drm_crtc_state - mutable crtc state
>   * @enable: whether the CRTC should be enabled, gates all other state
> + * @planes_changed: for use by helpers and drivers when computing state updates
>   * @mode: current mode timings
>   * @event: optional pointer to a DRM event to signal upon completion of the
>   * state update
> @@ -237,6 +238,9 @@ struct drm_atomic_state;
>  struct drm_crtc_state {
>   bool enable        : 1;
>
> + /* computed state bits used by helpers and drivers */
> + bool planes_changed : 1;
> +
>   struct drm_display_mode mode;
>
>   struct drm_pending_vblank_event *event;
> @@ -748,6 +752,8 @@ struct drm_plane {
>
>   enum drm_plane_type type;
>
> + void *helper_private;
> +
>   struct drm_plane_state *state;
>  };
>
> diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
> index a3d75fefd010..adec48b27aa5 100644
> --- a/include/drm/drm_crtc_helper.h
> +++ b/include/drm/drm_crtc_helper.h
> @@ -81,6 +81,12 @@ struct drm_crtc_helper_funcs {
>
>   /* disable crtc when not in use - more explicit than dpms off */
>   void (*disable)(struct drm_crtc *crtc);
> +
> + /* atomic helpers */
> + int (*atomic_check)(struct drm_crtc *crtc,
> +    struct drm_crtc_state *state);
> + void (*atomic_begin)(struct drm_crtc *crtc);
> + void (*atomic_flush)(struct drm_crtc *crtc);
>  };
>
>  /**
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index 0e1dcb163b81..c2146a7d91fb 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -46,6 +46,28 @@ extern int drm_crtc_init(struct drm_device *dev,
>   struct drm_crtc *crtc,
>   const struct drm_crtc_funcs *funcs);
>
> +/**
> + * drm_plane_helper_funcs - helper operations for CRTCs
> + *
> + * The helper operations are called by the mid-layer CRTC helper.
> + */
> +struct drm_plane_helper_funcs {
> + int (*prepare_fb)(struct drm_plane *plane,
> +  struct drm_framebuffer *fb);
> + void (*cleanup_fb)(struct drm_plane *plane,
> +   struct drm_framebuffer *fb);
> +
> + int (*atomic_check)(struct drm_plane *plane,
> +    struct drm_plane_state *state);
> + void (*atomic_update)(struct drm_plane *plane);
> +};
> +
> +static inline void drm_plane_helper_add(struct drm_plane *plane,
> + const struct drm_plane_helper_funcs *funcs)
> +{
> + plane->helper_private = (void *)funcs;
> +}
> +
>  extern int drm_plane_helper_check_update(struct drm_plane *plane,
>   struct drm_crtc *crtc,
>   struct drm_framebuffer *fb,
> --
> 2.1.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx