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 <luciano.coelho@xxxxxxxxx>
> > ---
> >
> > 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 <stanislav.lisovskiy@xxxxxxxxx>
Thanks for the review, Stan!
--
Cheers,
Luca.
