[Intel-gfx] [PATCH v3 17/19] drm/i915: Make setting color key atomic.

Matt Roper matthew.d.roper at intel.com
Thu Jun 18 07:21:23 PDT 2015


On Mon, Jun 15, 2015 at 12:33:54PM +0200, Maarten Lankhorst wrote:
> By making color key atomic there are no more transitional helpers.
> The plane check function will reject the color key when a scaler is
> active.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c |  1 +
>  drivers/gpu/drm/i915/intel_display.c      |  7 ++-
>  drivers/gpu/drm/i915/intel_drv.h          |  6 +--
>  drivers/gpu/drm/i915/intel_sprite.c       | 85 +++++++++++++++----------------
>  4 files changed, 46 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 91d53768df9d..10a8ecedc942 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -56,6 +56,7 @@ intel_create_plane_state(struct drm_plane *plane)
>  
>  	state->base.plane = plane;
>  	state->base.rotation = BIT(DRM_ROTATE_0);
> +	state->ckey.flags = I915_SET_COLORKEY_NONE;
>  
>  	return state;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5facd0501a34..746c73d2ab84 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4401,9 +4401,9 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
>  		return ret;
>  
>  	/* check colorkey */
> -	if (WARN_ON(intel_plane->ckey.flags != I915_SET_COLORKEY_NONE)) {
> +	if (plane_state->ckey.flags != I915_SET_COLORKEY_NONE) {
>  		DRM_DEBUG_KMS("[PLANE:%d] scaling with color key not allowed",
> -			intel_plane->base.base.id);
> +			      intel_plane->base.base.id);
>  		return -EINVAL;
>  	}
>  
> @@ -13733,7 +13733,7 @@ intel_check_primary_plane(struct drm_plane *plane,
>  
>  	/* use scaler when colorkey is not required */
>  	if (INTEL_INFO(plane->dev)->gen >= 9 &&
> -	    to_intel_plane(plane)->ckey.flags == I915_SET_COLORKEY_NONE) {
> +	    state->ckey.flags == I915_SET_COLORKEY_NONE) {
>  		min_scale = 1;
>  		max_scale = skl_max_scale(to_intel_crtc(crtc), crtc_state);
>  		can_position = true;
> @@ -13881,7 +13881,6 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  	primary->check_plane = intel_check_primary_plane;
>  	primary->commit_plane = intel_commit_primary_plane;
>  	primary->disable_plane = intel_disable_primary_plane;
> -	primary->ckey.flags = I915_SET_COLORKEY_NONE;
>  	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
>  		primary->plane = !pipe;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 93b9542ab8dc..3a2ac82b0970 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -274,6 +274,8 @@ struct intel_plane_state {
>  	 *     update_scaler_plane.
>  	 */
>  	int scaler_id;
> +
> +	struct drm_intel_sprite_colorkey ckey;
>  };
>  
>  struct intel_initial_plane_config {
> @@ -588,9 +590,6 @@ struct intel_plane {
>  	bool can_scale;
>  	int max_downscale;
>  
> -	/* FIXME convert to properties */
> -	struct drm_intel_sprite_colorkey ckey;
> -
>  	/* Since we need to change the watermarks before/after
>  	 * enabling/disabling the planes, we need to store the parameters here
>  	 * as the other pieces of the struct may not reflect the values we want
> @@ -1390,7 +1389,6 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
>  
>  /* intel_sprite.c */
>  int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
> -int intel_plane_restore(struct drm_plane *plane);
>  int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  			      struct drm_file *file_priv);
>  bool intel_pipe_update_start(struct intel_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 168f90f346c2..21d3f7882c4d 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -182,7 +182,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	const int plane = intel_plane->plane + 1;
>  	u32 plane_ctl, stride_div, stride;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> -	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
> +	const struct drm_intel_sprite_colorkey *key =
> +		&to_intel_plane_state(drm_plane->state)->ckey;
>  	unsigned long surf_addr;
>  	u32 tile_height, plane_offset, plane_size;
>  	unsigned int rotation;
> @@ -344,7 +345,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  	u32 sprctl;
>  	unsigned long sprsurf_offset, linear_offset;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> -	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
> +	const struct drm_intel_sprite_colorkey *key =
> +		&to_intel_plane_state(dplane->state)->ckey;
>  
>  	sprctl = SP_ENABLE;
>  
> @@ -487,7 +489,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	u32 sprctl, sprscale = 0;
>  	unsigned long sprsurf_offset, linear_offset;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> -	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
> +	const struct drm_intel_sprite_colorkey *key =
> +		&to_intel_plane_state(plane->state)->ckey;
>  
>  	sprctl = SPRITE_ENABLE;
>  
> @@ -627,7 +630,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	unsigned long dvssurf_offset, linear_offset;
>  	u32 dvscntr, dvsscale;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> -	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
> +	const struct drm_intel_sprite_colorkey *key =
> +		&to_intel_plane_state(plane->state)->ckey;
>  
>  	dvscntr = DVS_ENABLE;
>  
> @@ -778,7 +782,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	/* setup can_scale, min_scale, max_scale */
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		/* use scaler when colorkey is not required */
> -		if (intel_plane->ckey.flags == I915_SET_COLORKEY_NONE) {
> +		if (state->ckey.flags == I915_SET_COLORKEY_NONE) {
>  			can_scale = 1;
>  			min_scale = 1;
>  			max_scale = skl_max_scale(intel_crtc, crtc_state);
> @@ -798,7 +802,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	 * coordinates and sizes. We probably need some way to decide whether
>  	 * more strict checking should be done instead.
>  	 */
> -
>  	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
>  			state->base.rotation);
>  
> @@ -808,7 +811,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
>  	BUG_ON(vscale < 0);
>  
> -	state->visible =  drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
> +	state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
>  
>  	crtc_x = dst->x1;
>  	crtc_y = dst->y1;
> @@ -950,7 +953,9 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  {
>  	struct drm_intel_sprite_colorkey *set = data;
>  	struct drm_plane *plane;
> -	struct intel_plane *intel_plane;
> +	struct drm_plane_state *plane_state;
> +	struct drm_atomic_state *state;
> +	struct drm_modeset_acquire_ctx ctx;
>  	int ret = 0;

Can we simplify this by just calling
drm_atomic_helper_plane_set_property() and let it handle the atomic
transaction for us?


Matt

>  
>  	/* Make sure we don't try to enable both src & dest simultaneously */
> @@ -961,50 +966,41 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  	    set->flags & I915_SET_COLORKEY_DESTINATION)
>  		return -EINVAL;
>  
> -	drm_modeset_lock_all(dev);
> -
>  	plane = drm_plane_find(dev, set->plane_id);
> -	if (!plane || plane->type != DRM_PLANE_TYPE_OVERLAY) {
> -		ret = -ENOENT;
> -		goto out_unlock;
> -	}
> +	if (!plane || plane->type != DRM_PLANE_TYPE_OVERLAY)
> +		return -ENOENT;
>  
> -	intel_plane = to_intel_plane(plane);
> +	drm_modeset_acquire_init(&ctx, 0);
>  
> -	if (INTEL_INFO(dev)->gen >= 9) {
> -		/* plane scaling and colorkey are mutually exclusive */
> -		if (to_intel_plane_state(plane->state)->scaler_id >= 0) {
> -			DRM_ERROR("colorkey not allowed with scaler\n");
> -			ret = -EINVAL;
> -			goto out_unlock;
> -		}
> +	state = drm_atomic_state_alloc(plane->dev);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto out;
>  	}
> +	state->acquire_ctx = &ctx;
> +
> +	while (1) {
> +		plane_state = drm_atomic_get_plane_state(state, plane);
> +		ret = PTR_ERR_OR_ZERO(plane_state);
> +		if (!ret) {
> +			to_intel_plane_state(plane_state)->ckey = *set;
> +			ret = drm_atomic_commit(state);
> +		}
>  
> -	intel_plane->ckey = *set;
> -
> -	/*
> -	 * The only way this could fail would be due to
> -	 * the current plane state being unsupportable already,
> -	 * and we dont't consider that an error for the
> -	 * colorkey ioctl. So just ignore any error.
> -	 */
> -	intel_plane_restore(plane);
> +		if (ret != -EDEADLK)
> +			break;
>  
> -out_unlock:
> -	drm_modeset_unlock_all(dev);
> -	return ret;
> -}
> +		drm_atomic_state_clear(state);
> +		drm_modeset_backoff(&ctx);
> +	}
>  
> -int intel_plane_restore(struct drm_plane *plane)
> -{
> -	if (!plane->crtc || !plane->state->fb)
> -		return 0;
> +	if (ret)
> +		drm_atomic_state_free(state);
>  
> -	return drm_plane_helper_update(plane, plane->crtc, plane->state->fb,
> -				       plane->state->crtc_x, plane->state->crtc_y,
> -				       plane->state->crtc_w, plane->state->crtc_h,
> -				       plane->state->src_x, plane->state->src_y,
> -				       plane->state->src_w, plane->state->src_h);
> +out:
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +	return ret;
>  }
>  
>  static const uint32_t ilk_plane_formats[] = {
> @@ -1133,7 +1129,6 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  	intel_plane->plane = plane;
>  	intel_plane->check_plane = intel_check_sprite_plane;
>  	intel_plane->commit_plane = intel_commit_sprite_plane;
> -	intel_plane->ckey.flags = I915_SET_COLORKEY_NONE;
>  	possible_crtcs = (1 << pipe);
>  	ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
>  				       &intel_plane_funcs,
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795


More information about the Intel-gfx mailing list