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@xxxxxxxxxxxxxxx>
> ---
> 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.
> +
> + 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.
Matt
> struct intel_plane *intel_plane = to_intel_plane(plane);
> struct drm_framebuffer *fb = state->base.fb;
> int crtc_x, crtc_y;
> @@ -757,10 +758,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
> bool can_scale;
> int pixel_size;
>
> - intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> - crtc_state = state->base.state ?
> - intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
> -
> if (!fb) {
> state->visible = false;
> return 0;
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
