Re: [PATCH 1/7] drm: add atomic fxns

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 10296 bytes --]

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@gmail.com>
> ---
>  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 #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel