Hi,
Minor comments below. LGTM otherwise.
On 11/05/2016 09:56 PM, Rob Clark wrote:
> (re)assign the hw pipes to planes based on required caps, and to handle
> situations where we could not modify an in-use plane (ie. SMP block
> reallocation).
>
> This means all planes advertise the superset of formats and properties.
> Userspace must (as always) use atomic TEST_ONLY step for atomic updates,
> as not all planes may be available for use on every frame.
>
> The mapping of hwpipe to plane is stored in mdp5_state, so that state
> updates are atomically committed in the same way that plane/etc state
> updates are managed. This is needed because the mdp5_plane_state keeps
> a pointer to the hwpipe, and we don't want global state to become out
> of sync with the plane state if an atomic update fails, we hit deadlock/
> backoff scenario, etc. The use of state_lock keeps multiple parallel
> updates which both re-assign hwpipes properly serialized.
>
> Signed-off-by: Rob Clark <robdclark at gmail.com>
> ---
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 2 +-
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 4 +-
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 7 +-
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c | 70 +++++++++++++
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.h | 10 ++
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 158
> ++++++++++++++++--------------
> 6 files changed, 171 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> index 6213f51..99958f7 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> @@ -407,7 +407,7 @@ static int mdp5_crtc_atomic_check(struct drm_crtc *crtc,
> for (i = 0; i < cnt; i++) {
> pstates[i].state->stage = STAGE_BASE + i + base;
> DBG("%s: assign pipe %s on stage=%d", crtc->name,
> - pipe2name(mdp5_plane_pipe(pstates[i].plane)),
> + pstates[i].plane->name,
> pstates[i].state->stage);
> }
>
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> index ca6dfeb..3542adf 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> @@ -92,7 +92,7 @@ struct mdp5_state *mdp5_get_state(struct drm_atomic_state
> *s)
> return ERR_PTR(-ENOMEM);
>
> /* Copy state: */
> - /* TODO */
> + new_state->hwpipe = mdp5_kms->state->hwpipe;
>
> state->state = new_state;
>
> @@ -377,7 +377,7 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
> struct drm_plane *plane;
> struct drm_crtc *crtc;
>
> - plane = mdp5_plane_init(dev, mdp5_kms->hwpipes[i], primary);
> + plane = mdp5_plane_init(dev, primary);
> if (IS_ERR(plane)) {
> ret = PTR_ERR(plane);
> dev_err(dev->dev, "failed to construct plane %d
> (%d)\n", i, ret);
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> index 52914ec..4475090 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> @@ -82,7 +82,7 @@ struct mdp5_kms {
> * For atomic updates which require modifying global state,
> */
> struct mdp5_state {
> - uint32_t dummy;
> + struct mdp5_hw_pipe_state hwpipe;
I was wondering if we could rename the above as hwpipes/hwpipe_state/hwpipe_map,
because it gets a confusing since variables of the type mdp5_hw_pipe are
also named hwpipe.
> };
>
> struct mdp5_state *__must_check
> @@ -94,6 +94,8 @@ mdp5_get_state(struct drm_atomic_state *s);
> struct mdp5_plane_state {
> struct drm_plane_state base;
>
> + struct mdp5_hw_pipe *hwpipe;
> +
> /* aligned with property */
> uint8_t premultiplied;
> uint8_t zpos;
> @@ -232,8 +234,7 @@ uint32_t mdp5_plane_get_flush(struct drm_plane *plane);
> void mdp5_plane_complete_commit(struct drm_plane *plane,
> struct drm_plane_state *state);
> enum mdp5_pipe mdp5_plane_pipe(struct drm_plane *plane);
> -struct drm_plane *mdp5_plane_init(struct drm_device *dev,
> - struct mdp5_hw_pipe *hwpipe, bool primary);
> +struct drm_plane *mdp5_plane_init(struct drm_device *dev, bool primary);
>
> uint32_t mdp5_crtc_vblank(struct drm_crtc *crtc);
>
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c
> index 7f3e8e50..115de7d 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c
> @@ -17,6 +17,76 @@
>
> #include "mdp5_kms.h"
>
> +struct mdp5_hw_pipe *mdp5_pipe_assign(struct drm_atomic_state *s,
> + struct drm_plane *plane, uint32_t caps)
> +{
> + struct msm_drm_private *priv = s->dev->dev_private;
> + struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
> + struct mdp5_state *state = mdp5_get_state(s);
> + struct mdp5_hw_pipe_state *old_state, *new_state;
> + struct mdp5_hw_pipe *hwpipe = NULL;
> + int i;
> +
Can we assign "state = mdp5_get_state(s);" here instead for clarity? Since the
func
does more than just getting the mdp5_state pointer?
> + if (IS_ERR(state))
> + return ERR_CAST(state);
> +
> + /* grab old_state after mdp5_get_state(), since now we hold lock: */
> + old_state = &mdp5_kms->state->hwpipe;
> + new_state = &state->hwpipe;
> +
> + for (i = 0; i < mdp5_kms->num_hwpipes; i++) {
> + struct mdp5_hw_pipe *cur = mdp5_kms->hwpipes[i];
> +
> + /* skip if already in-use.. check both new and old state,
> + * since we cannot immediately re-use a pipe that is
> + * released in the current update in some cases:
> + * (1) mdp5 has SMP (non-double-buffered)
> + * (2) hw pipe previously assigned to different CRTC
> + * (vblanks might not be aligned)
For 1), we have non-SMP SoCs (a.k.a 8996), and for 2) we would have
the info whether the hwpipe is changing crtcs in old and new states.
I guess we could leave this as an optimization for the future.
> + */
> + if (new_state->hwpipe_to_plane[cur->idx] ||
> + old_state->hwpipe_to_plane[cur->idx])
> + continue;
> +
> + /* skip if doesn't support some required caps: */
> + if (caps & ~cur->caps)
> + continue;
> +
> + /* possible candidate, take the one with the
> + * fewest unneeded caps bits set:
> + */
> + if (!hwpipe || (hweight_long(cur->caps & ~caps) <
> + hweight_long(hwpipe->caps & ~caps)))
> + hwpipe = cur;
> + }
> +
> + if (!hwpipe)
> + return ERR_PTR(-ENOMEM);
> +
> + DBG("%s: assign to plane %s for caps %x",
> + hwpipe->name, plane->name, caps);
> + new_state->hwpipe_to_plane[hwpipe->idx] = plane;
> +
> + return hwpipe;
> +}
> +
> +void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe
> *hwpipe)
> +{
> + struct mdp5_state *state = mdp5_get_state(s);
> + struct mdp5_hw_pipe_state *new_state = &state->hwpipe;
> +
> + if (!hwpipe)
> + return;
> +
> + if (WARN_ON(!new_state->hwpipe_to_plane[hwpipe->idx]))
> + return;
> +
> + DBG("%s: release from plane %s", hwpipe->name,
> + new_state->hwpipe_to_plane[hwpipe->idx]->name);
> +
> + new_state->hwpipe_to_plane[hwpipe->idx] = NULL;
> +}
> +
> void mdp5_pipe_destroy(struct mdp5_hw_pipe *hwpipe)
> {
> kfree(hwpipe);
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.h
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.h
> index dcfa0e0..bac5900 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.h
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.h
> @@ -32,6 +32,16 @@ struct mdp5_hw_pipe {
> uint32_t flush_mask; /* used to commit pipe registers */
> };
>
> +/* global atomic state of assignment between pipes and planes: */
> +struct mdp5_hw_pipe_state {
> + struct drm_plane *hwpipe_to_plane[16];
We could use SSPP_MAX here instead of 16. We would need to move its
definition from mdp5_crtc.c to mdp5_kms.h, though.
> +};
> +
> +struct mdp5_hw_pipe *__must_check
> +mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane,
> + uint32_t caps);
> +void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe
> *hwpipe);
> +
> struct mdp5_hw_pipe *mdp5_pipe_init(enum mdp5_pipe pipe,
> uint32_t reg_offset, uint32_t caps);
> void mdp5_pipe_destroy(struct mdp5_hw_pipe *hwpipe);
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> index e4ecfeb..64e97ef 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> @@ -22,8 +22,6 @@
> struct mdp5_plane {
> struct drm_plane base;
>
> - struct mdp5_hw_pipe *hwpipe;
> -
> uint32_t nformats;
> uint32_t formats[32];
> };
> @@ -63,12 +61,6 @@ static void mdp5_plane_destroy(struct drm_plane *plane)
> static void mdp5_plane_install_rotation_property(struct drm_device *dev,
> struct drm_plane *plane)
> {
> - struct mdp5_plane *mdp5_plane = to_mdp5_plane(plane);
> -
> - if (!(mdp5_plane->hwpipe->caps & MDP_PIPE_CAP_HFLIP) &&
> - !(mdp5_plane->hwpipe->caps & MDP_PIPE_CAP_VFLIP))
> - return;
> -
> drm_plane_create_rotation_property(plane,
> DRM_ROTATE_0,
> DRM_ROTATE_0 |
> @@ -184,6 +176,8 @@ mdp5_plane_atomic_print_state(struct drm_printer *p,
> {
> struct mdp5_plane_state *pstate = to_mdp5_plane_state(state);
>
> + drm_printf(p, "\thwpipe=%s\n", pstate->hwpipe ?
> + pstate->hwpipe->name : "(null)");
> drm_printf(p, "\tpremultiplied=%u\n", pstate->premultiplied);
> drm_printf(p, "\tzpos=%u\n", pstate->zpos);
> drm_printf(p, "\talpha=%u\n", pstate->alpha);
> @@ -237,10 +231,12 @@ mdp5_plane_duplicate_state(struct drm_plane *plane)
> static void mdp5_plane_destroy_state(struct drm_plane *plane,
> struct drm_plane_state *state)
> {
> + struct mdp5_plane_state *pstate = to_mdp5_plane_state(state);
> +
> if (state->fb)
> drm_framebuffer_unreference(state->fb);
>
> - kfree(to_mdp5_plane_state(state));
> + kfree(pstate);
> }
>
> static const struct drm_plane_funcs mdp5_plane_funcs = {
> @@ -285,70 +281,81 @@ static void mdp5_plane_cleanup_fb(struct drm_plane
> *plane,
> static int mdp5_plane_atomic_check(struct drm_plane *plane,
> struct drm_plane_state *state)
> {
> - struct mdp5_plane *mdp5_plane = to_mdp5_plane(plane);
> + struct mdp5_plane_state *mdp5_state = to_mdp5_plane_state(state);
> struct drm_plane_state *old_state = plane->state;
> - const struct mdp_format *format;
> - bool vflip, hflip;
> + bool new_hwpipe = false;
> + uint32_t caps = 0;
>
> DBG("%s: check (%d -> %d)", plane->name,
> plane_enabled(old_state), plane_enabled(state));
>
> + /* We don't allow faster-than-vblank updates.. if we did add this
> + * some day, we would need to disallow in cases where hwpipe
> + * changes
> + */
> + if (WARN_ON(to_mdp5_plane_state(old_state)->pending))
> + return -EBUSY;
> +
> if (plane_enabled(state)) {
> unsigned int rotation;
> + const struct mdp_format *format;
>
> format = to_mdp_format(msm_framebuffer_format(state->fb));
> - if (MDP_FORMAT_IS_YUV(format) &&
> - !pipe_supports_yuv(mdp5_plane->hwpipe->caps)) {
> - DBG("Pipe doesn't support YUV\n");
> -
> - return -EINVAL;
> - }
> -
> - if (!(mdp5_plane->hwpipe->caps & MDP_PIPE_CAP_SCALE) &&
> - (((state->src_w >> 16) != state->crtc_w) ||
> - ((state->src_h >> 16) != state->crtc_h))) {
> - DBG("Pipe doesn't support scaling (%dx%d -> %dx%d)\n",
> - state->src_w >> 16, state->src_h >> 16,
> - state->crtc_w, state->crtc_h);
> + if (MDP_FORMAT_IS_YUV(format))
> + caps |= MDP_PIPE_CAP_SCALE | MDP_PIPE_CAP_CSC;
>
> - return -EINVAL;
> - }
> + if (((state->src_w >> 16) != state->crtc_w) ||
> + ((state->src_h >> 16) != state->crtc_h))
> + caps |= MDP_PIPE_CAP_SCALE;
>
> rotation = drm_rotation_simplify(state->rotation,
> DRM_ROTATE_0 |
> DRM_REFLECT_X |
> DRM_REFLECT_Y);
>
> - hflip = !!(rotation & DRM_REFLECT_X);
> - vflip = !!(rotation & DRM_REFLECT_Y);
> -
> - if ((vflip && !(mdp5_plane->hwpipe->caps & MDP_PIPE_CAP_VFLIP))
> ||
> - (hflip && !(mdp5_plane->hwpipe->caps &
> MDP_PIPE_CAP_HFLIP))) {
> - DBG("Pipe doesn't support flip\n");
> -
> - return -EINVAL;
> + if (rotation & DRM_REFLECT_X)
> + caps |= MDP_PIPE_CAP_HFLIP;
> +
> + if (rotation & DRM_REFLECT_Y)
> + caps |= MDP_PIPE_CAP_VFLIP;
> +
> + /* (re)allocate hw pipe if we don't have one or caps-mismatch:
> */
> + if (!mdp5_state->hwpipe || (caps & ~mdp5_state->hwpipe->caps))
> + new_hwpipe = true;
> +
> + if (plane_enabled(old_state)) {
> + bool full_modeset = false;
> + if (state->fb->pixel_format !=
> old_state->fb->pixel_format) {
> + DBG("%s: pixel_format change!", plane->name);
> + full_modeset = true;
> + }
> + if (state->src_w != old_state->src_w) {
> + DBG("%s: src_w change!", plane->name);
> + full_modeset = true;
> + }
> + if (full_modeset) {
> + /* cannot change SMP block allocation during
> + * scanout:
> + */
> + if (get_kms(plane)->smp)
> + new_hwpipe = true;
> + }
> }
> - }
>
> - if (plane_enabled(state) && plane_enabled(old_state)) {
> - /* we cannot change SMP block configuration during scanout: */
> - bool full_modeset = false;
> - if (state->fb->pixel_format != old_state->fb->pixel_format) {
> - DBG("%s: pixel_format change!", plane->name);
> - full_modeset = true;
> - }
> - if (state->src_w != old_state->src_w) {
> - DBG("%s: src_w change!", plane->name);
> - full_modeset = true;
> - }
> - if (to_mdp5_plane_state(old_state)->pending) {
> - DBG("%s: still pending!", plane->name);
> - full_modeset = true;
> - }
> - if (full_modeset) {
> - struct drm_crtc_state *crtc_state =
> - drm_atomic_get_crtc_state(state->state,
> state->crtc);
> - crtc_state->mode_changed = true;
> + /* (re)assign hwpipe if needed, otherwise keep old one: */
> + if (new_hwpipe) {
> + /* TODO maybe we want to re-assign hwpipe sometimes
> + * in cases when we no-longer need some caps to make
> + * it available for other planes?
> + */
> + struct mdp5_hw_pipe *hwpipe = mdp5_state->hwpipe;
Maybe hwpipe above could be curr_hwpipe or old_hwpipe?
> + mdp5_state->hwpipe =
> + mdp5_pipe_assign(state->state, plane, caps);
> + if (IS_ERR(mdp5_state->hwpipe)) {
> + DBG("%s: failed to assign hwpipe!",
> plane->name);
> + return PTR_ERR(mdp5_state->hwpipe);
> + }
> + mdp5_pipe_release(state->state, hwpipe);
> }
> }
Thanks,
Archit
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
