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

-- 
Ville Syrjälä
Intel