Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

["Coelho, Luciano" <luciano.coelho@intel.com>]

On Wed, 2022-11-23 at 08:47 +0200, Ville Syrjälä wrote:
> On Tue, Nov 22, 2022 at 12:23:43PM +0200, Luca Coelho wrote:
> > In newer hardware versions (i.e. display version >= 14), the second
> > scaler doesn't support vertical scaling.
> > 
> > The current implementation of the scaling limits is simplified and
> > only occurs when the planes are created, so we don't know which scaler
> > is being used.
> > 
> > In order to handle separate scaling limits for horizontal and vertical
> > scaling, and different limits per scaler, split the checks in two
> > phases.  We first do a simple check during plane creation and use the
> > best-case scenario (because we don't know the scaler that may be used
> > at a later point) and then do a more specific check when the scalers
> > are actually being set up.
> > 
> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > ---


[...]
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index 6621aa245caf..43b1c7a227f8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -38,6 +38,7 @@
> >  #include "intel_atomic.h"
> >  #include "intel_cdclk.h"
> >  #include "intel_display_types.h"
> > +#include "intel_fb.h"
> >  #include "intel_global_state.h"
> >  #include "intel_hdcp.h"
> >  #include "intel_psr.h"
> > @@ -375,6 +376,52 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
> >  		mode = SKL_PS_SCALER_MODE_DYN;
> >  	}
> >  
> > +	if (plane_state && plane_state->hw.fb) {
> > +		const struct drm_framebuffer *fb = plane_state->hw.fb;
> > +		struct drm_rect *src = &plane_state->uapi.src;
> > +		struct drm_rect *dst = &plane_state->uapi.dst;
> 
> We want the scale factor checks for the pfit use case too. So this
> stuff shouldn't be so tied into planes. I guess we could go with
> a FIXME initially.

Okay, I'll add a FIXME.  It was tied to the plane checks before,
though, wasn't it? So nothing should have changed in that regard here.


> > +		int hscale, vscale, max_vscale, max_hscale;
> > +
> > +		if (DISPLAY_VER(dev_priv) >= 14) {
> > +			/*
> > +			 * On versions 14 and up, only the first
> > +			 * scaler supports a vertical scaling factor
> > +			 * of more than 1.0, while a horizontal
> > +			 * scaling factor of 3.0 is supported.
> > +			 */
> > +			max_hscale = 0x30000 - 1;
> > +			if (*scaler_id == 0)
> > +				max_vscale = 0x30000 - 1;
> > +			else
> > +				max_vscale = 0x10000;
> 
> We still have the chicken vs. egg problem here that we'd need to
> consider the scale factors already when selecting the scaler.
> But that could be another FIXME.

Do you mean in regards to the HQ vs. non-HQ needs?


> > +
> > +		} else if (DISPLAY_VER(dev_priv) >= 10 ||
> > +			   !intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier)) {
> > +			max_hscale = 0x30000 - 1;
> > +			max_vscale = 0x30000 - 1;
> > +		} else {
> > +			max_hscale = 0x20000 - 1;
> > +			max_vscale = 0x20000 - 1;
> > +		}
> 
> Pre-glk hq scaler case not handled.

I don't recall seen this specifically checked before.  Is this the
stuff I missed from g4x_sprite_check() below? Or am I missing something
else?


> > +
> > +		/* Check if required scaling is within limits */
> > +		hscale = drm_rect_calc_hscale(src, dst, 1, max_hscale);
> > +		vscale = drm_rect_calc_vscale(src, dst, 1, max_vscale);
> > +
> > +		if (hscale < 0 || vscale < 0) {
> > +			drm_dbg_kms(&dev_priv->drm,
> > +				    "Scaler %d doesn't support required plane scaling\n",
> > +				    *scaler_id);
> > +			drm_rect_debug_print("src: ", src, true);
> > +			drm_rect_debug_print("dst: ", dst, false);
> > +
> > +			scaler_state->scalers[*scaler_id].in_use = 0;
> > +			*scaler_id = -1;
> > +
> > +			return;
> 
> This would have to return an error rather than pretending that
> everything is fine.

We were already pretending everything is fine if a scaler if we
couldn't find a free scaler, for instance, so I just kept the same
logic, clearing up the scaler_id and marking the scaler as not in use
as well.

I can convert this to return an error, of course.  But then in the "not
free scaler" case we would still just ignore it or should we change the
behavior and make it fail as well?


[...]
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index 10e1fc9d0698..9100f328df60 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > @@ -887,7 +887,7 @@ void intel_crtc_planes_update_arm(struct intel_atomic_state *state,
> >  
> >  int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
> >  				      struct intel_crtc_state *crtc_state,
> > -				      int min_scale, int max_scale,
> > +				      bool allow_scaling,
> >  				      bool can_position)
> >  {
> >  	struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> > @@ -897,19 +897,50 @@ int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
> >  	const struct drm_rect *clip = &crtc_state->pipe_src;
> >  	unsigned int rotation = plane_state->hw.rotation;
> >  	int hscale, vscale;
> > +	int max_hscale, min_hscale, max_vscale, min_vscale;
> >  
> >  	if (!fb) {
> >  		plane_state->uapi.visible = false;
> >  		return 0;
> >  	}
> >  
> > +	/*
> > +	 * At this point we don't really know the HW limitations, so
> > +	 * we just sanitize the values against the maximum supported
> > +	 * scaling.
> > +	 */
> > +	if (allow_scaling) {
> > +		min_vscale = 1;
> > +		min_hscale = 1;
> > +
> > +		if (DISPLAY_VER(i915) < 10 ||
> > +		    intel_format_info_is_yuv_semiplanar(fb->format,
> > +							fb->modifier)) {
> > +			max_vscale = 0x20000 - 1;
> > +			max_hscale = 0x20000 - 1;
> > +		} else {
> > +			max_vscale = 0x30000 - 1;
> > +			max_hscale = 0x30000 - 1;
> > +		}
> > +	} else {
> > +		min_hscale = DRM_PLANE_NO_SCALING;
> > +		max_hscale = DRM_PLANE_NO_SCALING;
> > +		min_vscale = DRM_PLANE_NO_SCALING;
> > +		max_vscale = DRM_PLANE_NO_SCALING;
> > +	}
> 
> I still don't see the point in moving this hw specific knowledge
> from the more hw specific files into the hw agnostic file.

Is this file really that HW agnostic? I see lots of version checks with
"if (DISPLAY_VER(x))" all over the place.

As we discussed before, I think this kind of rules should be in HW-
specific configurations, but we don't have that yet.  And I thought it
would be better to keep these decisions in a single place rather than
just calling functions in other files...

If you prefer, I can move this back to skl_universal_plane.c or some
other of the skl_*.c files, but TBH they don't seem to be the right
place for this to me either...


[...]
> > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> > index e6b4d24b9cd0..9ad1173a0551 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > @@ -1355,22 +1355,11 @@ g4x_sprite_check(struct intel_crtc_state *crtc_state,
> >  {
> >  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > -	int min_scale = DRM_PLANE_NO_SCALING;
> > -	int max_scale = DRM_PLANE_NO_SCALING;
> >  	int ret;
> >  
> > -	if (g4x_fb_scalable(plane_state->hw.fb)) {
> > -		if (DISPLAY_VER(dev_priv) < 7) {
> > -			min_scale = 1;
> > -			max_scale = 16 << 16;
> > -		} else if (IS_IVYBRIDGE(dev_priv)) {
> > -			min_scale = 1;
> > -			max_scale = 2 << 16;
> > -		}
> > -	}
> 
> So what happened to these limits?

Oh, it seems that I lost them.  I guess they should be moved to the
intel_atomic_plane_check_clipping() function.  Again, to keep it all in
a single place.  But this seems to be only required in the sprite code,
so I'm not sure what I can do.

It's a problem to have this kinds of checks everywhere. 😞


[...]
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 76490cc59d8f..e2ae6624378f 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1463,22 +1463,6 @@ static int skl_plane_check_nv12_rotation(const struct intel_plane_state *plane_s
> >  	return 0;
> >  }
> >  
> > -static int skl_plane_max_scale(struct drm_i915_private *dev_priv,
> > -			       const struct drm_framebuffer *fb)
> > -{
> > -	/*
> > -	 * We don't yet know the final source width nor
> > -	 * whether we can use the HQ scaler mode. Assume
> > -	 * the best case.
> > -	 * FIXME need to properly check this later.
> > -	 */
> 
> Doesn't look like that FIXME has been dealt with as far
> as the hq scaler is concerned.

We now check the limits _after_ having decided whether HQ mode is used.
So that should be covered, right?


> 
Thanks for your review!

--
Cheers,
Luca.