On Wed, 14 Feb 2024 at 20:47, Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>
> 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?
No, I'm just saying that the scaling ability depends on the rotation
and other plane properties. So I had to separate the basic plane
checks and the scaling check.
Basic (noscale) plane check source and destination rectangles, etc.
After that the driver identifies possible hardware pipe usage and
after that it can perform a scaling check.
>
> >
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > ---
> > > 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
--
With best wishes
Dmitry
