[PATCH v4 05/13] drm/msm/dpu: move scaling limitations out of the hw_catalog

[Dmitry Baryshkov]

On Fri, May 31, 2024 at 12:20:24PM -0700, Abhinav Kumar wrote:
> 
> 
> On 5/31/2024 1:16 AM, Dmitry Baryshkov wrote:
> > On Fri, 31 May 2024 at 04:02, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
> > > 
> > > 
> > > 
> > > On 3/13/2024 5:02 PM, Dmitry Baryshkov wrote:
> > > > Max upscale / downscale factors are constant between platforms. In
> > > > preparation to adding support for virtual planes and allocating SSPP
> > > > blocks on demand move max scaling factors out of the HW catalog and
> > > > handle them in the dpu_plane directly. If any of the scaling blocks gets
> > > > different limitations, this will have to be handled separately, after
> > > > the plane refactoring.
> > > > 
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> > > > ---
> > > >    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 12 ------------
> > > >    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  4 ----
> > > >    drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c      | 16 +++++++++++++---
> > > >    3 files changed, 13 insertions(+), 19 deletions(-)
> > > > 
> > > 
> > > <Snip>
> > > 
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > > index 70d6a8989e1a..6360052523b5 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > > @@ -785,12 +785,15 @@ static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu,
> > > >        return 0;
> > > >    }
> > > > 
> > > > +#define MAX_UPSCALE_RATIO    20
> > > > +#define MAX_DOWNSCALE_RATIO  4
> > > > +
> > > >    static int dpu_plane_atomic_check(struct drm_plane *plane,
> > > >                                  struct drm_atomic_state *state)
> > > >    {
> > > >        struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
> > > >                                                                                 plane);
> > > > -     int ret = 0, min_scale;
> > > > +     int ret = 0, min_scale, max_scale;
> > > >        struct dpu_plane *pdpu = to_dpu_plane(plane);
> > > >        struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
> > > >        u64 max_mdp_clk_rate = kms->perf.max_core_clk_rate;
> > > > @@ -822,10 +825,17 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> > > >        pipe_hw_caps = pipe->sspp->cap;
> > > >        sblk = pipe->sspp->cap->sblk;
> > > > 
> > > > -     min_scale = FRAC_16_16(1, sblk->maxupscale);
> > > > +     if (sblk->scaler_blk.len) {
> > > > +             min_scale = FRAC_16_16(1, MAX_UPSCALE_RATIO);
> > > > +             max_scale = MAX_DOWNSCALE_RATIO << 16;
> > > > +     } else {
> > > > +             min_scale = 1 << 16;
> > > > +             max_scale = 1 << 16;
> > > 
> > > You can use DRM_PLANE_NO_SCALING instead.
> > 
> > Ack
> > 
> > > 
> > > > +     }
> > > > +
> > > >        ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
> > > >                                                  min_scale,
> > > > -                                               sblk->maxdwnscale << 16,
> > > > +                                               max_scale,
> > > >                                                  true, true);
> > > 
> > > I am missing something here.
> > > 
> > > As per the documentation of this API, min and max are the scaling limits
> > > of both directions and not max_upscale and max_downscale.
> > > 
> > > **
> > > 837  * drm_atomic_helper_check_plane_state() - Check plane state for
> > > validity
> > > 838  * @plane_state: plane state to check
> > > 839  * @crtc_state: CRTC state to check
> > > 840  * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
> > > 841  * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
> > > 842  * @can_position: is it legal to position the plane such that it
> > > 
> > > 
> > > But this change is passing max_upscale and max_downscale as the min and
> > > max resp. Isnt that wrong?
> > 
> > First of all, please notice that I'm not changing the values that are
> > passed to the function. What was being passed beforehand gets passed
> > after this commit. I just moved it out of the catalog.
> > 
> 
> Ack.
> 
> > Second, if we take a look at drm_calc_scale(), we can see that it
> > calculates src / dst and checks that it is within the min_scale and
> > max_scale boundaries, just like documented.
> > In our case, the boundaries are (I'm omitting 16.16 math):
> > - upscale 20 times. dst = 20 * src, scale = src/dst = 1/20
> > - downscale 4 times. dst = 1/4 * src, scale = src/dst = 4
> > 
> > So, from the point of view of drm_calc_scale(), the min_scale is
> > 1/MAX_UPSCALE, max_scale = MAX_DOWNSCALE and the values the code is
> > passing are correct.
> > 
> 
> That part is fine. Agreed.
> 
> But I do think, that API is not correct if the scaling limits are different
> in the Horizontal Vs Vertical direction as today it assumes the limits are
> same in both.

Agree. But if we ever need to support different scaling limits, it would
be easy to extend the API.

> Anyway, thats outside the scope of this patch. So I am good
> for now.
> 
> > > 
> > > 
> > > >        if (ret) {
> > > >                DPU_DEBUG_PLANE(pdpu, "Check plane state failed (%d)\n", ret);
> > 
> > 
> > 

-- 
With best wishes
Dmitry