[Intel-gfx] [PATCH v3 09/19] drm/i915: clean up atomic plane check functions, v2.
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Mon Jun 22 00:23:04 PDT 2015
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 at 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
More information about the Intel-gfx
mailing list