On Tue, 2022-12-20 at 23:00 +0200, Ville Syrjälä wrote:
> 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.
Done.
> > + 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.
I added another 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.
Right. This is reminiscent from the previous version of the patch,
where we were not returning errors. I removed this now.
> > +
> > + 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.
Okay. My view is that by having different error codes we can at least
have a bit more clue and make it easier to track down. I'll change it
to -EINVAL, and the -EBUSY I added earlier to -EINVAL as well.
Thanks for the review.
--
Cheers,
Luca.
>
