On Wed, Feb 14, 2024 at 08:37:02PM +0200, Ville Syrjälä wrote:
> 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?
Actually, what are you even doing in there? Are you saying that
the hardware has absolutely no limits on how much it can scale
in either direction?
>
> >
> > 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
--
Ville Syrjälä
Intel
