Re: [PATCH v3 01/12] drm/atomic-helper: split not-scaling part of drm_atomic_helper_check_plane_state

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Sep 14, 2023 at 08:06:55AM +0300, Dmitry Baryshkov wrote:
> The helper drm_atomic_helper_check_plane_state() runs several checks on
> plane src and dst rectangles, including the check whether required
> scaling fits into the required margins. The msm driver would benefit
> from having a function that does all these checks except the scaling
> one. Split them into a new helper called
> drm_atomic_helper_check_plane_noscale().

What's the point in eliminating a nop scaling check?

> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 110 ++++++++++++++++++++++------
>  include/drm/drm_atomic_helper.h     |   7 ++
>  2 files changed, 96 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 292e38eb6218..2d7dd66181c9 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -825,11 +825,9 @@ drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
>  EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
>  
>  /**
> - * drm_atomic_helper_check_plane_state() - Check plane state for validity
> + * drm_atomic_helper_check_plane_noscale() - Check plane state for validity
>   * @plane_state: plane state to check
>   * @crtc_state: CRTC state to check
> - * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
> - * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
>   * @can_position: is it legal to position the plane such that it
>   *                doesn't cover the entire CRTC?  This will generally
>   *                only be false for primary planes.
> @@ -845,19 +843,16 @@ EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
>   * RETURNS:
>   * Zero if update appears valid, error code on failure
>   */
> -int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> -					const struct drm_crtc_state *crtc_state,
> -					int min_scale,
> -					int max_scale,
> -					bool can_position,
> -					bool can_update_disabled)
> +int drm_atomic_helper_check_plane_noscale(struct drm_plane_state *plane_state,
> +					  const struct drm_crtc_state *crtc_state,
> +					  bool can_position,
> +					  bool can_update_disabled)
>  {
>  	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;
>  	struct drm_rect clip = {};
> -	int hscale, vscale;
>  
>  	WARN_ON(plane_state->crtc && plane_state->crtc != crtc_state->crtc);
>  
> @@ -883,17 +878,6 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
>  
>  	drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
>  
> -	/* Check scaling */
> -	hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
> -	vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
> -	if (hscale < 0 || vscale < 0) {
> -		drm_dbg_kms(plane_state->plane->dev,
> -			    "Invalid scaling of plane\n");
> -		drm_rect_debug_print("src: ", &plane_state->src, true);
> -		drm_rect_debug_print("dst: ", &plane_state->dst, false);
> -		return -ERANGE;
> -	}
> -
>  	if (crtc_state->enable)
>  		drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2);
>  
> @@ -921,6 +905,90 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(drm_atomic_helper_check_plane_noscale);
> +
> +/**
> + * drm_atomic_helper_check_plane_scale() - Check whether plane can be scaled
> + * @plane_state: plane state to check
> + * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
> + * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
> + *
> + * Checks that a desired plane scale fits into the min_scale..max_scale
> + * boundaries.
> + * Drivers that provide their own plane handling rather than helper-provided
> + * implementations may still wish to call this function to avoid duplication of
> + * error checking code.
> + *
> + * RETURNS:
> + * Zero if update appears valid, error code on failure
> + */
> +int drm_atomic_helper_check_plane_scale(struct drm_plane_state *plane_state,
> +					int min_scale,
> +					int max_scale)
> +{
> +	struct drm_framebuffer *fb = plane_state->fb;
> +	struct drm_rect src;
> +	struct drm_rect dst;
> +	int hscale, vscale;
> +
> +	if (!plane_state->visible)
> +		return 0;
> +
> +	src = drm_plane_state_src(plane_state);
> +	dst = drm_plane_state_dest(plane_state);
> +
> +	drm_rect_rotate(&src, fb->width << 16, fb->height << 16, plane_state->rotation);
> +
> +	hscale = drm_rect_calc_hscale(&src, &dst, min_scale, max_scale);
> +	vscale = drm_rect_calc_vscale(&src, &dst, min_scale, max_scale);
> +	if (hscale < 0 || vscale < 0) {
> +		drm_dbg_kms(plane_state->plane->dev,
> +			    "Invalid scaling of plane\n");
> +		drm_rect_debug_print("src: ", &plane_state->src, true);
> +		drm_rect_debug_print("dst: ", &plane_state->dst, false);
> +		return -ERANGE;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_check_plane_scale);
> +
> +/**
> + * drm_atomic_helper_check_plane_state() - Check plane state for validity
> + * @plane_state: plane state to check
> + * @crtc_state: CRTC state to check
> + * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
> + * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
> + * @can_position: is it legal to position the plane such that it
> + *                doesn't cover the entire CRTC?  This will generally
> + *                only be false for primary planes.
> + * @can_update_disabled: can the plane be updated while the CRTC
> + *                       is disabled?
> + *
> + * Checks that a desired plane update is valid, and updates various
> + * bits of derived state (clipped coordinates etc.). Drivers that provide
> + * their own plane handling rather than helper-provided implementations may
> + * still wish to call this function to avoid duplication of error checking
> + * code.
> + *
> + * RETURNS:
> + * Zero if update appears valid, error code on failure
> + */
> +int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> +					const struct drm_crtc_state *crtc_state,
> +					int min_scale,
> +					int max_scale,
> +					bool can_position,
> +					bool can_update_disabled)
> +{
> +	int ret;
> +
> +	ret = drm_atomic_helper_check_plane_noscale(plane_state, crtc_state, can_position, can_update_disabled);
> +	if (ret < 0)
> +		return ret;
> +
> +	return drm_atomic_helper_check_plane_scale(plane_state, min_scale, max_scale);
> +}
>  EXPORT_SYMBOL(drm_atomic_helper_check_plane_state);
>  
>  /**
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 536a0b0091c3..32ac55aea94e 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -52,6 +52,13 @@ int drm_atomic_helper_check_modeset(struct drm_device *dev,
>  int
>  drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
>  					 struct drm_connector_state *conn_state);
> +int drm_atomic_helper_check_plane_noscale(struct drm_plane_state *plane_state,
> +					  const struct drm_crtc_state *crtc_state,
> +					  bool can_position,
> +					  bool can_update_disabled);
> +int drm_atomic_helper_check_plane_scale(struct drm_plane_state *plane_state,
> +					int min_scale,
> +					int max_scale);
>  int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
>  					const struct drm_crtc_state *crtc_state,
>  					int min_scale,
> -- 
> 2.39.2

-- 
Ville Syrjälä
Intel