From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 1/7] drm: add atomic fxns Date: Thu, 24 Jul 2014 12:02:09 +0200 Message-ID: <20140724100207.GC3811@ulmo.nvidia.com> References: <1406144300-4995-1-git-send-email-robdclark@gmail.com> <1406144300-4995-2-git-send-email-robdclark@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1721342584==" Return-path: Received: from mail-we0-f173.google.com (mail-we0-f173.google.com [74.125.82.173]) by gabe.freedesktop.org (Postfix) with ESMTP id 0C7D76E6CA for ; Thu, 24 Jul 2014 03:02:13 -0700 (PDT) Received: by mail-we0-f173.google.com with SMTP id q58so2470713wes.18 for ; Thu, 24 Jul 2014 03:02:11 -0700 (PDT) In-Reply-To: <1406144300-4995-2-git-send-email-robdclark@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Rob Clark Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1721342584== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="+xNpyl7Qekk2NvDX" Content-Disposition: inline --+xNpyl7Qekk2NvDX Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jul 23, 2014 at 03:38:14PM -0400, Rob Clark wrote: > The 'atomic' mechanism allows for multiple properties to be updated, > checked, and commited atomically. This will be the basis of atomic- > modeset and nuclear-pageflip. >=20 > The basic flow is: >=20 > state =3D dev->atomic_begin(); > for (... one or more ...) > obj->set_property(obj, state, prop, value); > if (dev->atomic_check(state)) > dev->atomic_commit(state); > dev->atomic_end(state); >=20 > The split of check and commit steps is to allow for ioctls with a > test-only flag (which would skip the commit step). >=20 > Signed-off-by: Rob Clark > --- > drivers/gpu/drm/Makefile | 2 +- > drivers/gpu/drm/armada/armada_crtc.c | 3 +- > drivers/gpu/drm/armada/armada_output.c | 3 +- > drivers/gpu/drm/armada/armada_overlay.c | 3 +- > drivers/gpu/drm/ast/ast_drv.c | 6 + > drivers/gpu/drm/ast/ast_drv.h | 1 + > drivers/gpu/drm/cirrus/cirrus_drv.c | 6 + > drivers/gpu/drm/cirrus/cirrus_drv.h | 1 + > drivers/gpu/drm/drm_atomic.c | 274 ++++++++++++++++++++++= ++++++ > drivers/gpu/drm/drm_crtc.c | 149 +++++++++------ > drivers/gpu/drm/drm_modeset_lock.c | 28 +++ > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 4 +- > drivers/gpu/drm/exynos/exynos_drm_drv.c | 7 + > drivers/gpu/drm/exynos/exynos_drm_plane.c | 4 +- > drivers/gpu/drm/gma500/cdv_intel_crt.c | 4 +- > drivers/gpu/drm/gma500/cdv_intel_dp.c | 4 +- > drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 4 +- > drivers/gpu/drm/gma500/cdv_intel_lvds.c | 4 +- > drivers/gpu/drm/gma500/mdfld_dsi_output.c | 4 +- > drivers/gpu/drm/gma500/psb_drv.c | 7 + > drivers/gpu/drm/gma500/psb_drv.h | 1 + > drivers/gpu/drm/gma500/psb_intel_drv.h | 4 +- > drivers/gpu/drm/gma500/psb_intel_lvds.c | 4 +- > drivers/gpu/drm/gma500/psb_intel_sdvo.c | 4 +- > drivers/gpu/drm/i915/i915_drv.c | 8 + > drivers/gpu/drm/i915/intel_crt.c | 4 +- > drivers/gpu/drm/i915/intel_dp.c | 4 +- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_hdmi.c | 4 +- > drivers/gpu/drm/i915/intel_lvds.c | 4 +- > drivers/gpu/drm/i915/intel_sdvo.c | 4 +- > drivers/gpu/drm/i915/intel_tv.c | 6 +- > drivers/gpu/drm/mgag200/mgag200_drv.c | 7 + > drivers/gpu/drm/mgag200/mgag200_drv.h | 1 + > drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 3 +- > drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 3 +- > drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 3 +- > drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 3 +- > drivers/gpu/drm/msm/msm_drv.c | 6 + > drivers/gpu/drm/msm/msm_drv.h | 1 + > drivers/gpu/drm/nouveau/dispnv04/overlay.c | 5 +- > drivers/gpu/drm/nouveau/nouveau_connector.c | 3 +- > drivers/gpu/drm/nouveau/nouveau_drm.c | 7 + > drivers/gpu/drm/nouveau/nouveau_drm.h | 1 + > drivers/gpu/drm/omapdrm/omap_crtc.c | 6 +- > drivers/gpu/drm/omapdrm/omap_drv.c | 6 + > drivers/gpu/drm/omapdrm/omap_drv.h | 4 +- > drivers/gpu/drm/omapdrm/omap_plane.c | 3 +- > drivers/gpu/drm/qxl/qxl_display.c | 4 +- > drivers/gpu/drm/qxl/qxl_drv.c | 9 + > drivers/gpu/drm/radeon/radeon_connectors.c | 9 +- > drivers/gpu/drm/radeon/radeon_drv.c | 9 + > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 7 + > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 4 +- > drivers/gpu/drm/shmobile/shmob_drm_drv.c | 7 + > drivers/gpu/drm/tilcdc/tilcdc_drv.c | 6 + > drivers/gpu/drm/tilcdc/tilcdc_drv.h | 1 + > drivers/gpu/drm/tilcdc/tilcdc_slave.c | 3 +- > drivers/gpu/drm/udl/udl_connector.c | 6 +- > drivers/gpu/drm/udl/udl_drv.c | 8 + > drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 7 + > drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 1 + > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 4 +- > include/drm/drmP.h | 80 ++++++++ > include/drm/drm_atomic.h | 114 ++++++++++++ > include/drm/drm_crtc.h | 15 +- > include/drm/drm_modeset_lock.h | 47 +++++ > include/uapi/drm/drm_mode.h | 3 + > 69 files changed, 873 insertions(+), 103 deletions(-) > create mode 100644 drivers/gpu/drm/drm_atomic.c > create mode 100644 include/drm/drm_atomic.h The above list seems to include only drivers that actually implement properties. But that also means that none of the .atomic_* functions will get set for drivers that lack properties. I'm wondering if that isn't going to cause problems later on. What's the effect of not providing .atomic_*() implementations? Also for the drivers that are modified in this patch the only change seems to be the addition of a new parameter to .set_property(), but that alone surely isn't enough to implement atomic modesetting, right? So I guess my question is: wouldn't it be more useful to consistently set .atomic_* for all drivers? Even if they don't provide any custom properties they'll get some of the standard ones. > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c [...] > +/** > + * drm_atomic_begin - start a sequence of atomic updates > + * @dev: DRM device > + * @flags: the modifier flags that userspace has requested > + * > + * Begin a sequence of atomic property sets. Returns a driver > + * private state object that is passed back into the various > + * object's set_property() fxns, and into the remainder of the > + * atomic funcs. The state object should accumulate the changes > + * from one o more set_property()'s. At the end, the state can > + * be checked, and optionally committed. > + * > + * RETURNS > + * a driver state object, which is passed back in to the > + * various other atomic fxns, or error (such as -EBUSY if > + * there is still a pending async update) > + */ This section should be "Return: a driver state object ..." according to Documentation/kernel-doc-nano-HOWTO.txt. > +struct drm_atomic_state *drm_atomic_begin(struct drm_device *dev, > + uint32_t flags) Nit: I think we usually try to align arguments on subsequent lines with the first argument on the first line. > +{ > + struct drm_atomic_state *state; > + uint32_t acquire_flags =3D 0; > + int sz; > + void *ptr; > + > + sz =3D sizeof(*state); > + > + ptr =3D kzalloc(sz, GFP_KERNEL); Shouldn't this error out on ptr =3D=3D NULL? > + > + state =3D ptr; > + ptr =3D &state[1]; I'm not exactly sure what this is doing. ptr doesn't seem to be used anymore, so the last assignment becomes unnecessary. It also seems wrong since ptr now points to unallocated memory. And if that assignment is removed, then state =3D ptr isn't necessary either anymore, since you could simply do: state =3D kzalloc(sz, GFP_KERNEL); But maybe I'm missing something? > +static void grab_locks(struct drm_atomic_state *a, > + struct ww_acquire_ctx *ww_ctx) > +{ > + struct drm_modeset_acquire_ctx *ctx =3D &a->acquire_ctx; > + struct drm_modeset_lock *lock, *slow_locked, *contended; > + int ret; > + > + lock =3D slow_locked =3D contended =3D NULL; > + > + There's a gratuituous newline here. > +static void commit_locks(struct drm_atomic_state *a, > + struct ww_acquire_ctx *ww_ctx) > +{ > + /* and properly release them (clear in_atomic, remove from list): */ > + drm_modeset_drop_locks(&a->acquire_ctx); > + ww_acquire_fini(ww_ctx); > + a->committed =3D true; > +} > + > +static int atomic_commit(struct drm_atomic_state *a, > + struct ww_acquire_ctx *ww_ctx) > +{ > + int ret =3D 0; > + > + commit_locks(a, ww_ctx); > + > + return ret; > +} These look somewhat unnecessary, but I suspect that subsequent patches will flesh them out. > +void _drm_atomic_state_free(struct kref *kref) > +{ > + struct drm_atomic_state *a =3D > + container_of(kref, struct drm_atomic_state, refcount); > + > + /* in case we haven't already: */ > + if (!a->committed) { > + grab_locks(a, &a->acquire_ctx.ww_ctx); > + commit_locks(a, &a->acquire_ctx.ww_ctx); > + } > + > + __drm_modeset_acquire_fini(&a->acquire_ctx); > + > + kfree(a); > +} > +EXPORT_SYMBOL(_drm_atomic_state_free); > + > + There's another gratuituous blank line above. > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c [...] > -static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj, > - struct drm_property *property, > - uint64_t value) > +static int drm_mode_connector_set_obj_prop(struct drm_connector *connect= or, > + struct drm_atomic_state *state, struct drm_property *property, > + uint64_t value, void *blob_data) > { > int ret =3D -EINVAL; > - struct drm_connector *connector =3D obj_to_connector(obj); > =20 > /* Do DPMS ourselves */ > if (property =3D=3D connector->dev->mode_config.dpms_property) { > if (connector->funcs->dpms) > (*connector->funcs->dpms)(connector, (int)value); > ret =3D 0; > - } else if (connector->funcs->set_property) > - ret =3D connector->funcs->set_property(connector, property, value); > + } else if (connector->funcs->set_property) { > + ret =3D connector->funcs->set_property(connector, state, > + property, value, blob_data); > + } Why the extra braces here? There's still only one statement in the block. > /** > - * drm_mode_getproperty_ioctl - get the current value of a object's prop= erty > + * drm_mode_obj_get_properties_ioctl - get the current value of a object= 's property > * @dev: DRM device > * @data: ioctl data > * @file_priv: DRM file info This isn't really introduced by this patch, but isn't this kerneldoc comment wrong? drm_mode_obj_get_properties_ioctl() seems to return the values of all properties of an object rather than just one. Thierry --+xNpyl7Qekk2NvDX Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJT0NmfAAoJEN0jrNd/PrOhR4gP/3irHXvcFHaOVMdvzlUM3RU2 GWGZQHcZq0Rh2LCdPRe6XPhneNjJmPCgaIFt+dMMPiYFXZNGvnKEv6VZD04frwom 47IIuExaE5vGhc5gVEKInpaPnxbnhWhTPOA+4V5jiQmXFG6tvgaFcT11oW7G0IbA HXURHTAUcNxQ5RFCQ5zx0rxuPu41THeU/VCFFnnwW1ldfuU1/sF18gZFkzb45U5n EU2U6AvGUTorQCr8CWEudZqjBvVEgguMebp0FYL4h29Imr8OJXShkxKi5Fj4E7iK ZXiGMCTZaonqu0zTSg4XcKVwXK1ksEcGxzi11mJpn8z9N7GMxhH2iJMADvMBjUrX p6N6soQ/d5+sLe8FXEm2eJGKeGnqdq+DK+nOfRpVz/2pJNuCmr0cWt3yEiYZgkik qkxTCurzVYwgYXTExoPFRyuwSUW0PnVGxbCTj1CmkMIOLDRifIIlEbSja6ogqaHy Boenbg+b2fHDppXj42taJM3kVo0DK+WbkQqJhLQJDGK7Qh6CjH4IWew0xBITiEI9 Dlx26eGNr0Wk9UeYa10mmj0rdgC1+JppXLoijWuEUNjtzDF8cEpd32AOQik5WJs7 2jE6NFAnJG/sBO7xDgeBs863NxCaX3PGeOMPHEHxW90WmGj62hwaWk2Wi1cqPlt3 A+enLPcAumYlvffFZyCc =oks+ -----END PGP SIGNATURE----- --+xNpyl7Qekk2NvDX-- --===============1721342584== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============1721342584==--