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@xxxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > > 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
