On Wed, Nov 01, 2017 at 10:15:58PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> drm_plane_helper_check_state() is supposed to do things the atomic way,
> so it should not be inspecting crtc->enabled. Rather we should be
> looking at crtc_state->enable.
Oh dear, how often are those legacy state thingies going to hurt us :-/
> We have a slight complication due to drm_plane_helper_check_update()
> reusing drm_plane_helper_check_state() for non-atomic drivers. Thus
> we'll have to pass the crtc_state in manally and construct a fake
> crtc_state in drm_plane_helper_check_update().
>
> v2: Fix the WARNs about plane_state->crtc matching crtc_state->crtc
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
I guess even more reasons to drop the clip rectangle and just look at
crtc_state?
Anyway, this looks good to me.
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> drivers/gpu/drm/arm/hdlcd_crtc.c | 2 +-
> drivers/gpu/drm/arm/malidp_planes.c | 3 +-
> drivers/gpu/drm/drm_plane_helper.c | 51 ++++++++++++++++-------------
> drivers/gpu/drm/drm_simple_kms_helper.c | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 2 ++
> drivers/gpu/drm/imx/ipuv3-plane.c | 2 +-
> drivers/gpu/drm/mediatek/mtk_drm_plane.c | 2 +-
> drivers/gpu/drm/meson/meson_plane.c | 2 +-
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 4 +--
> drivers/gpu/drm/nouveau/nv50_display.c | 6 ++--
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 +-
> drivers/gpu/drm/tegra/dc.c | 4 +--
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 +-
> drivers/gpu/drm/zte/zx_plane.c | 4 +--
> include/drm/drm_plane_helper.h | 3 +-
> 15 files changed, 52 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index 72b22b805412..14721723fa8a 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -252,7 +252,7 @@ static int hdlcd_plane_atomic_check(struct drm_plane *plane,
> clip.x2 = crtc_state->adjusted_mode.hdisplay;
> clip.y2 = crtc_state->adjusted_mode.vdisplay;
>
> - return drm_plane_helper_check_state(state, &clip,
> + return drm_plane_helper_check_state(state, crtc_state, &clip,
> DRM_PLANE_HELPER_NO_SCALING,
> DRM_PLANE_HELPER_NO_SCALING,
> false, true);
> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
> index 94e7e3fa3408..492d99dd55d4 100644
> --- a/drivers/gpu/drm/arm/malidp_planes.c
> +++ b/drivers/gpu/drm/arm/malidp_planes.c
> @@ -150,7 +150,8 @@ static int malidp_se_check_scaling(struct malidp_plane *mp,
>
> clip.x2 = crtc_state->adjusted_mode.hdisplay;
> clip.y2 = crtc_state->adjusted_mode.vdisplay;
> - ret = drm_plane_helper_check_state(state, &clip, 0, INT_MAX, true, true);
> + ret = drm_plane_helper_check_state(state, crtc_state, &clip,
> + 0, INT_MAX, true, true);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index 759ed93f4ba8..eef0ffcd7ed5 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -101,7 +101,8 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
>
> /**
> * drm_plane_helper_check_state() - Check plane state for validity
> - * @state: plane state to check
> + * @plane_state: plane state to check
> + * @crtc_state: crtc state to check
> * @clip: integer clipping coordinates
> * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
> * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
> @@ -120,35 +121,37 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
> * RETURNS:
> * Zero if update appears valid, error code on failure
> */
> -int drm_plane_helper_check_state(struct drm_plane_state *state,
> +int drm_plane_helper_check_state(struct drm_plane_state *plane_state,
> + const struct drm_crtc_state *crtc_state,
> const struct drm_rect *clip,
> int min_scale,
> int max_scale,
> bool can_position,
> bool can_update_disabled)
> {
> - struct drm_crtc *crtc = state->crtc;
> - struct drm_framebuffer *fb = state->fb;
> - struct drm_rect *src = &state->src;
> - struct drm_rect *dst = &state->dst;
> - unsigned int rotation = state->rotation;
> + struct drm_framebuffer *fb = plane_state->fb;
> + struct drm_rect *src = &plane_state->src;
> + struct drm_rect *dst = &plane_state->dst;
> + unsigned int rotation = plane_state->rotation;
> int hscale, vscale;
>
> - *src = drm_plane_state_src(state);
> - *dst = drm_plane_state_dest(state);
> + WARN_ON(plane_state->crtc && plane_state->crtc != crtc_state->crtc);
> +
> + *src = drm_plane_state_src(plane_state);
> + *dst = drm_plane_state_dest(plane_state);
>
> if (!fb) {
> - state->visible = false;
> + plane_state->visible = false;
> return 0;
> }
>
> /* crtc should only be NULL when disabling (i.e., !fb) */
> - if (WARN_ON(!crtc)) {
> - state->visible = false;
> + if (WARN_ON(!plane_state->crtc)) {
> + plane_state->visible = false;
> return 0;
> }
>
> - if (!crtc->enabled && !can_update_disabled) {
> + if (!crtc_state->enable && !can_update_disabled) {
> DRM_DEBUG_KMS("Cannot update plane of a disabled CRTC.\n");
> return -EINVAL;
> }
> @@ -160,16 +163,16 @@ int drm_plane_helper_check_state(struct drm_plane_state *state,
> vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
> if (hscale < 0 || vscale < 0) {
> DRM_DEBUG_KMS("Invalid scaling of plane\n");
> - drm_rect_debug_print("src: ", &state->src, true);
> - drm_rect_debug_print("dst: ", &state->dst, false);
> + drm_rect_debug_print("src: ", &plane_state->src, true);
> + drm_rect_debug_print("dst: ", &plane_state->dst, false);
> return -ERANGE;
> }
>
> - state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
> + plane_state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
>
> drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
>
> - if (!state->visible)
> + if (!plane_state->visible)
> /*
> * Plane isn't visible; some drivers can handle this
> * so we just return success here. Drivers that can't
> @@ -230,7 +233,7 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> bool can_update_disabled,
> bool *visible)
> {
> - struct drm_plane_state state = {
> + struct drm_plane_state plane_state = {
> .plane = plane,
> .crtc = crtc,
> .fb = fb,
> @@ -245,18 +248,22 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> .rotation = rotation,
> .visible = *visible,
> };
> + struct drm_crtc_state crtc_state = {
> + .crtc = crtc,
> + .enable = crtc->enabled,
> + };
> int ret;
>
> - ret = drm_plane_helper_check_state(&state, clip,
> + ret = drm_plane_helper_check_state(&plane_state, &crtc_state, clip,
> min_scale, max_scale,
> can_position,
> can_update_disabled);
> if (ret)
> return ret;
>
> - *src = state.src;
> - *dst = state.dst;
> - *visible = state.visible;
> + *src = plane_state.src;
> + *dst = plane_state.dst;
> + *visible = plane_state.visible;
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index dc9fd109de14..d428c805025c 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -103,7 +103,7 @@ static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
> clip.x2 = crtc_state->adjusted_mode.hdisplay;
> clip.y2 = crtc_state->adjusted_mode.vdisplay;
>
> - ret = drm_plane_helper_check_state(plane_state, &clip,
> + ret = drm_plane_helper_check_state(plane_state, crtc_state, &clip,
> DRM_PLANE_HELPER_NO_SCALING,
> DRM_PLANE_HELPER_NO_SCALING,
> false, true);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 737de251d0f8..4a459726b36b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9317,6 +9317,7 @@ static int intel_check_cursor(struct intel_crtc_state *crtc_state,
> int ret;
>
> ret = drm_plane_helper_check_state(&plane_state->base,
> + &crtc_state->base,
> &plane_state->clip,
> DRM_PLANE_HELPER_NO_SCALING,
> DRM_PLANE_HELPER_NO_SCALING,
> @@ -12808,6 +12809,7 @@ intel_check_primary_plane(struct intel_plane *plane,
> }
>
> ret = drm_plane_helper_check_state(&state->base,
> + &crtc_state->base,
> &state->clip,
> min_scale, max_scale,
> can_position, true);
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index 247c60e6bed2..51b23ac175e3 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -342,7 +342,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> clip.y1 = 0;
> clip.x2 = crtc_state->adjusted_mode.hdisplay;
> clip.y2 = crtc_state->adjusted_mode.vdisplay;
> - ret = drm_plane_helper_check_state(state, &clip,
> + ret = drm_plane_helper_check_state(state, crtc_state, &clip,
> DRM_PLANE_HELPER_NO_SCALING,
> DRM_PLANE_HELPER_NO_SCALING,
> can_position, true);
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index 6f121891430f..7ebb33657704 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -111,7 +111,7 @@ static int mtk_plane_atomic_check(struct drm_plane *plane,
> clip.x2 = crtc_state->mode.hdisplay;
> clip.y2 = crtc_state->mode.vdisplay;
>
> - return drm_plane_helper_check_state(state, &clip,
> + return drm_plane_helper_check_state(state, crtc_state, &clip,
> DRM_PLANE_HELPER_NO_SCALING,
> DRM_PLANE_HELPER_NO_SCALING,
> true, true);
> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
> index 17e96fa47868..2a00b720c371 100644
> --- a/drivers/gpu/drm/meson/meson_plane.c
> +++ b/drivers/gpu/drm/meson/meson_plane.c
> @@ -61,7 +61,7 @@ static int meson_plane_atomic_check(struct drm_plane *plane,
> clip.x2 = crtc_state->mode.hdisplay;
> clip.y2 = crtc_state->mode.vdisplay;
>
> - return drm_plane_helper_check_state(state, &clip,
> + return drm_plane_helper_check_state(state, crtc_state, &clip,
> DRM_PLANE_HELPER_NO_SCALING,
> DRM_PLANE_HELPER_NO_SCALING,
> true, true);
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> index 4b22ac3413a1..1c194a9453d9 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> @@ -348,8 +348,8 @@ static int mdp5_plane_atomic_check_with_state(struct drm_crtc_state *crtc_state,
> min_scale = FRAC_16_16(1, 8);
> max_scale = FRAC_16_16(8, 1);
>
> - ret = drm_plane_helper_check_state(state, &clip, min_scale,
> - max_scale, true, true);
> + ret = drm_plane_helper_check_state(state, crtc_state, &clip,
> + min_scale, max_scale, true, true);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
> index e4751f92b342..7f11a88d0593 100644
> --- a/drivers/gpu/drm/nouveau/nv50_display.c
> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
> @@ -1143,7 +1143,8 @@ nv50_curs_acquire(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw,
> {
> int ret;
>
> - ret = drm_plane_helper_check_state(&asyw->state, &asyw->clip,
> + ret = drm_plane_helper_check_state(&asyw->state, &asyh->state,
> + &asyw->clip,
> DRM_PLANE_HELPER_NO_SCALING,
> DRM_PLANE_HELPER_NO_SCALING,
> true, true);
> @@ -1432,7 +1433,8 @@ nv50_base_acquire(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw,
> if (!fb->format->depth)
> return -EINVAL;
>
> - ret = drm_plane_helper_check_state(&asyw->state, &asyw->clip,
> + ret = drm_plane_helper_check_state(&asyw->state, &asyh->state,
> + &asyw->clip,
> DRM_PLANE_HELPER_NO_SCALING,
> DRM_PLANE_HELPER_NO_SCALING,
> false, true);
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 19128b4dea54..36d0f101e30d 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -659,7 +659,7 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
> clip.x2 = crtc_state->adjusted_mode.hdisplay;
> clip.y2 = crtc_state->adjusted_mode.vdisplay;
>
> - ret = drm_plane_helper_check_state(state, &clip,
> + ret = drm_plane_helper_check_state(state, crtc_state, &clip,
> min_scale, max_scale,
> true, true);
> if (ret)
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 24a5ef4f5bb8..e95d9e2a7f6c 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -491,8 +491,8 @@ static int tegra_plane_state_add(struct tegra_plane *plane,
> clip.y2 = crtc_state->mode.vdisplay;
>
> /* Check plane state for visibility and calculate clipping bounds */
> - err = drm_plane_helper_check_state(state, &clip, 0, INT_MAX,
> - true, true);
> + err = drm_plane_helper_check_state(state, crtc_state, &clip,
> + 0, INT_MAX, true, true);
> if (err < 0)
> return err;
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index efa41c086198..d95e7b1c3b11 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -454,7 +454,7 @@ int vmw_du_primary_plane_atomic_check(struct drm_plane *plane,
> clip.y2 = crtc_state->adjusted_mode.vdisplay;
> }
>
> - ret = drm_plane_helper_check_state(state, &clip,
> + ret = drm_plane_helper_check_state(state, crtc_state, &clip,
> DRM_PLANE_HELPER_NO_SCALING,
> DRM_PLANE_HELPER_NO_SCALING,
> false, true);
> diff --git a/drivers/gpu/drm/zte/zx_plane.c b/drivers/gpu/drm/zte/zx_plane.c
> index 18e763493264..ee0002529b8f 100644
> --- a/drivers/gpu/drm/zte/zx_plane.c
> +++ b/drivers/gpu/drm/zte/zx_plane.c
> @@ -80,7 +80,7 @@ static int zx_vl_plane_atomic_check(struct drm_plane *plane,
> clip.x2 = crtc_state->adjusted_mode.hdisplay;
> clip.y2 = crtc_state->adjusted_mode.vdisplay;
>
> - return drm_plane_helper_check_state(plane_state, &clip,
> + return drm_plane_helper_check_state(plane_state, crtc_state, &clip,
> min_scale, max_scale,
> true, true);
> }
> @@ -315,7 +315,7 @@ static int zx_gl_plane_atomic_check(struct drm_plane *plane,
> clip.x2 = crtc_state->adjusted_mode.hdisplay;
> clip.y2 = crtc_state->adjusted_mode.vdisplay;
>
> - return drm_plane_helper_check_state(plane_state, &clip,
> + return drm_plane_helper_check_state(plane_state, crtc_state, &clip,
> DRM_PLANE_HELPER_NO_SCALING,
> DRM_PLANE_HELPER_NO_SCALING,
> false, true);
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index 7c8a00ceadb7..41b8309b0a57 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -38,7 +38,8 @@
> */
> #define DRM_PLANE_HELPER_NO_SCALING (1<<16)
>
> -int drm_plane_helper_check_state(struct drm_plane_state *state,
> +int drm_plane_helper_check_state(struct drm_plane_state *plane_state,
> + const struct drm_crtc_state *crtc_state,
> const struct drm_rect *clip,
> int min_scale, int max_scale,
> bool can_position,
> --
> 2.13.6
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
