Re: [PATCH 2/3] drm/msm/disp/dpu1: allow dspp selection for all the interfaces

[Dmitry Baryshkov]

On 18/01/2023 05:30, Kalyan Thota wrote:
> > -----Original Message-----
> > From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > Sent: Tuesday, January 17, 2023 10:26 PM
> > To: Kalyan Thota (QUIC) <quic_kalyant@xxxxxxxxxxx>; dri-
> > devel@xxxxxxxxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx;
> > freedreno@xxxxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; robdclark@xxxxxxxxxxxx;
> > dianders@xxxxxxxxxxxx; swboyd@xxxxxxxxxxxx; Vinod Polimera (QUIC)
> > <quic_vpolimer@xxxxxxxxxxx>; Abhinav Kumar (QUIC)
> > <quic_abhinavk@xxxxxxxxxxx>
> > Subject: Re: [PATCH 2/3] drm/msm/disp/dpu1: allow dspp selection for all the
> > interfaces
> >
> > WARNING: This email originated from outside of Qualcomm. Please be wary of
> > any links or attachments, and do not enable macros.
> >
> > On 17/01/2023 18:21, Kalyan Thota wrote:
> > > Allow dspps to be populated as a requirement for all the encoder types
> > > it need not be just DSI. If for any encoder the dspp allocation
> > > doesn't go through then there can be an option to fallback for color
> > > features.
> > >
> > > Signed-off-by: Kalyan Thota <quic_kalyant@xxxxxxxxxxx>
> > > ---
> > >    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 18 +++++++++---------
> > >    1 file changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > index 9c6817b..e39b345 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder
> > *drm_enc)
> > >    static struct msm_display_topology dpu_encoder_get_topology(
> > >                        struct dpu_encoder_virt *dpu_enc,
> > >                        struct dpu_kms *dpu_kms,
> > > -                     struct drm_display_mode *mode)
> > > +                     struct drm_display_mode *mode,
> > > +                     struct drm_crtc_state *crtc_state)
> >
> > Is this new argument used at all?
> >
> > >    {
> > >        struct msm_display_topology topology = {0};
> > >        int i, intf_count = 0;
> > > @@ -563,8 +564,9 @@ static struct msm_display_topology
> > dpu_encoder_get_topology(
> > >         * 1 LM, 1 INTF
> > >         * 2 LM, 1 INTF (stream merge to support high resolution interfaces)
> > >         *
> > > -      * Adding color blocks only to primary interface if available in
> > > -      * sufficient number
> > > +      * dspp blocks are made optional. If RM manager cannot allocate
> > > +      * dspp blocks, then reservations will still go through with non dspp LM's
> > > +      * so as to allow color management support via composer
> > > + fallbacks
> > >         */
> >
> > No, this is not the way to go.
> >
> > First, RM should prefer non-DSPP-enabled LMs if DSPP blocks are not required.
> > Right now your patch makes it possible to allocate LMs, that have DSPP attached,
> > for non-CTM-enabled encoder and later fail allocation of DSPP for the CRTC
> > which has CTM blob attached.
> >
> > Second, the decision on using DSPPs should come from dpu_crtc_atomic_check().
> > Pass 'bool need_dspp' to this function from dpu_atomic_check(). Fail if the
> > need_dspp constraint can't be fulfilled.
> We may not get color_mgmt_changed property set during modeset commit, where as our resource allocation happens during modeset.

So, you have to fix the conditions to perform LM reallocation if CTM 
usage has changed (note, color_mgmt_changed is not a correct one here).

> With this approach, dspps will get allocated on first come first serve basis

I don't think that this is what we have agreed upon.

> @Rob, is it possible to send color management property during modeset, in that case, we can use it for dspp allocation to the datapath. The current approach doesn't assume it.
> On chrome compositor, I see that color property was being set in the subsequent commits but not in modeset.
>
> > >        if (intf_count == 2)
> > >                topology.num_lm = 2;
> > > @@ -573,11 +575,9 @@ static struct msm_display_topology
> > dpu_encoder_get_topology(
> > >        else
> > >                topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT)
> > > ? 2 : 1;
> > >
> > > -     if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
> > > -             if (dpu_kms->catalog->dspp &&
> > > -                     (dpu_kms->catalog->dspp_count >= topology.num_lm))
> > > -                     topology.num_dspp = topology.num_lm;
> > > -     }
> > > +     if (dpu_kms->catalog->dspp &&
> > > +         (dpu_kms->catalog->dspp_count >= topology.num_lm))
> > > +             topology.num_dspp = topology.num_lm;
> > >
> > >        topology.num_enc = 0;
> > >        topology.num_intf = intf_count;
> > > @@ -643,7 +643,7 @@ static int dpu_encoder_virt_atomic_check(
> > >                }
> > >        }
> > >
> > > -     topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
> > > +     topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode,
> > > + crtc_state);
> > >
> > >        /* Reserve dynamic resources now. */
> > >        if (!ret) {
> >
> > --
> > With best wishes
> > Dmitry

--
With best wishes
Dmitry