Re: [Intel-gfx] [PATCH v7 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14
On Mon, 2023-01-09 at 09:06 +0200, Lisovskiy, Stanislav wrote:
> On Fri, Dec 23, 2022 at 03:05:08PM +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).
> >
> > In v7:
> >* Constify a couple of local variables;
> >* Return -EINVAL, instead of -EOPNOTSUPP and -EBUSY;
> >* Add another FIXME;
> >* Remove unnecessary undoing of change in error cases.
> >
> >
> > drivers/gpu/drm/i915/display/intel_atomic.c | 85 ++---
> > 1 file changed, 75 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..a9a3f3715279 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 -EINVAL;
>
> As I understand that change is a bit irrelevant to the patch topic,
> ideally it should be reflected in the commit message, that we are doing
> this and most importantly why.
Right, maybe this should have been mentioned in the commit message. I
initially didn't return an error for the new failure path, but Ville
asked me to do so, so I changed the function to return an error here as
well.
> However I'm not going to be picky here, as it is a small thing, just
> as a side note.
Thanks!
> Reviewed-by: Stanislav Lisovskiy
Thanks for the review, Stan!
--
Cheers,
Luca.
Re: [Intel-gfx] [PATCH v7 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14
On Fri, Dec 23, 2022 at 03:05:08PM +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).
>
> In v7:
>* Constify a couple of local variables;
>* Return -EINVAL, instead of -EOPNOTSUPP and -EBUSY;
>* Add another FIXME;
>* Remove unnecessary undoing of change in error cases.
>
>
> drivers/gpu/drm/i915/display/intel_atomic.c | 85 ++---
> 1 file changed, 75 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..a9a3f3715279 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 -EINVAL;
As I understand that change is a bit irrelevant to the patch topic,
ideally it should be reflected in the commit message, that we are doing
this and most importantly why.
However I'm not going to be picky here, as it is a small thing, just
as a side note.
Reviewed-by: Stanislav Lisovskiy
>
> /* set scaler mode */
> if (plane_state && plane_state->hw.fb &&
> @@ -375,9 +376,71 @@ 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;
> + const struct drm_rect *src = &plane_state->uapi.src;
> + const 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 = 0x3 - 1;
> + if (*scaler_id == 0)
>
[Intel-gfx] [PATCH v7 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14
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).
In v7:
* Constify a couple of local variables;
* Return -EINVAL, instead of -EOPNOTSUPP and -EBUSY;
* Add another FIXME;
* Remove unnecessary undoing of change in error cases.
drivers/gpu/drm/i915/display/intel_atomic.c | 85 ++---
1 file changed, 75 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..a9a3f3715279 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 -EINVAL;
/* set scaler mode */
if (plane_state && plane_state->hw.fb &&
@@ -375,9 +376,71 @@ 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;
+ const struct drm_rect *src = &plane_state->uapi.src;
+ const 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 = 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;
+ max_vscale =
