Re: [PATCH 12/17] drm: convert crtc to properties/state

On Mon, May 26, 2014 at 11:31:10AM +0200, Daniel Vetter wrote:

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

...

Break the mutable state of a crtc out into a separate structure and use atomic properties mechanism to set crtc attributes. This makes it easier to have some helpers for crtc->set_property() and for checking for invalid params. The idea is that individual drivers can wrap the state struct in their own struct which adds driver specific parameters, for easy build-up of state across multiple set_property() calls and for easy atomic commit or roll- back.

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

Same comments about interface design as for the plane patch apply here. One additional comment below.

...

---

drivers/gpu/drm/armada/armada_crtc.c | 11 +- drivers/gpu/drm/ast/ast_mode.c | 1 + drivers/gpu/drm/cirrus/cirrus_mode.c | 1 + drivers/gpu/drm/drm_atomic.c | 231 ++++++++++- drivers/gpu/drm/drm_crtc.c | 598 ++++++++++++++++++----------- drivers/gpu/drm/drm_fb_helper.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 7 +- drivers/gpu/drm/gma500/cdv_intel_display.c | 1 + drivers/gpu/drm/gma500/psb_intel_display.c | 1 + drivers/gpu/drm/i915/intel_display.c | 1 + drivers/gpu/drm/mgag200/mgag200_mode.c | 1 + drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 6 +- drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 6 +- drivers/gpu/drm/nouveau/dispnv04/crtc.c | 1 + drivers/gpu/drm/nouveau/nv50_display.c | 1 + drivers/gpu/drm/omapdrm/omap_crtc.c | 12 +- drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- drivers/gpu/drm/qxl/qxl_display.c | 2 + drivers/gpu/drm/radeon/radeon_display.c | 2 + drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 + drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 2 + drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 1 + drivers/gpu/drm/udl/udl_modeset.c | 2 + drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 1 + drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 1 + include/drm/drm_atomic.h | 29 ++ include/drm/drm_crtc.h | 103 ++++- 27 files changed, 785 insertions(+), 243 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index 7d3c649..6237af4 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -9,6 +9,7 @@ #include <linux/clk.h> #include <drm/drmP.h> #include <drm/drm_crtc_helper.h> +#include <drm/drm_atomic.h> #include "armada_crtc.h" #include "armada_drm.h" #include "armada_fb.h" @@ -966,7 +967,12 @@ armada_drm_crtc_set_property(struct drm_crtc *crtc, { struct armada_private *priv = crtc->dev->dev_private; struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);

• struct drm_crtc_state *cstate = drm_atomic_get_crtc_state(crtc, state); bool update_csc = false;

• int ret = 0;

• if (IS_ERR(cstate))

• return PTR_ERR(cstate);
  
  

  if (property == priv->csc_yuv_prop) { dcrtc->csc_yuv_mode = val;

@@ -974,6 +980,9 @@ armada_drm_crtc_set_property(struct drm_crtc *crtc, } else if (property == priv->csc_rgb_prop) { dcrtc->csc_rgb_mode = val; update_csc = true;

• } else {

• ret = drm_crtc_set_property(crtc, cstate, property,
  

• 		val, blob_data);
  

  }

  if (update_csc) {

@@ -984,7 +993,7 @@ armada_drm_crtc_set_property(struct drm_crtc *crtc, writel_relaxed(val, dcrtc->base + LCD_SPU_IOPAD_CONTROL); }

• return 0;

• return ret;

}

static struct drm_crtc_funcs armada_crtc_funcs = { diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 114aee9..c08e0e1 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -632,6 +632,7 @@ static const struct drm_crtc_funcs ast_crtc_funcs = { .cursor_move = ast_cursor_move, .reset = ast_crtc_reset, .set_config = drm_crtc_helper_set_config,

• .set_property = drm_atomic_crtc_set_property, .gamma_set = ast_crtc_gamma_set, .destroy = ast_crtc_destroy,

}; diff --git a/drivers/gpu/drm/cirrus/cirrus_mode.c b/drivers/gpu/drm/cirrus/cirrus_mode.c index 49332c5..3c4428c 100644 --- a/drivers/gpu/drm/cirrus/cirrus_mode.c +++ b/drivers/gpu/drm/cirrus/cirrus_mode.c @@ -366,6 +366,7 @@ static void cirrus_crtc_destroy(struct drm_crtc *crtc) static const struct drm_crtc_funcs cirrus_crtc_funcs = { .gamma_set = cirrus_crtc_gamma_set, .set_config = drm_crtc_helper_set_config,

• .set_property = drm_atomic_crtc_set_property, .destroy = cirrus_crtc_destroy,

};

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 403ffc5..863a0fe 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -48,11 +48,13 @@ struct drm_atomic_state *drm_atomic_begin(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 sz; void *ptr;

  sz = sizeof(*state); sz += (sizeof(state->planes) + sizeof(state->pstates)) * nplanes;

• sz += (sizeof(state->crtcs) + sizeof(state->cstates)) * ncrtcs;

  ptr = kzalloc(sz, GFP_KERNEL);

@@ -74,6 +76,12 @@ struct drm_atomic_state *drm_atomic_begin(struct drm_device *dev, state->pstates = ptr; ptr = &state->pstates[nplanes];

• state->crtcs = ptr;
• ptr = &state->crtcs[ncrtcs];
• state->cstates = ptr;
• ptr = &state->cstates[ncrtcs];
• return state;

} EXPORT_SYMBOL(drm_atomic_begin); @@ -92,7 +100,18 @@ int drm_atomic_set_event(struct drm_device *dev, struct drm_atomic_state *state, struct drm_mode_object *obj, struct drm_pending_vblank_event *event) {

• return -EINVAL; /* for now */

• switch (obj->type) {
• case DRM_MODE_OBJECT_CRTC: {

• struct drm_crtc_state *cstate =
  

• 	drm_atomic_get_crtc_state(obj_to_crtc(obj), state);
  

• if (IS_ERR(cstate))
  

• 	return PTR_ERR(cstate);
  

• cstate->event = event;
  

• return 0;
  

• }
• default:

• return -EINVAL;
  

• }

Hm, I think if we only want completion events on crtcs (which I agree on)

I don't. Unless you have a nice way of passing some kind of "fbs now available for rendering" list back to userland in the single crtc event. Last time I looked making the drm event stuff deal with variable length events looked more painful than just adding per plane events. But I must admit that I didn't really try to do it.