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.barysh...@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
