Re: [Intel-gfx] [PATCH v7 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

["Coelho, Luciano" <luciano.coelho@intel.com>]

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@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).
> > 
> > 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@intel.com>

Thanks for the review, Stan!

--
Cheers,
Luca.