Re: [PATCH 4/5] drm: Check crtc_state->enable rather than crtc->enabled in drm_plane_helper_check_state()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux