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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

-- 
Ville Syrjälä
Intel OTC