On Tue, Dec 20, 2022 at 02:07:23PM +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.coe...@intel.com>
> ---
>
> In v2:
> * fix DRM_PLANE_NO_SCALING renamed macros;
>
> In v3:
> * No changes.
>
> In v4:
> * Got rid of the changes in the general planes max scale code;
> * Added a couple of FIXMEs;
> * Made intel_atomic_setup_scaler() return an int with errors;
>
> In v5:
> * Just resent with a cover letter.
>
> In v6:
> * Now the correct version again (same as v4).
>
>
> drivers/gpu/drm/i915/display/intel_atomic.c | 83 ++++++++++++++++++---
> 1 file changed, 73 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> b/drivers/gpu/drm/i915/display/intel_atomic.c
> index 6621aa245caf..8373be283d8b 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -41,6 +41,7 @@
> #include "intel_global_state.h"
> #include "intel_hdcp.h"
> #include "intel_psr.h"
> +#include "intel_fb.h"
> #include "skl_universal_plane.h"
>
> /**
> @@ -310,11 +311,11 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
> kfree(crtc_state);
> }
>
> -static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state
> *scaler_state,
> - int num_scalers_need, struct intel_crtc
> *intel_crtc,
> - const char *name, int idx,
> - struct intel_plane_state *plane_state,
> - int *scaler_id)
> +static int intel_atomic_setup_scaler(struct intel_crtc_scaler_state
> *scaler_state,
> + int num_scalers_need, struct intel_crtc
> *intel_crtc,
> + const char *name, int idx,
> + struct intel_plane_state *plane_state,
> + int *scaler_id)
> {
> struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> int j;
> @@ -334,7 +335,7 @@ static void intel_atomic_setup_scaler(struct
> intel_crtc_scaler_state *scaler_sta
>
> if (drm_WARN(&dev_priv->drm, *scaler_id < 0,
> "Cannot find scaler for %s:%d\n", name, idx))
> - return;
> + return -EBUSY;
>
> /* set scaler mode */
> if (plane_state && plane_state->hw.fb &&
> @@ -375,9 +376,69 @@ static void intel_atomic_setup_scaler(struct
> intel_crtc_scaler_state *scaler_sta
> mode = SKL_PS_SCALER_MODE_DYN;
> }
>
> + /*
> + * FIXME: we should also check the scaler factors for pfit, so
> + * this shouldn't be tied directly to planes.
> + */
> + 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;
Those can be const.
> + int hscale, vscale, max_vscale, max_hscale;
> +
> + /*
> + * FIXME: When two scalers are needed, but only one of
> + * them needs to downscale, we should make sure that
> + * the one that needs downscaling support is assigned
> + * as the first scaler, so we don't reject downscaling
> + * unnecessarily.
> + */
> +
> + 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;
> +
> + } 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;
> + }
We'd want something along these lines if we want to handle
the hq vs. dyn scaler stuff correctly.
if (DISPLAY_VER(dev_priv) >= 14) {
...
} else if (DISPLAY_VER(dev_priv) >= 10)
max_hscale = 0x30000 - 1;
max_vscale = 0x30000 - 1;
} else if (mode == NV12) {
max_hscale = 0x20000 - 1;
max_vscale = 0x20000 - 1;
} else if (mode == HQ || src_w <= 2048) {
max_hscale = 0x30000 - 1;
max_vscale = 0x30000 - 1;
} else {
max_hscale = 0x30000 - 1;
max_vscale = 0x20000 - 1;
}
Though we could leave that for a followup patch, in which
case perhaps add a FIXME.
> +
> + /* 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;
There should be no need to undo stuff like this.
> +
> + return -EOPNOTSUPP;
We typically just go with -EINVAL for pretty much everything.
Given the number of things that can go wrong no one can realistically
figure out what happened/how to resolve it based on the errno alone
anyway.
> + }
> + }
> +
> drm_dbg_kms(&dev_priv->drm, "Attached scaler id %u.%u to %s:%d\n",
> intel_crtc->pipe, *scaler_id, name, idx);
> scaler_state->scalers[*scaler_id].mode = mode;
> +
> + return 0;
> }
>
> /**
> @@ -437,7 +498,7 @@ int intel_atomic_setup_scalers(struct drm_i915_private
> *dev_priv,
> for (i = 0; i < sizeof(scaler_state->scaler_users) * 8; i++) {
> int *scaler_id;
> const char *name;
> - int idx;
> + int idx, ret;
>
> /* skip if scaler not required */
> if (!(scaler_state->scaler_users & (1 << i)))
> @@ -494,9 +555,11 @@ int intel_atomic_setup_scalers(struct drm_i915_private
> *dev_priv,
> scaler_id = &plane_state->scaler_id;
> }
>
> - intel_atomic_setup_scaler(scaler_state, num_scalers_need,
> - intel_crtc, name, idx,
> - plane_state, scaler_id);
> + ret = intel_atomic_setup_scaler(scaler_state, num_scalers_need,
> + intel_crtc, name, idx,
> + plane_state, scaler_id);
> + if (ret < 0)
> + return ret;
> }
>
> return 0;
> --
> 2.38.1
--
Ville Syrjälä
Intel
