Re: [PATCH v3 09/19] drm/i915: clean up atomic plane check functions, v2.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 18-06-15 om 03:48 schreef Matt Roper:
> On Mon, Jun 15, 2015 at 12:33:46PM +0200, Maarten Lankhorst wrote:
>> By passing crtc_state to the check_plane functions a lot of duplicated
>> code can be removed. There are still some transitional helper calls,
>> they will be removed later.
>>
>> Changes since v1:
>> - Revert state->visible changes.
>> - Use plane->state->crtc instead of plane->crtc.
>> - Use drm_atomic_get_existing_crtc_state.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic_plane.c | 16 +++++++----
>>  drivers/gpu/drm/i915/intel_display.c      | 48 ++++++++++---------------------
>>  drivers/gpu/drm/i915/intel_drv.h          |  1 +
>>  drivers/gpu/drm/i915/intel_sprite.c       |  9 ++----
>>  4 files changed, 29 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> index aa2128369a0a..91d53768df9d 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> @@ -116,7 +116,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>>  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
>>  	int ret;
>>  
>> -	crtc = crtc ? crtc : plane->crtc;
>> +	crtc = crtc ? crtc : plane->state->crtc;
>>  	intel_crtc = to_intel_crtc(crtc);
>>  
>>  	/*
>> @@ -131,10 +131,13 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>>  	/* FIXME: temporary hack necessary while we still use the plane update
>>  	 * helper. */
>>  	if (state->state) {
>> -		crtc_state =
>> -			intel_atomic_get_crtc_state(state->state, intel_crtc);
>> -		if (IS_ERR(crtc_state))
>> -			return PTR_ERR(crtc_state);
>> +		struct drm_crtc_state *drm_crtc_state =
>> +			drm_atomic_get_existing_crtc_state(state->state, crtc);
> Is this change important?  We can't get to this point without the crtc
> state being in the atomic transaction can we?  The only case where that
> used to happen was if crtc was actually NULL due to a property update of
> a disabled plane, but we bail out on !crtc just above this.
>
> Not that this change would cause any problems that I see, I just don't
> understand the motivation.
It's just paranoia. There were some cases in which we didn't add the crtc state correctly, which was a bug.
This mostly happened without the other hunk that checks plane->state->crtc, but since we really don't want
to deal with errors by allocating crtc state here, I felt adding a WARN_ON was more appropriate.
>> +
>> +		if (WARN_ON(!drm_crtc_state))
>> +			return -EINVAL;
>> +
>> +		crtc_state = to_intel_crtc_state(drm_crtc_state);
>>  	} else {
>>  		crtc_state = intel_crtc->config;
>>  	}
>> @@ -185,7 +188,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>>  		}
>>  	}
>>  
>> -	ret = intel_plane->check_plane(plane, intel_state);
>> +	intel_state->visible = false;
>> +	ret = intel_plane->check_plane(plane, crtc_state, intel_state);
>>  	if (ret || !state->state)
>>  		return ret;
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index ec4924eecd68..61697335bff2 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13703,36 +13703,25 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
>>  
>>  static int
>>  intel_check_primary_plane(struct drm_plane *plane,
>> +			  struct intel_crtc_state *crtc_state,
>>  			  struct intel_plane_state *state)
>>  {
>> -	struct drm_device *dev = plane->dev;
>>  	struct drm_crtc *crtc = state->base.crtc;
>> -	struct intel_crtc *intel_crtc;
>> -	struct intel_crtc_state *crtc_state;
>>  	struct drm_framebuffer *fb = state->base.fb;
>> -	struct drm_rect *dest = &state->dst;
>> -	struct drm_rect *src = &state->src;
>> -	const struct drm_rect *clip = &state->clip;
>> -	bool can_position = false;
>> -	int max_scale = DRM_PLANE_HELPER_NO_SCALING;
>>  	int min_scale = DRM_PLANE_HELPER_NO_SCALING;
>> +	int max_scale = DRM_PLANE_HELPER_NO_SCALING;
>> +	bool can_position = false;
>>  
>> -	crtc = crtc ? crtc : plane->crtc;
>> -	intel_crtc = to_intel_crtc(crtc);
>> -	crtc_state = state->base.state ?
>> -		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
>> -
>> -	if (INTEL_INFO(dev)->gen >= 9) {
>> -		/* use scaler when colorkey is not required */
>> -		if (to_intel_plane(plane)->ckey.flags == I915_SET_COLORKEY_NONE) {
>> -			min_scale = 1;
>> -			max_scale = skl_max_scale(intel_crtc, crtc_state);
>> -		}
>> +	/* use scaler when colorkey is not required */
>> +	if (INTEL_INFO(plane->dev)->gen >= 9 &&
>> +	    to_intel_plane(plane)->ckey.flags == I915_SET_COLORKEY_NONE) {
>> +		min_scale = 1;
>> +		max_scale = skl_max_scale(to_intel_crtc(crtc), crtc_state);
>>  		can_position = true;
>>  	}
>>  
>> -	return drm_plane_helper_check_update(plane, crtc, fb,
>> -					     src, dest, clip,
>> +	return drm_plane_helper_check_update(plane, crtc, fb, &state->src,
>> +					     &state->dst, &state->clip,
>>  					     min_scale, max_scale,
>>  					     can_position, true,
>>  					     &state->visible);
>> @@ -13973,24 +13962,17 @@ void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *
>>  
>>  static int
>>  intel_check_cursor_plane(struct drm_plane *plane,
>> +			 struct intel_crtc_state *crtc_state,
>>  			 struct intel_plane_state *state)
>>  {
>> -	struct drm_crtc *crtc = state->base.crtc;
>> -	struct drm_device *dev = plane->dev;
>> +	struct drm_crtc *crtc = crtc_state->base.crtc;
>>  	struct drm_framebuffer *fb = state->base.fb;
>> -	struct drm_rect *dest = &state->dst;
>> -	struct drm_rect *src = &state->src;
>> -	const struct drm_rect *clip = &state->clip;
>>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>> -	struct intel_crtc *intel_crtc;
>>  	unsigned stride;
>>  	int ret;
>>  
>> -	crtc = crtc ? crtc : plane->crtc;
>> -	intel_crtc = to_intel_crtc(crtc);
>> -
>> -	ret = drm_plane_helper_check_update(plane, crtc, fb,
>> -					    src, dest, clip,
>> +	ret = drm_plane_helper_check_update(plane, crtc, fb, &state->src,
>> +					    &state->dst, &state->clip,
>>  					    DRM_PLANE_HELPER_NO_SCALING,
>>  					    DRM_PLANE_HELPER_NO_SCALING,
>>  					    true, true, &state->visible);
>> @@ -14002,7 +13984,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>>  		return 0;
>>  
>>  	/* Check for which cursor types we support */
>> -	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
>> +	if (!cursor_size_ok(plane->dev, state->base.crtc_w, state->base.crtc_h)) {
>>  		DRM_DEBUG("Cursor dimension %dx%d not supported\n",
>>  			  state->base.crtc_w, state->base.crtc_h);
>>  		return -EINVAL;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 9f5867bf745e..8c0f17e84eee 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -613,6 +613,7 @@ struct intel_plane {
>>  	void (*disable_plane)(struct drm_plane *plane,
>>  			      struct drm_crtc *crtc, bool force);
>>  	int (*check_plane)(struct drm_plane *plane,
>> +			   struct intel_crtc_state *crtc_state,
>>  			   struct intel_plane_state *state);
>>  	void (*commit_plane)(struct drm_plane *plane,
>>  			     struct intel_plane_state *state);
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index c909b8b8ce85..999a5753dde3 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -739,11 +739,12 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc, bool force)
>>  
>>  static int
>>  intel_check_sprite_plane(struct drm_plane *plane,
>> +			 struct intel_crtc_state *crtc_state,
>>  			 struct intel_plane_state *state)
>>  {
>>  	struct drm_device *dev = plane->dev;
>> -	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
>> -	struct intel_crtc_state *crtc_state;
>> +	struct drm_crtc *crtc = state->base.crtc;
>> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> We don't actually use crtc anywhere after this, so I'm not sure if there
> was a need to change the assignment of intel_crtc?  Not a big deal
> either way.
>
Can't remember why I did this, but it's not worth sending a new patch for. :)

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