From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 12/17] drm: convert crtc to properties/state Date: Mon, 26 May 2014 18:23:05 +0300 Message-ID: <20140526152305.GJ27580@intel.com> References: <1400956226-28053-1-git-send-email-robdclark@gmail.com> <1400956226-28053-13-git-send-email-robdclark@gmail.com> <20140526093110.GC14357@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 97C2E6E5A9 for ; Mon, 26 May 2014 08:23:09 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140526093110.GC14357@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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 > = > 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/arm= ada/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 > > #include > > #include > > +#include > > #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 =3D crtc->dev->dev_private; > > struct armada_crtc *dcrtc =3D drm_to_armada_crtc(crtc); > > + struct drm_crtc_state *cstate =3D drm_atomic_get_crtc_state(crtc, sta= te); > > bool update_csc =3D false; > > + int ret =3D 0; > > + > > + if (IS_ERR(cstate)) > > + return PTR_ERR(cstate); > > = > > if (property =3D=3D priv->csc_yuv_prop) { > > dcrtc->csc_yuv_mode =3D val; > > @@ -974,6 +980,9 @@ armada_drm_crtc_set_property(struct drm_crtc *crtc, > > } else if (property =3D=3D priv->csc_rgb_prop) { > > dcrtc->csc_rgb_mode =3D val; > > update_csc =3D true; > > + } else { > > + ret =3D 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 =3D { > > diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_m= ode.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 = =3D { > > .cursor_move =3D ast_cursor_move, > > .reset =3D ast_crtc_reset, > > .set_config =3D drm_crtc_helper_set_config, > > + .set_property =3D drm_atomic_crtc_set_property, > > .gamma_set =3D ast_crtc_gamma_set, > > .destroy =3D ast_crtc_destroy, > > }; > > diff --git a/drivers/gpu/drm/cirrus/cirrus_mode.c b/drivers/gpu/drm/cir= rus/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 *cr= tc) > > static const struct drm_crtc_funcs cirrus_crtc_funcs =3D { > > .gamma_set =3D cirrus_crtc_gamma_set, > > .set_config =3D drm_crtc_helper_set_config, > > + .set_property =3D drm_atomic_crtc_set_property, > > .destroy =3D 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 dr= m_device *dev, > > { > > struct drm_atomic_state *state; > > int nplanes =3D dev->mode_config.num_total_plane; > > + int ncrtcs =3D dev->mode_config.num_crtc; > > int sz; > > void *ptr; > > = > > sz =3D sizeof(*state); > > sz +=3D (sizeof(state->planes) + sizeof(state->pstates)) * nplanes; > > + sz +=3D (sizeof(state->crtcs) + sizeof(state->cstates)) * ncrtcs; > > = > > ptr =3D kzalloc(sz, GFP_KERNEL); > > = > > @@ -74,6 +76,12 @@ struct drm_atomic_state *drm_atomic_begin(struct drm= _device *dev, > > state->pstates =3D ptr; > > ptr =3D &state->pstates[nplanes]; > > = > > + state->crtcs =3D ptr; > > + ptr =3D &state->crtcs[ncrtcs]; > > + > > + state->cstates =3D ptr; > > + ptr =3D &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 =3D > > + drm_atomic_get_crtc_state(obj_to_crtc(obj), state); > > + if (IS_ERR(cstate)) > > + return PTR_ERR(cstate); > > + cstate->event =3D 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=E4l=E4 Intel OTC