Re: [PATCH 05/10] drm/i915: Add main atomic entrypoints (v2)

[Ander Conselvan de Oliveira <conselvan2@gmail.com>]

On 01/22/2015 02:35 AM, Matt Roper wrote:
> Add the top-level atomic entrypoints for check/commit.  These won't get
> called yet; we still need to either enable the atomic ioctl or switch to
> using the non-transitional atomic helpers for legacy operations.
>
> v2:
>   - Use plane->pipe rather than plane->possible_crtcs while ensuring that
>     only a single CRTC is in use.  Either way will work fine since i915
>     drm_plane's are always tied to a single CRTC, but plane->pipe is
>     slightly more intuitive. (Ander)
>   - Simplify crtc/connector checking logic. (Ander)
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/Makefile        |   1 +
>   drivers/gpu/drm/i915/intel_atomic.c  | 164 +++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_display.c |   2 +
>   drivers/gpu/drm/i915/intel_drv.h     |   7 ++
>   4 files changed, 174 insertions(+)
>   create mode 100644 drivers/gpu/drm/i915/intel_atomic.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 16e3dc3..c7e2ab5 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -66,6 +66,7 @@ i915-y += dvo_ch7017.o \
>   	  dvo_ns2501.o \
>   	  dvo_sil164.o \
>   	  dvo_tfp410.o \
> +	  intel_atomic.o \
>   	  intel_atomic_plane.o \
>   	  intel_crt.o \
>   	  intel_ddi.o \
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> new file mode 100644
> index 0000000..5c31f54
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -0,0 +1,164 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +/**
> + * DOC: atomic modeset support
> + *
> + * The functions here implement the state management and hardware programming
> + * dispatch required by the atomic modeset infrastructure.
> + * See intel_atomic_plane.c for the plane-specific atomic functionality.
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_plane_helper.h>
> +#include "intel_drv.h"
> +
> +
> +/**
> + * intel_atomic_check - validate state object
> + * @dev: drm device
> + * @state: state to validate
> + */
> +int intel_atomic_check(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 nconnectors = dev->mode_config.num_connector;
> +	enum pipe nuclear_pipe = INVALID_PIPE;
> +	int ret;
> +	int i;
> +	bool not_nuclear = false;
> +
> +	/*
> +	 * FIXME:  At the moment, we only support "nuclear pageflip" on a
> +	 * single CRTC.  Cross-crtc updates will be added later.
> +	 */
> +	for (i = 0; i < nplanes; i++) {
> +		struct intel_plane *plane = to_intel_plane(state->planes[i]);
> +		if (!plane)
> +			continue;
> +
> +		if (nuclear_pipe == INVALID_PIPE) {
> +			nuclear_pipe = plane->pipe;
> +		} else if (nuclear_pipe != plane->pipe) {
> +			DRM_DEBUG_KMS("i915 only support atomic plane operations on a single CRTC at the moment\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/*
> +	 * FIXME:  We only handle planes for now; make sure there are no CRTC's
> +	 * or connectors involved.
> +	 */
> +	state->allow_modeset = false;
> +	for (i = 0; i < ncrtcs; i++) {
> +		struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
> +		if (crtc && crtc->pipe != nuclear_pipe)
> +			not_nuclear = true;
> +	}
> +	for (i = 0; i < nconnectors; i++)
> +		if (state->connectors[i] != NULL)
> +			not_nuclear = true;
> +
> +	if (not_nuclear) {
> +		DRM_DEBUG_KMS("i915 only supports atomic plane operations at the moment\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = drm_atomic_helper_check_planes(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	return ret;
> +}
> +
> +
> +/**
> + * intel_atomic_commit - commit validated state object
> + * @dev: DRM device
> + * @state: the top-level driver state object
> + * @async: asynchronous commit
> + *
> + * This function commits a top-level state object that has been validated
> + * with drm_atomic_helper_check().
> + *
> + * FIXME:  Atomic modeset support for i915 is not yet complete.  At the moment
> + * we can only handle plane-related operations and do not yet support
> + * asynchronous commit.
> + *
> + * RETURNS
> + * Zero for success or -errno.
> + */
> +int intel_atomic_commit(struct drm_device *dev,
> +			struct drm_atomic_state *state,
> +			bool async)
> +{
> +	int ret;
> +	int i;
> +
> +	if (async) {
> +		DRM_DEBUG_KMS("i915 does not yet support async commit\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = drm_atomic_helper_prepare_planes(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	/* Point of no return */
> +
> +	/*
> +	 * FIXME:  The proper sequence here will eventually be:
> +	 *
> +	 * drm_atomic_helper_swap_state(dev, state)
> +	 * drm_atomic_helper_commit_pre_planes(dev, state);
> +	 * drm_atomic_helper_commit_planes(dev, state);
> +	 * drm_atomic_helper_commit_post_planes(dev, state);
> +	 * drm_atomic_helper_wait_for_vblanks(dev, state);
> +	 * drm_atomic_helper_cleanup_planes(dev, state);
> +	 * drm_atomic_state_free(state);
> +	 *
> +	 * once we have full atomic modeset.  For now, just manually update
> +	 * plane states to avoid clobbering good states with dummy states
> +	 * while nuclear pageflipping.
> +	 */
> +	for (i = 0; i < dev->mode_config.num_total_plane; i++) {
> +		struct drm_plane *plane = state->planes[i];
> +
> +		if (!plane)
> +			continue;
> +
> +		plane->state->state = state;
> +		swap(state->plane_states[i], plane->state);
> +		plane->state->state = NULL;
> +	}
> +	drm_atomic_helper_commit_planes(dev, state);
> +	drm_atomic_helper_wait_for_vblanks(dev, state);

Isn't it possible that we wait for two vblanks here? One in 
intel_finish_crtc_commit() and then again in 
drm_atomic_helper_wait_for_vblanks().

That would happen when disabling planes, or enabling the primary plane 
in BDW. The former will be solved by fixing the watermark code and, if I 
understand correctly, the wait in drm_atomic_helper_wait_for_vblanks()
will always cover the BDW case. So I guess we can leave it this way for 
now, and fix this with a follow up patch once we're completely atomic.

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>


> +	drm_atomic_helper_cleanup_planes(dev, state);
> +	drm_atomic_state_free(state);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 047c77e..f557f2d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12752,6 +12752,8 @@ static inline void intel_fbdev_output_poll_changed(struct drm_device *dev)
>   static const struct drm_mode_config_funcs intel_mode_funcs = {
>   	.fb_create = intel_user_framebuffer_create,
>   	.output_poll_changed = intel_fbdev_output_poll_changed,
> +	.atomic_check = intel_atomic_check,
> +	.atomic_commit = intel_atomic_commit,
>   };
>
>   /* Set up chip specific display functions */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 24c63fb..833f44c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1244,6 +1244,13 @@ void intel_pre_disable_primary(struct drm_crtc *crtc);
>   void intel_tv_init(struct drm_device *dev);
>
>   /* intel_atomic.c */
> +int intel_atomic_check(struct drm_device *dev,
> +		       struct drm_atomic_state *state);
> +int intel_atomic_commit(struct drm_device *dev,
> +			struct drm_atomic_state *state,
> +			bool async);
> +
> +/* intel_atomic_plane.c */
>   struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
>   struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
>   void intel_plane_destroy_state(struct drm_plane *plane,
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx