Hi Ville,
Thank you for the patch.
On Wednesday, 1 November 2017 20:29:19 EET 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.
>
> 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().
The only user of drm_plane_helper_check_update() apart from vmwgfx which you
converted in patch 2/5 is the armada driver. It would be nice to convert it to
atomic modesetting and drop this function :-) At the very least, I'd add a
comment to drm_plane_helper_check_update() that this change should be reverted
when the function is dropped, otherwise we'll carry the CRTC state argument
for a long time without realizing it's not needed anymore.
Apart from that,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/arm/hdlcd_crtc.c | 2 +-
> drivers/gpu/drm/arm/malidp_planes.c | 3 +-
> drivers/gpu/drm/drm_plane_helper.c | 52 ++++++++++++++------------
> 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, 53 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..adb8d94484f2
> 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,38 @@ 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(!crtc_state != !plane_state->crtc);
> + WARN_ON(crtc_state && crtc_state->crtc != plane_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 +164,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 +234,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 +249,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,
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
