Re: [PATCH 14/17] drm/msm: add atomic support

On Sat, May 24, 2014 at 02:30:23PM -0400, Rob Clark wrote:

Signed-off-by: Rob Clark robdclark@gmail.com

drivers/gpu/drm/msm/Makefile | 1 + drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 57 ++++++------ drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 6 ++ drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h | 1 + drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 5 -- drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 56 ++++++------ drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 6 ++ drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 2 +- drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 5 -- drivers/gpu/drm/msm/msm_atomic.c | 141 ++++++++++++++++++++++++++++++ drivers/gpu/drm/msm/msm_drv.c | 22 ++++- drivers/gpu/drm/msm/msm_drv.h | 7 ++ drivers/gpu/drm/msm/msm_gem.c | 24 +---- drivers/gpu/drm/msm/msm_gem.h | 13 +++ drivers/gpu/drm/msm/msm_kms.h | 1 + 15 files changed, 253 insertions(+), 94 deletions(-) create mode 100644 drivers/gpu/drm/msm/msm_atomic.c

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile index 5e1e6b0..dd12f56 100644 --- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile @@ -27,6 +27,7 @@ msm-y := \ mdp/mdp5/mdp5_kms.o \ mdp/mdp5/mdp5_plane.o \ mdp/mdp5/mdp5_smp.o \

• msm_atomic.o \ msm_drv.o \ msm_fb.o \ msm_gem.o \

diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c index d0d8befd..2fa6b75 100644 --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c @@ -52,7 +52,6 @@ struct mdp4_crtc {

/* if there is a pending flip, these will be non-null: */ struct drm_pending_vblank_event *event;

• struct msm_fence_cb pageflip_cb;

#define PENDING_CURSOR 0x1 #define PENDING_FLIP 0x2 @@ -93,12 +92,16 @@ static void request_pending(struct drm_crtc *crtc, uint32_t pending) mdp_irq_register(&get_kms(crtc)->base, &mdp4_crtc->vblank); }

-static void crtc_flush(struct drm_crtc *crtc) +void mdp4_crtc_flush(struct drm_crtc *crtc) {

• struct msm_drm_private *priv = crtc->dev->dev_private; struct mdp4_crtc *mdp4_crtc = to_mdp4_crtc(crtc); struct mdp4_kms *mdp4_kms = get_kms(crtc); uint32_t i, flush = 0;

• if (priv->pending_crtcs & (1 << crtc->id))

• return;
  

• for (i = 0; i < ARRAY_SIZE(mdp4_crtc->planes); i++) { struct drm_plane *plane = mdp4_crtc->planes[i]; if (plane) {

@@ -142,7 +145,7 @@ static void update_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) * so that we can safely queue unref to current fb (ie. next * vblank we know hw is done w/ previous scanout_fb). */

• crtc_flush(crtc);

• mdp4_crtc_flush(crtc);

  if (mdp4_crtc->scanout_fb) drm_flip_work_queue(&mdp4_crtc->unref_fb_work,

@@ -177,21 +180,6 @@ static void complete_flip(struct drm_crtc *crtc, struct drm_file *file) spin_unlock_irqrestore(&dev->event_lock, flags); }

-static void pageflip_cb(struct msm_fence_cb *cb) -{

• struct mdp4_crtc *mdp4_crtc =

• container_of(cb, struct mdp4_crtc, pageflip_cb);
  

• struct drm_crtc *crtc = &mdp4_crtc->base;
• struct drm_framebuffer *fb = crtc->primary->fb;
• if (!fb)

• return;
  

• drm_framebuffer_reference(fb);
• mdp4_plane_set_scanout(mdp4_crtc->plane, fb);
• update_scanout(crtc, fb);

-}

static void unref_fb_worker(struct drm_flip_work *work, void *val) { struct mdp4_crtc *mdp4_crtc = @@ -406,7 +394,7 @@ static void mdp4_crtc_prepare(struct drm_crtc *crtc) static void mdp4_crtc_commit(struct drm_crtc *crtc) { mdp4_crtc_dpms(crtc, DRM_MODE_DPMS_ON);

• crtc_flush(crtc);

• mdp4_crtc_flush(crtc); /* drop the ref to mdp clk's that we got in prepare: */ mdp4_disable(get_kms(crtc));

} @@ -448,23 +436,27 @@ static int mdp4_crtc_page_flip(struct drm_crtc *crtc, { struct mdp4_crtc *mdp4_crtc = to_mdp4_crtc(crtc); struct drm_device *dev = crtc->dev;

• struct drm_gem_object *obj; unsigned long flags;

• spin_lock_irqsave(&dev->event_lock, flags); if (mdp4_crtc->event) { dev_err(dev->dev, "already pending flip!\n");

• spin_unlock_irqrestore(&dev->event_lock, flags);
  

  return -EBUSY; }

• obj = msm_framebuffer_bo(new_fb, 0);

• spin_lock_irqsave(&dev->event_lock, flags); mdp4_crtc->event = event; spin_unlock_irqrestore(&dev->event_lock, flags);

  update_fb(crtc, new_fb);

• return msm_gem_queue_inactive_cb(obj, &mdp4_crtc->pageflip_cb);

• /* grab extra ref for update_scanout() */
• drm_framebuffer_reference(new_fb);
• mdp4_plane_set_scanout(mdp4_crtc->plane, new_fb);
• update_scanout(crtc, new_fb);
• return 0;

}

static int mdp4_crtc_set_property(struct drm_crtc *crtc, @@ -598,7 +590,7 @@ static int mdp4_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) mdp4_crtc->cursor.y = y; spin_unlock_irqrestore(&mdp4_crtc->cursor.lock, flags);

• crtc_flush(crtc);

• mdp4_crtc_flush(crtc); request_pending(crtc, PENDING_CURSOR);

  return 0;

@@ -635,8 +627,13 @@ static void mdp4_crtc_vblank_irq(struct mdp_irq *irq, uint32_t irqstatus) pending = atomic_xchg(&mdp4_crtc->pending, 0);

if (pending & PENDING_FLIP) {

• complete_flip(crtc, NULL);
  

• drm_flip_work_commit(&mdp4_crtc->unref_fb_work, priv->wq);
  

• if (priv->pending_crtcs & (1 << crtc->id)) {
  

• 	/* our update hasn't been flushed yet: */
  

• 	request_pending(crtc, PENDING_FLIP);
  

• } else {
  

• 	complete_flip(crtc, NULL);
  

• 	drm_flip_work_commit(&mdp4_crtc->unref_fb_work, priv->wq);
  

• }
  

  }

  if (pending & PENDING_CURSOR) {

@@ -650,7 +647,7 @@ static void mdp4_crtc_err_irq(struct mdp_irq *irq, uint32_t irqstatus) struct mdp4_crtc *mdp4_crtc = container_of(irq, struct mdp4_crtc, err); struct drm_crtc *crtc = &mdp4_crtc->base; DBG("%s: error: %08x", mdp4_crtc->name, irqstatus);

• crtc_flush(crtc);

• mdp4_crtc_flush(crtc);

}

uint32_t mdp4_crtc_vblank(struct drm_crtc *crtc) @@ -730,7 +727,7 @@ static void set_attach(struct drm_crtc *crtc, enum mdp4_pipe pipe_id, mdp4_crtc->planes[pipe_id] = plane; blend_setup(crtc); if (mdp4_crtc->enabled && (plane != mdp4_crtc->plane))

• crtc_flush(crtc);
  

• mdp4_crtc_flush(crtc);
  

}

void mdp4_crtc_attach(struct drm_crtc *crtc, struct drm_plane *plane) @@ -792,8 +789,6 @@ struct drm_crtc *mdp4_crtc_init(struct drm_device *dev, ret = drm_flip_work_init(&mdp4_crtc->unref_cursor_work, 64, "unref cursor", unref_cursor_worker);

• INIT_FENCE_CB(&mdp4_crtc->pageflip_cb, pageflip_cb);
• drm_crtc_init_with_planes(dev, crtc, plane, NULL, &mdp4_crtc_funcs); drm_crtc_helper_add(crtc, &mdp4_crtc_helper_funcs);

diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c index 0bb4faa..af0bfb1 100644 --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c @@ -131,6 +131,11 @@ static long mdp4_round_pixclk(struct msm_kms *kms, unsigned long rate, return mdp4_dtv_round_pixclk(encoder, rate); }

+static void mdp4_flush(struct msm_kms *kms, struct drm_crtc *crtc) +{

• mdp4_crtc_flush(crtc);

+}

static void mdp4_preclose(struct msm_kms *kms, struct drm_file *file) { struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms)); @@ -162,6 +167,7 @@ static const struct mdp_kms_funcs kms_funcs = { .disable_vblank = mdp4_disable_vblank, .get_format = mdp_get_format, .round_pixclk = mdp4_round_pixclk,

• .flush           = mdp4_flush,
  

  .preclose = mdp4_preclose, .destroy = mdp4_destroy, },

diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h index 715520c5..834454c 100644 --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h @@ -184,6 +184,7 @@ enum mdp4_pipe mdp4_plane_pipe(struct drm_plane *plane); struct drm_plane *mdp4_plane_init(struct drm_device *dev, enum mdp4_pipe pipe_id, bool private_plane);

+void mdp4_crtc_flush(struct drm_crtc *crtc); uint32_t mdp4_crtc_vblank(struct drm_crtc *crtc); void mdp4_crtc_cancel_pending_flip(struct drm_crtc *crtc, struct drm_file *file); void mdp4_crtc_set_config(struct drm_crtc *crtc, uint32_t config); diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c index 4c92985..f35848d 100644 --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c @@ -48,11 +48,6 @@ static int mdp4_plane_update(struct drm_plane *plane,

mdp4_plane->enabled = true;

• if (plane->fb)

• drm_framebuffer_unreference(plane->fb);
  

• drm_framebuffer_reference(fb);
• return mdp4_plane_mode_set(plane, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, src_x, src_y, src_w, src_h);

diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c index 7f4ee99..0e74317 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c @@ -35,7 +35,6 @@ struct mdp5_crtc {

/* if there is a pending flip, these will be non-null: */ struct drm_pending_vblank_event *event;

• struct msm_fence_cb pageflip_cb;

#define PENDING_CURSOR 0x1 #define PENDING_FLIP 0x2 @@ -73,13 +72,17 @@ static void request_pending(struct drm_crtc *crtc, uint32_t pending) mdp_irq_register(&get_kms(crtc)->base, &mdp5_crtc->vblank); }

-static void crtc_flush(struct drm_crtc *crtc) +void mdp5_crtc_flush(struct drm_crtc *crtc) {

• struct msm_drm_private *priv = crtc->dev->dev_private; struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc); struct mdp5_kms *mdp5_kms = get_kms(crtc); int id = mdp5_crtc->id; uint32_t i, flush = 0;

• if (priv->pending_crtcs & (1 << crtc->id))

• return;
  

• for (i = 0; i < ARRAY_SIZE(mdp5_crtc->planes); i++) { struct drm_plane *plane = mdp5_crtc->planes[i]; if (plane) {

@@ -124,7 +127,7 @@ static void update_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) * so that we can safely queue unref to current fb (ie. next * vblank we know hw is done w/ previous scanout_fb). */

• crtc_flush(crtc);

• mdp5_crtc_flush(crtc);

  if (mdp5_crtc->scanout_fb) drm_flip_work_queue(&mdp5_crtc->unref_fb_work,

@@ -165,21 +168,6 @@ static void complete_flip(struct drm_crtc *crtc, struct drm_file *file) } }

-static void pageflip_cb(struct msm_fence_cb *cb) -{

• struct mdp5_crtc *mdp5_crtc =

• container_of(cb, struct mdp5_crtc, pageflip_cb);
  

• struct drm_crtc *crtc = &mdp5_crtc->base;
• struct drm_framebuffer *fb = mdp5_crtc->fb;
• if (!fb)

• return;
  

• drm_framebuffer_reference(fb);
• mdp5_plane_set_scanout(mdp5_crtc->plane, fb);
• update_scanout(crtc, fb);

-}

static void unref_fb_worker(struct drm_flip_work *work, void *val) { struct mdp5_crtc *mdp5_crtc = @@ -324,7 +312,7 @@ static void mdp5_crtc_prepare(struct drm_crtc *crtc) static void mdp5_crtc_commit(struct drm_crtc *crtc) { mdp5_crtc_dpms(crtc, DRM_MODE_DPMS_ON);

• crtc_flush(crtc);

• mdp5_crtc_flush(crtc); /* drop the ref to mdp clk's that we got in prepare: */ mdp5_disable(get_kms(crtc));

} @@ -366,23 +354,26 @@ static int mdp5_crtc_page_flip(struct drm_crtc *crtc, { struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc); struct drm_device *dev = crtc->dev;

• struct drm_gem_object *obj; unsigned long flags;

• spin_lock_irqsave(&dev->event_lock, flags); if (mdp5_crtc->event) { dev_err(dev->dev, "already pending flip!\n");

• spin_unlock_irqrestore(&dev->event_lock, flags);
  

  return -EBUSY; }

• obj = msm_framebuffer_bo(new_fb, 0);

• spin_lock_irqsave(&dev->event_lock, flags); mdp5_crtc->event = event; spin_unlock_irqrestore(&dev->event_lock, flags);

  update_fb(crtc, new_fb);

• return msm_gem_queue_inactive_cb(obj, &mdp5_crtc->pageflip_cb);

• /* grab extra ref for update_scanout() */
• drm_framebuffer_reference(new_fb);
• mdp5_plane_set_scanout(mdp5_crtc->plane, new_fb);
• update_scanout(crtc, new_fb);
• return 0;

}

static int mdp5_crtc_set_property(struct drm_crtc *crtc, @@ -424,8 +415,13 @@ static void mdp5_crtc_vblank_irq(struct mdp_irq *irq, uint32_t irqstatus) pending = atomic_xchg(&mdp5_crtc->pending, 0);

if (pending & PENDING_FLIP) {

• complete_flip(crtc, NULL);
  

• drm_flip_work_commit(&mdp5_crtc->unref_fb_work, priv->wq);
  

• if (priv->pending_crtcs & (1 << crtc->id)) {
  

• 	/* our update hasn't been flushed yet: */
  

• 	request_pending(crtc, PENDING_FLIP);
  

• } else {
  

• 	complete_flip(crtc, NULL);
  

• 	drm_flip_work_commit(&mdp5_crtc->unref_fb_work, priv->wq);
  

• }
  

  }

}

@@ -434,7 +430,7 @@ static void mdp5_crtc_err_irq(struct mdp_irq *irq, uint32_t irqstatus) struct mdp5_crtc *mdp5_crtc = container_of(irq, struct mdp5_crtc, err); struct drm_crtc *crtc = &mdp5_crtc->base; DBG("%s: error: %08x", mdp5_crtc->name, irqstatus);

• crtc_flush(crtc);

• mdp5_crtc_flush(crtc);

}

uint32_t mdp5_crtc_vblank(struct drm_crtc *crtc) @@ -501,7 +497,7 @@ void mdp5_crtc_set_intf(struct drm_crtc *crtc, int intf, MDP5_CTL_OP_MODE(MODE_NONE) | MDP5_CTL_OP_INTF_NUM(intfnum[intf]));

• crtc_flush(crtc);

• mdp5_crtc_flush(crtc);

}

static void set_attach(struct drm_crtc *crtc, enum mdp5_pipe pipe_id, @@ -517,7 +513,7 @@ static void set_attach(struct drm_crtc *crtc, enum mdp5_pipe pipe_id, mdp5_crtc->planes[pipe_id] = plane; blend_setup(crtc); if (mdp5_crtc->enabled && (plane != mdp5_crtc->plane))

• crtc_flush(crtc);
  

• mdp5_crtc_flush(crtc);
  

}

void mdp5_crtc_attach(struct drm_crtc *crtc, struct drm_plane *plane) @@ -563,8 +559,6 @@ struct drm_crtc *mdp5_crtc_init(struct drm_device *dev, if (ret) goto fail;

• INIT_FENCE_CB(&mdp5_crtc->pageflip_cb, pageflip_cb);
• drm_crtc_init_with_planes(dev, crtc, plane, NULL, &mdp5_crtc_funcs); drm_crtc_helper_add(crtc, &mdp5_crtc_helper_funcs);

diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c index ee8446c..01c3d70 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c @@ -91,6 +91,11 @@ static long mdp5_round_pixclk(struct msm_kms *kms, unsigned long rate, return rate; }

+static void mdp5_flush(struct msm_kms *kms, struct drm_crtc *crtc) +{

• mdp5_crtc_flush(crtc);

+}

static void mdp5_preclose(struct msm_kms *kms, struct drm_file *file) { struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); @@ -118,6 +123,7 @@ static const struct mdp_kms_funcs kms_funcs = { .disable_vblank = mdp5_disable_vblank, .get_format = mdp_get_format, .round_pixclk = mdp5_round_pixclk,

• .flush           = mdp5_flush,
  

  .preclose = mdp5_preclose, .destroy = mdp5_destroy, },

diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h index c8b1a25..18b031b 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h @@ -197,8 +197,8 @@ enum mdp5_pipe mdp5_plane_pipe(struct drm_plane *plane); struct drm_plane *mdp5_plane_init(struct drm_device *dev, enum mdp5_pipe pipe, bool private_plane);

+void mdp5_crtc_flush(struct drm_crtc *crtc); uint32_t mdp5_crtc_vblank(struct drm_crtc *crtc);

void mdp5_crtc_cancel_pending_flip(struct drm_crtc *crtc, struct drm_file *file); void mdp5_crtc_set_intf(struct drm_crtc *crtc, int intf, enum mdp5_intf intf_id); diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c index 53cc8c6..f1bf3c2 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c @@ -48,11 +48,6 @@ static int mdp5_plane_update(struct drm_plane *plane,

mdp5_plane->enabled = true;

• if (plane->fb)

• drm_framebuffer_unreference(plane->fb);
  

• drm_framebuffer_reference(fb);
• return mdp5_plane_mode_set(plane, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, src_x, src_y, src_w, src_h);

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c new file mode 100644 index 0000000..b231377 --- /dev/null +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -0,0 +1,141 @@ +/*

• • Copyright (C) 2013 Red Hat
• • Author: Rob Clark robdclark@gmail.com
• • This program is free software; you can redistribute it and/or modify it
• • under the terms of the GNU General Public License version 2 as published by
• • the Free Software Foundation.
• • This program is distributed in the hope that it will be useful, but WITHOUT
• • ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
• • FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
• • more details.
• • You should have received a copy of the GNU General Public License along with
• • this program. If not, see http://www.gnu.org/licenses/.
• */

+#include "msm_drv.h" +#include "msm_kms.h" +#include "msm_gem.h"

+struct msm_async_commit {

• struct drm_atomic_state *state;
• uint32_t fence;
• struct msm_fence_cb fence_cb;

+};

+static void fence_cb(struct msm_fence_cb *cb); +static int commit_sync(struct drm_device *dev, void *state, bool unlocked);

+static struct msm_async_commit *new_commit(struct drm_atomic_state *state) +{

• struct msm_async_commit *c = kzalloc(sizeof(*c), GFP_KERNEL);
• if (!c)

• return NULL;
  

• drm_atomic_state_reference(state);
• c->state = state;
• INIT_FENCE_CB(&c->fence_cb, fence_cb);
• return c;

+} +static void free_commit(struct msm_async_commit *c) +{

• drm_atomic_state_unreference(c->state);
• kfree(c);

+}

+static void fence_cb(struct msm_fence_cb *cb) +{

• struct msm_async_commit *c =

• 	container_of(cb, struct msm_async_commit, fence_cb);
  

• commit_sync(c->state->dev, c->state, true);
• free_commit(c);

+}

+static void add_fb(struct msm_async_commit *c, struct drm_crtc *crtc,

• struct drm_framebuffer *fb)
  

+{

• struct drm_gem_object *obj = msm_framebuffer_bo(fb, 0);
• c->fence = max(c->fence, msm_gem_fence(to_msm_bo(obj), MSM_PREP_READ));

+}

+static int wait_fb(struct drm_crtc *crtc, struct drm_framebuffer *fb) +{

• // XXX TODO wait..
• return 0;

+}

+#define pending_fb(state) ((state) && (state)->fb && (state)->new_fb)

+static int commit_sync(struct drm_device *dev, void *state, bool unlocked) +{

• struct drm_atomic_state *a = state;
• struct msm_drm_private *priv = dev->dev_private;
• struct msm_kms *kms = priv->kms;
• int ncrtcs = dev->mode_config.num_crtc;
• uint32_t pending_crtcs = 0;
• int i, ret;
• for (i = 0; i < ncrtcs; i++)

• if (a->crtcs[i])
  

• 	pending_crtcs |= (1 << a->crtcs[i]->id);
  

• mutex_lock(&dev->struct_mutex);
• WARN_ON(priv->pending_crtcs & pending_crtcs);
• priv->pending_crtcs |= pending_crtcs;
• mutex_unlock(&dev->struct_mutex);
• if (unlocked)

• ret = drm_atomic_commit_unlocked(dev, state);
  

• else

• ret = drm_atomic_commit(dev, state);
  

• mutex_lock(&dev->struct_mutex);
• priv->pending_crtcs &= ~pending_crtcs;
• mutex_unlock(&dev->struct_mutex);
• if (ret)

• return ret;
  

• for (i = 0; i < ncrtcs; i++)

• if (a->crtcs[i])
  

• 	kms->funcs->flush(kms, a->crtcs[i]);
  

• return 0;

+}

+int msm_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state) +{

• struct drm_atomic_state *a = state;
• int nplanes = dev->mode_config.num_total_plane;
• int i;
• if (a->flags & DRM_MODE_ATOMIC_NONBLOCK) {

• /* non-block mode: defer commit until fb's are ready */
  

• struct msm_async_commit *c = new_commit(state);
  

• if (!c)
  

• 	return -ENOMEM;
  

• for (i = 0; i < nplanes; i++)
  

• 	if (pending_fb(a->pstates[i]))
  

• 		add_fb(c, a->pstates[i]->crtc, a->pstates[i]->fb);
  

• return msm_queue_fence_cb(dev, &c->fence_cb, c->fence);
  

• } else {

• /* blocking mode: wait until fb's are ready */
  

• int ret = 0;
  

• for (i = 0; i < nplanes && !ret; i++)
  

• 	if (pending_fb(a->pstates[i]))
  

• 		ret = wait_fb(a->pstates[i]->crtc, a->pstates[i]->fb);
  

• if (ret)
  

• 	return ret;
  

• return commit_sync(dev, state, false);
  

• }

+}

Ok, I think I should have read your msm implementation a _lot_ earlier. Explains your desing choices neatly.

Two observations:

- A GO bit makes nuclear pageflips ridiculously easy to implement, presuming the hardware actually works. And it's the sane model, so imo a good one to wrap the atomic helpers around.

But reality is often a lot more ugly, especially if you're employed by Intel. Which is why I'm harping so much on this helpers-vs-core interface issues ... We really need the full state transition in one piece to do anything useful.

- msm doesn't have any resource sharing going on for modeset operations (which I mean lighting up crtcs to feed pixel streams to connectors). Which means the simplistic "loop over all crtcs and call the old ->setcrtc" approach actually works.

The problem I see here is that if you have hardware with more elaborate needs (e.g. shared dplls), but otherwise sanity for plane updates (i.e. a GO bit) then your current atomic helpers will make it rather hard to mix this. So I think we should pimp the crtc helpers a bit to be atomic compliant (i.e. kill all outputs first before enabling anything new) and try to integrate this with the atomic helpers used for GO bit style updates.

i915 has dpll sharing on ivb/hsw, but doesn't use the the crtc helpers anymore. But the radone eyefinity (or whatever the damn thing is called) I have lying around here fits the bill: It has 5 DP+ outputs but only 2 dplls. So you can drive e.g. 3 DP screens and then switch to 2 HDMI screens atomically and it should all pan out.

But since your current helpers just loop over all crtcs without any regard to ordering constraints this will fall over if the 2 HDMI outputs get enabled before the 3 DP outputs get disabled all disabled.

So with all that in mind I have 3 sanity checks for the atomic interfaces and helpers:

1. The atomic helpers should make enabling sane hw with a GO bit easy for nuclear pageflips. Benchmark would be sane hw like msm or omap.

2. It should cooperate with the crtc helpers such that hw with some shared resources (like dplls) works for atomic modesets. That pretty much means "disable everything (including crtc/connetor pipelines that only changed some parts) before enabling anything". Benchmark would be a platform with shared dplls.

3. The core->driver interface should be powerful enough to support insanity like i915, but no more. Which means all the code that's share (i.e. the set_prop code I've been harping all over the place about) should be done in the core, without any means for drivers to override. Currently the drm core also takes care of a bunch of things like basic locking and refcounting, and that's kinda the spirit for this. i915 is the obvious benchmark here.

I think we can roll this out piecemeal (and it's probably better to do it that way), but I also think that until we've resolved the requirements of 2&3 we should try to minimize subsystem wide changes as much as possible by making them opt-in and the vfuncs optional.

If you compare this approach with my review for Matt's universal plane patches it's exactly the same song.

I hope this elaboration of my thinking clarifies all my review comments a bit and explains what I'm aiming for.

Cheers, Daniel