On Tue, 2022-12-20 at 13:09 +0200, Ville Syrjälä wrote:
> On Tue, Dec 20, 2022 at 10:11:16AM +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.
> >
> > drivers/gpu/drm/i915/display/i9xx_plane.c | 4 +-
> > drivers/gpu/drm/i915/display/intel_atomic.c | 83 ++++++++++++++++---
> > .../gpu/drm/i915/display/intel_atomic_plane.c | 30 ++++++-
> > .../gpu/drm/i915/display/intel_atomic_plane.h | 2 +-
> > drivers/gpu/drm/i915/display/intel_cursor.c | 4 +-
> > drivers/gpu/drm/i915/display/intel_sprite.c | 20 +----
> > .../drm/i915/display/skl_universal_plane.c | 45 +++++-----
> > 7 files changed, 128 insertions(+), 60 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c
> > b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > index ecaeb7dc196b..390e96f0692b 100644
> > --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> > +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > @@ -326,9 +326,7 @@ i9xx_plane_check(struct intel_crtc_state *crtc_state,
> > if (ret)
> > return ret;
> >
> > - ret = intel_atomic_plane_check_clipping(plane_state, crtc_state,
> > - DRM_PLANE_NO_SCALING,
> > - DRM_PLANE_NO_SCALING,
> > + ret = intel_atomic_plane_check_clipping(plane_state, crtc_state, false,
> >
> > i9xx_plane_has_windowing(plane));
> > if (ret)
> > return ret;
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index 6621aa245caf..bf4761a40675 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"
> > @@ -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;
> > + 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;
> > + }
> > +
> > + /* 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 -EOPNOTSUPP;
> > + }
> > + }
> > +
> > 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)
> > + return ret;
> > }
> >
> > return 0;
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index 10e1fc9d0698..50a05ccd2dda 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,41 @@ 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_hscale = DRM_PLANE_NO_SCALING;
> > + max_hscale = DRM_PLANE_NO_SCALING;
> > + min_vscale = DRM_PLANE_NO_SCALING;
> > + max_vscale = DRM_PLANE_NO_SCALING;
> > + } else {
> > + skl_plane_max_scale(i915, fb,
> > + &max_hscale, &min_hscale,
> > + &max_vscale, &min_vscale);
> > + }
>
> This stuff is still broken for pre-skl. Please just drop all
> the changes to intel_atomic_plane_check_clipping().
Damn, it seems that I sent the wrong version. This was removed in v4,
but I took the wrong version here. I'll resend.
--
Cheers,
Luca.
