On Wed, 2022-11-23 at 11:57 +0200, Ville Syrjälä wrote:
> On Wed, Nov 23, 2022 at 09:16:42AM +0000, Coelho, Luciano wrote:
> > On Wed, 2022-11-23 at 08:47 +0200, Ville Syrjälä wrote:
> > > On Tue, Nov 22, 2022 at 12:23:43PM +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>
> > > > ---
> >
> >
> > [...]
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > index 6621aa245caf..43b1c7a227f8 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > @@ -38,6 +38,7 @@
> > > > #include "intel_atomic.h"
> > > > #include "intel_cdclk.h"
> > > > #include "intel_display_types.h"
> > > > +#include "intel_fb.h"
> > > > #include "intel_global_state.h"
> > > > #include "intel_hdcp.h"
> > > > #include "intel_psr.h"
> > > > @@ -375,6 +376,52 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
> > > > mode = SKL_PS_SCALER_MODE_DYN;
> > > > }
> > > >
> > > > + 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;
> > >
> > > We want the scale factor checks for the pfit use case too. So this
> > > stuff shouldn't be so tied into planes. I guess we could go with
> > > a FIXME initially.
> >
> > Okay, I'll add a FIXME. It was tied to the plane checks before,
> > though, wasn't it? So nothing should have changed in that regard here.
> >
> >
> > > > + int hscale, vscale, max_vscale, max_hscale;
> > > > +
> > > > + 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;
> > >
> > > We still have the chicken vs. egg problem here that we'd need to
> > > consider the scale factors already when selecting the scaler.
> > > But that could be another FIXME.
> >
> > Do you mean in regards to the HQ vs. non-HQ needs?
>
> I mean the "no downscaling on the second scaler" thing. The
> problem is that this thing walks the scaler users in order
> and assigns each one a scaler in turn. So if the first user
> doesn't need downscaling but the second one does then we
> will fail. OTOH had we just assigned the scalers in the
> reverse order we would have succeeded.
Ah, now I get it.
So, I guess we can do a similar thing to what we already do earlier in
this function to select the first scaler if HQ is needed. If
downscaling is needed in one of the scalers but not in the other, we
can swap them to make sure the one that needs downscaling in in the
first one.
But I agree with you in that this could be an added FIXME in this patch
and the actual change could be made in a separate patch.
> > > > +
> > > > + } 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;
> > > > + }
> > >
> > > Pre-glk hq scaler case not handled.
> >
> > I don't recall seen this specifically checked before. Is this the
> > stuff I missed from g4x_sprite_check() below? Or am I missing something
> > else?
>
> It was broken before in the sense that it allowed up to 3.0 scale factor
> whether or not the HQ scaler was used or not. Now it will reject anything
> above 2.0 even if the HQ scaler is used. So I guess technically it's a bit
> less broken now, but more limited. Anyways, should be possible to just
> check the scaler mode and pick the correct scaling limits based on it.
I see what you mean. But the code from before had the exact same
thing, I think? We were also checking for VER >= 10 and only then using
3.0. For anything else the limit was 2.0. Is your comment related to
the FIXME I removed from below? (your last comment in this email)
> > > > +
> > > > + /* 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;
> > > > +
> > > > + return;
> > >
> > > This would have to return an error rather than pretending that
> > > everything is fine.
> >
> > We were already pretending everything is fine if a scaler if we
> > couldn't find a free scaler, for instance, so I just kept the same
> > logic, clearing up the scaler_id and marking the scaler as not in use
> > as well.
> >
> > I can convert this to return an error, of course. But then in the "not
> > free scaler" case we would still just ignore it or should we change the
> > behavior and make it fail as well?
>
> The code is a mess, but it does look like intel_atomic_setup_scalers()
> should fail correctly if we can't find enough scalers.
Okay, so I'll change this function to return errors in both cases.
> > [...]
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > index 10e1fc9d0698..9100f328df60 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > @@ -887,7 +887,7 @@ void intel_crtc_planes_update_arm(struct intel_atomic_state *state,
> > > >
> > > > int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
> > > > struct intel_crtc_state *crtc_state,
> > > > - int min_scale, int max_scale,
> > > > + bool allow_scaling,
> > > > bool can_position)
> > > > {
> > > > struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> > > > @@ -897,19 +897,50 @@ int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
> > > > const struct drm_rect *clip = &crtc_state->pipe_src;
> > > > unsigned int rotation = plane_state->hw.rotation;
> > > > int hscale, vscale;
> > > > + int max_hscale, min_hscale, max_vscale, min_vscale;
> > > >
> > > > if (!fb) {
> > > > plane_state->uapi.visible = false;
> > > > return 0;
> > > > }
> > > >
> > > > + /*
> > > > + * At this point we don't really know the HW limitations, so
> > > > + * we just sanitize the values against the maximum supported
> > > > + * scaling.
> > > > + */
> > > > + if (allow_scaling) {
> > > > + min_vscale = 1;
> > > > + min_hscale = 1;
> > > > +
> > > > + if (DISPLAY_VER(i915) < 10 ||
> > > > + intel_format_info_is_yuv_semiplanar(fb->format,
> > > > + fb->modifier)) {
> > > > + max_vscale = 0x20000 - 1;
> > > > + max_hscale = 0x20000 - 1;
> > > > + } else {
> > > > + max_vscale = 0x30000 - 1;
> > > > + max_hscale = 0x30000 - 1;
> > > > + }
> > > > + } else {
> > > > + min_hscale = DRM_PLANE_NO_SCALING;
> > > > + max_hscale = DRM_PLANE_NO_SCALING;
> > > > + min_vscale = DRM_PLANE_NO_SCALING;
> > > > + max_vscale = DRM_PLANE_NO_SCALING;
> > > > + }
> > >
> > > I still don't see the point in moving this hw specific knowledge
> > > from the more hw specific files into the hw agnostic file.
> >
> > Is this file really that HW agnostic? I see lots of version checks with
> > "if (DISPLAY_VER(x))" all over the place.
>
> It's hw agnostic in the sense that most of it applies
> to all hw generations. And this function in particular is
> pretty much entirely hw agnostic currently.
>
> > As we discussed before, I think this kind of rules should be in HW-
> > specific configurations, but we don't have that yet. And I thought it
> > would be better to keep these decisions in a single place rather than
> > just calling functions in other files...
> >
> > If you prefer, I can move this back to skl_universal_plane.c or some
> > other of the skl_*.c files, but TBH they don't seem to be the right
> > place for this to me either...
>
> The current place knows exactly what kind of hardware/plane its
> dealing with, and thus knows its limits. Seems perfectly fine to me.
By "current place" you mean before this patch? I.e. in
skl_universal_plane.c?
> > [...]
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > index e6b4d24b9cd0..9ad1173a0551 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > @@ -1355,22 +1355,11 @@ g4x_sprite_check(struct intel_crtc_state *crtc_state,
> > > > {
> > > > struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> > > > struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > - int min_scale = DRM_PLANE_NO_SCALING;
> > > > - int max_scale = DRM_PLANE_NO_SCALING;
> > > > int ret;
> > > >
> > > > - if (g4x_fb_scalable(plane_state->hw.fb)) {
> > > > - if (DISPLAY_VER(dev_priv) < 7) {
> > > > - min_scale = 1;
> > > > - max_scale = 16 << 16;
> > > > - } else if (IS_IVYBRIDGE(dev_priv)) {
> > > > - min_scale = 1;
> > > > - max_scale = 2 << 16;
> > > > - }
> > > > - }
> > >
> > > So what happened to these limits?
> >
> > Oh, it seems that I lost them. I guess they should be moved to the
> > intel_atomic_plane_check_clipping() function. Again, to keep it all in
> > a single place. But this seems to be only required in the sprite code,
> > so I'm not sure what I can do.
> >
> > It's a problem to have this kinds of checks everywhere. 😞
> >
> >
> > [...]
> > > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > index 76490cc59d8f..e2ae6624378f 100644
> > > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > @@ -1463,22 +1463,6 @@ static int skl_plane_check_nv12_rotation(const struct intel_plane_state *plane_s
> > > > return 0;
> > > > }
> > > >
> > > > -static int skl_plane_max_scale(struct drm_i915_private *dev_priv,
> > > > - const struct drm_framebuffer *fb)
> > > > -{
> > > > - /*
> > > > - * We don't yet know the final source width nor
> > > > - * whether we can use the HQ scaler mode. Assume
> > > > - * the best case.
> > > > - * FIXME need to properly check this later.
> > > > - */
> > >
> > > Doesn't look like that FIXME has been dealt with as far
> > > as the hq scaler is concerned.
> >
> > We now check the limits _after_ having decided whether HQ mode is used.
> > So that should be covered, right?
>
> If we actaully add the hq mode check when determining the scaling
> limits.
Right, so the new FIXME I'll add as discussed above should cover this.
--
Cheers,
Luca.
