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 <[email protected]> >> --- >> 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 [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
