Re: [PATCH 02/13] drm/i915: Move rotation from intel_plane to intel_plane_state

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

On 01/20/2015 05:57 AM, Matt Roper wrote:
> Runtime state that can be manipulated via properties should now go in
> intel_plane_state instead so that it can be tracked as part of an atomic
> transaction.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 36 ++++++++++++++++++++++++------------
>   drivers/gpu/drm/i915/intel_drv.h     | 10 +++++++++-
>   drivers/gpu/drm/i915/intel_fbc.c     |  4 +++-
>   drivers/gpu/drm/i915/intel_sprite.c  | 33 ++++++++++++++++++++-------------
>   4 files changed, 56 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c5cbcd7..7863414 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2435,6 +2435,8 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>   {
>   	struct drm_device *dev = crtc->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane_state *state =
> +		to_intel_plane_state(crtc->primary->state);
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	struct drm_i915_gem_object *obj;
>   	int plane = intel_crtc->plane;
> @@ -2532,7 +2534,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>   		intel_crtc->dspaddr_offset = linear_offset;
>   	}
>
> -	if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180)) {
> +	if (state->rotation == BIT(DRM_ROTATE_180)) {
>   		dspcntr |= DISPPLANE_ROTATE_180;
>
>   		x += (intel_crtc->config->pipe_src_w - 1);
> @@ -2568,6 +2570,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>   	struct drm_device *dev = crtc->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane_state *state =
> +		to_intel_plane_state(crtc->primary->state);
>   	struct drm_i915_gem_object *obj;
>   	int plane = intel_crtc->plane;
>   	unsigned long linear_offset;
> @@ -2634,7 +2638,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>   					       pixel_size,
>   					       fb->pitches[0]);
>   	linear_offset -= intel_crtc->dspaddr_offset;
> -	if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180)) {
> +	if (state->rotation == BIT(DRM_ROTATE_180)) {
>   		dspcntr |= DISPPLANE_ROTATE_180;
>
>   		if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
> @@ -2673,6 +2677,8 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>   	struct drm_device *dev = crtc->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane_state *state =
> +		to_intel_plane_state(crtc->primary->state);
>   	struct intel_framebuffer *intel_fb;
>   	struct drm_i915_gem_object *obj;
>   	int pipe = intel_crtc->pipe;
> @@ -2731,7 +2737,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>   	}
>
>   	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> -	if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180))
> +	if (state->rotation == BIT(DRM_ROTATE_180))
>   		plane_ctl |= PLANE_CTL_ROTATE_180;
>
>   	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> @@ -8216,6 +8222,8 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>   	struct drm_device *dev = crtc->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane_state *state =
> +		to_intel_plane_state(crtc->cursor->state);
>   	int pipe = intel_crtc->pipe;
>   	uint32_t cntl;
>
> @@ -8242,7 +8250,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>   			cntl |= CURSOR_PIPE_CSC_ENABLE;
>   	}
>
> -	if (to_intel_plane(crtc->cursor)->rotation == BIT(DRM_ROTATE_180))
> +	if (state->rotation == BIT(DRM_ROTATE_180))
>   		cntl |= CURSOR_ROTATE_180;
>
>   	if (intel_crtc->cursor_cntl != cntl) {
> @@ -8265,6 +8273,8 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>   	struct drm_device *dev = crtc->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane_state *state =
> +		to_intel_plane_state(crtc->cursor->state);
>   	int pipe = intel_crtc->pipe;
>   	int x = crtc->cursor_x;
>   	int y = crtc->cursor_y;
> @@ -8303,8 +8313,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>   	I915_WRITE(CURPOS(pipe), pos);
>
>   	/* ILK+ do this automagically */
> -	if (HAS_GMCH_DISPLAY(dev) &&
> -		to_intel_plane(crtc->cursor)->rotation == BIT(DRM_ROTATE_180)) {
> +	if (HAS_GMCH_DISPLAY(dev) && state->rotation == BIT(DRM_ROTATE_180)) {
>   		base += (intel_crtc->cursor_height *
>   			intel_crtc->cursor_width - 1) * 4;
>   	}
> @@ -11756,7 +11765,6 @@ intel_check_primary_plane(struct drm_plane *plane,
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct drm_crtc *crtc = state->base.crtc;
>   	struct intel_crtc *intel_crtc;
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_framebuffer *fb = state->base.fb;
>   	struct drm_rect *dest = &state->dst;
>   	struct drm_rect *src = &state->src;
> @@ -11790,7 +11798,7 @@ intel_check_primary_plane(struct drm_plane *plane,
>   		if (intel_crtc->primary_enabled &&
>   		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
>   		    dev_priv->fbc.plane == intel_crtc->plane &&
> -		    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
> +		    state->rotation != BIT(DRM_ROTATE_0)) {
>   			intel_crtc->atomic.disable_fbc = true;
>   		}
>
> @@ -11974,6 +11982,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>   						    int pipe)
>   {
>   	struct intel_plane *primary;
> +	struct intel_plane_state *state;
>   	const uint32_t *intel_primary_formats;
>   	int num_formats;
>
> @@ -11986,12 +11995,13 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>   		kfree(primary);
>   		return NULL;
>   	}
> +	state = to_intel_plane_state(primary->base.state);
>
>   	primary->can_scale = false;
>   	primary->max_downscale = 1;
>   	primary->pipe = pipe;
>   	primary->plane = pipe;
> -	primary->rotation = BIT(DRM_ROTATE_0);
> +	state->rotation = BIT(DRM_ROTATE_0);

I'm thinking it would be better to split the part of 
intel_plane_duplicate_state() that allocates the state into a separate 
function and do this there. That way there is a clear place to 
initialize state default values like this.

Ander



>   	primary->check_plane = intel_check_primary_plane;
>   	primary->commit_plane = intel_commit_primary_plane;
>   	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> @@ -12019,7 +12029,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>   		if (dev->mode_config.rotation_property)
>   			drm_object_attach_property(&primary->base.base,
>   				dev->mode_config.rotation_property,
> -				primary->rotation);
> +				state->rotation);
>   	}
>
>   	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> @@ -12147,6 +12157,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>   						   int pipe)
>   {
>   	struct intel_plane *cursor;
> +	struct intel_plane_state *state;
>
>   	cursor = kzalloc(sizeof(*cursor), GFP_KERNEL);
>   	if (cursor == NULL)
> @@ -12157,12 +12168,13 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>   		kfree(cursor);
>   		return NULL;
>   	}
> +	state = to_intel_plane_state(cursor->base.state);
>
>   	cursor->can_scale = false;
>   	cursor->max_downscale = 1;
>   	cursor->pipe = pipe;
>   	cursor->plane = pipe;
> -	cursor->rotation = BIT(DRM_ROTATE_0);
> +	state->rotation = BIT(DRM_ROTATE_0);
>   	cursor->check_plane = intel_check_cursor_plane;
>   	cursor->commit_plane = intel_commit_cursor_plane;
>
> @@ -12181,7 +12193,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>   		if (dev->mode_config.rotation_property)
>   			drm_object_attach_property(&cursor->base.base,
>   				dev->mode_config.rotation_property,
> -				cursor->rotation);
> +				state->rotation);
>   	}
>
>   	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c8c0b7f..918cce2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -250,6 +250,9 @@ struct intel_plane_state {
>   	struct drm_rect clip;
>   	bool visible;
>
> +	/* Intel-specific plane properties */
> +	unsigned int rotation;
> +
>   	/*
>   	 * used only for sprite planes to determine when to implicitly
>   	 * enable/disable the primary plane
> @@ -509,7 +512,6 @@ struct intel_plane {
>   	struct drm_i915_gem_object *obj;
>   	bool can_scale;
>   	int max_downscale;
> -	unsigned int rotation;
>
>   	/* Since we need to change the watermarks before/after
>   	 * enabling/disabling the planes, we need to store the parameters here
> @@ -518,6 +520,12 @@ struct intel_plane {
>   	 */
>   	struct intel_plane_wm_parameters wm;
>
> +	/*
> +	 * NOTE: Do not place new plane state fields here (e.g., when adding
> +	 * new plane properties).  New runtime state should now be placed in
> +	 * the intel_plane_state structure and accessed via drm_plane->state.
> +	 */
> +
>   	void (*update_plane)(struct drm_plane *plane,
>   			     struct drm_crtc *crtc,
>   			     struct drm_framebuffer *fb,
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 5b1d7c4..3e87454 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -496,6 +496,7 @@ void intel_fbc_update(struct drm_device *dev)
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct drm_crtc *crtc = NULL, *tmp_crtc;
>   	struct intel_crtc *intel_crtc;
> +	struct intel_plane_state *primary_state;
>   	struct drm_framebuffer *fb;
>   	struct drm_i915_gem_object *obj;
>   	const struct drm_display_mode *adjusted_mode;
> @@ -540,6 +541,7 @@ void intel_fbc_update(struct drm_device *dev)
>   	}
>
>   	intel_crtc = to_intel_crtc(crtc);
> +	primary_state = to_intel_plane_state(crtc->primary->state);
>   	fb = crtc->primary->fb;
>   	obj = intel_fb_obj(fb);
>   	adjusted_mode = &intel_crtc->config->base.adjusted_mode;
> @@ -595,7 +597,7 @@ void intel_fbc_update(struct drm_device *dev)
>   		goto out_disable;
>   	}
>   	if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> -	    to_intel_plane(crtc->primary)->rotation != BIT(DRM_ROTATE_0)) {
> +	    primary_state->rotation != BIT(DRM_ROTATE_0)) {
>   		if (set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE))
>   			DRM_DEBUG_KMS("Rotation unsupported, disabling\n");
>   		goto out_disable;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0a6094e..140c5b7 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -187,6 +187,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>   	struct drm_device *dev = drm_plane->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
> +	struct intel_plane_state *state =
> +		to_intel_plane_state(drm_plane->state);
>   	const int pipe = intel_plane->pipe;
>   	const int plane = intel_plane->plane + 1;
>   	u32 plane_ctl, stride;
> @@ -256,7 +258,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>   	default:
>   		BUG();
>   	}
> -	if (intel_plane->rotation == BIT(DRM_ROTATE_180))
> +	if (state->rotation == BIT(DRM_ROTATE_180))
>   		plane_ctl |= PLANE_CTL_ROTATE_180;
>
>   	plane_ctl |= PLANE_CTL_ENABLE;
> @@ -407,6 +409,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>   	struct drm_device *dev = dplane->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_plane *intel_plane = to_intel_plane(dplane);
> +	struct intel_plane_state *state = to_intel_plane_state(dplane->state);
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	int pipe = intel_plane->pipe;
>   	int plane = intel_plane->plane;
> @@ -493,7 +496,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>   							fb->pitches[0]);
>   	linear_offset -= sprsurf_offset;
>
> -	if (intel_plane->rotation == BIT(DRM_ROTATE_180)) {
> +	if (state->rotation == BIT(DRM_ROTATE_180)) {
>   		sprctl |= SP_ROTATE_180;
>
>   		x += src_w;
> @@ -608,6 +611,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   	struct drm_device *dev = plane->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_plane_state *state = to_intel_plane_state(plane->state);
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	int pipe = intel_plane->pipe;
>   	u32 sprctl, sprscale = 0;
> @@ -684,7 +688,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   					       pixel_size, fb->pitches[0]);
>   	linear_offset -= sprsurf_offset;
>
> -	if (intel_plane->rotation == BIT(DRM_ROTATE_180)) {
> +	if (state->rotation == BIT(DRM_ROTATE_180)) {
>   		sprctl |= SPRITE_ROTATE_180;
>
>   		/* HSW and BDW does this automagically in hardware */
> @@ -813,6 +817,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   	struct drm_device *dev = plane->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_plane_state *state = to_intel_plane_state(plane->state);
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	int pipe = intel_plane->pipe;
>   	unsigned long dvssurf_offset, linear_offset;
> @@ -884,7 +889,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   					       pixel_size, fb->pitches[0]);
>   	linear_offset -= dvssurf_offset;
>
> -	if (intel_plane->rotation == BIT(DRM_ROTATE_180)) {
> +	if (state->rotation == BIT(DRM_ROTATE_180)) {
>   		dvscntr |= DVS_ROTATE_180;
>
>   		x += src_w;
> @@ -1125,7 +1130,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>   	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
>
>   	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> -			intel_plane->rotation);
> +			state->rotation);
>
>   	hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale);
>   	BUG_ON(hscale < 0);
> @@ -1166,7 +1171,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>   				     drm_rect_height(dst) * vscale - drm_rect_height(src));
>
>   		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
> -				    intel_plane->rotation);
> +				    state->rotation);
>
>   		/* sanity check to make sure the src viewport wasn't enlarged */
>   		WARN_ON(src->x1 < (int) state->base.src_x ||
> @@ -1367,7 +1372,7 @@ int intel_plane_set_property(struct drm_plane *plane,
>   			     uint64_t val)
>   {
>   	struct drm_device *dev = plane->dev;
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_plane_state *state = to_intel_plane_state(plane->state);
>   	uint64_t old_val;
>   	int ret = -ENOENT;
>
> @@ -1376,14 +1381,14 @@ int intel_plane_set_property(struct drm_plane *plane,
>   		if (hweight32(val & 0xf) != 1)
>   			return -EINVAL;
>
> -		if (intel_plane->rotation == val)
> +		if (state->rotation == val)
>   			return 0;
>
> -		old_val = intel_plane->rotation;
> -		intel_plane->rotation = val;
> +		old_val = state->rotation;
> +		state->rotation = val;
>   		ret = intel_plane_restore(plane);
>   		if (ret)
> -			intel_plane->rotation = old_val;
> +			state->rotation = old_val;
>   	}
>
>   	return ret;
> @@ -1457,6 +1462,7 @@ int
>   intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>   {
>   	struct intel_plane *intel_plane;
> +	struct intel_plane_state *state;
>   	unsigned long possible_crtcs;
>   	const uint32_t *plane_formats;
>   	int num_plane_formats;
> @@ -1475,6 +1481,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>   		kfree(intel_plane);
>   		return -ENOMEM;
>   	}
> +	state = to_intel_plane_state(intel_plane->base.state);
>
>   	switch (INTEL_INFO(dev)->gen) {
>   	case 5:
> @@ -1545,7 +1552,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>
>   	intel_plane->pipe = pipe;
>   	intel_plane->plane = plane;
> -	intel_plane->rotation = BIT(DRM_ROTATE_0);
> +	state->rotation = BIT(DRM_ROTATE_0);
>   	intel_plane->check_plane = intel_check_sprite_plane;
>   	intel_plane->commit_plane = intel_commit_sprite_plane;
>   	possible_crtcs = (1 << pipe);
> @@ -1567,7 +1574,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>   	if (dev->mode_config.rotation_property)
>   		drm_object_attach_property(&intel_plane->base.base,
>   					   dev->mode_config.rotation_property,
> -					   intel_plane->rotation);
> +					   state->rotation);
>
>   	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
>
>

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