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. > > The basic flow is: > > state = 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); > > The split of check and commit steps is to allow for ioctls with a > test-only flag (which would skip the commit step). > > Signed-off-by: Rob Clark <robdclark@xxxxxxxxx> > --- > 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 = 0; > + int sz; > + void *ptr; > + > + sz = sizeof(*state); > + > + ptr = kzalloc(sz, GFP_KERNEL); Shouldn't this error out on ptr == NULL? > + > + state = ptr; > + ptr = &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 = ptr isn't necessary either anymore, since you could simply do: state = 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 = &a->acquire_ctx; > + struct drm_modeset_lock *lock, *slow_locked, *contended; > + int ret; > + > + lock = slow_locked = contended = 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 = true; > +} > + > +static int atomic_commit(struct drm_atomic_state *a, > + struct ww_acquire_ctx *ww_ctx) > +{ > + int ret = 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 = > + 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 *connector, > + struct drm_atomic_state *state, struct drm_property *property, > + uint64_t value, void *blob_data) > { > int ret = -EINVAL; > - struct drm_connector *connector = obj_to_connector(obj); > > /* Do DPMS ourselves */ > if (property == connector->dev->mode_config.dpms_property) { > if (connector->funcs->dpms) > (*connector->funcs->dpms)(connector, (int)value); > ret = 0; > - } else if (connector->funcs->set_property) > - ret = connector->funcs->set_property(connector, property, value); > + } else if (connector->funcs->set_property) { > + ret = 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 property > + * 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
Attachment:
pgpf9bLdUXAzb.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel