Re: [Intel-gfx] [PATCH v6 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14
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
> > ---
> >
> > 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 = 0x3 - 1;
> > + if (*scaler_id == 0)
> > + max_vscale = 0x3 - 1;
> > + else
> > + max_vscale = 0x1;
> > +
> > + } else if (DISPLAY_VER(dev_priv) >= 10 ||
> > + !intel_format_info_is_yuv_semiplanar(fb->form
Re: [Intel-gfx] [PATCH v6 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14
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
> ---
>
> 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 = 0x3 - 1;
> + if (*scaler_id == 0)
> + max_vscale = 0x3 - 1;
> + else
> + max_vscale = 0x1;
> +
> + } else if (DISPLAY_VER(dev_priv) >= 10 ||
> +!intel_format_info_is_yuv_semiplanar(fb->format,
> fb->modifier)) {
> + max_hscale = 0x3 - 1;
> + max_vscale = 0x3 - 1;
> + } else {
> + max_hscale = 0x2 - 1;
> +
