Re: [PATCH v1 3/5] drm/msm/dpu: save dpu topology configuration

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 4/20/2023 5:12 PM, Dmitry Baryshkov wrote:
> On 21/04/2023 02:25, Kuogee Hsieh wrote:
> > At current implementation, topology configuration is thrown away after
> > dpu_rm_reserve(). This patch save the topology so that it can be used
> > for DSC related calculation later.
>
> Please take a look at 
> https://patchwork.freedesktop.org/patch/527960/?series=115283&rev=2 .

Let the review of this series go on and lets try to get the acks on the 
non-topology related pieces. I think 2/5 patches in this series are 
conflicting in the design. We will resolve that in a weeks time. 
Meanwhile, I think we can keep the reviews / versions going on the rest.

I think we can move patch 5 of this series to patch 3. That way we get 
acks on patches 1-3 and patches 4 & 5 which deal with topology are dealt 
with together with virtual planes.

I will review virtual planes next week and we will decide the best 
course of action. Moving resource allocation to CRTC needs to be thought 
of a bit deeper for DSC as that one is directly tied to encoder.

> > Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32 
> > ++++++++++++++---------------
> >   1 file changed, 16 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index ecb87bc..2fdacf1 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -542,13 +542,13 @@ bool dpu_encoder_use_dsc_merge(struct 
> > drm_encoder *drm_enc)
> >       return (num_dsc > 0) && (num_dsc > intf_count);
> >   }
> > -static struct msm_display_topology dpu_encoder_get_topology(
> > +static void dpu_encoder_get_topology(
> >               struct dpu_encoder_virt *dpu_enc,
> >               struct dpu_kms *dpu_kms,
> >               struct drm_display_mode *mode,
> > -            struct drm_crtc_state *crtc_state)
> > +            struct drm_crtc_state *crtc_state,
> > +            struct msm_display_topology *topology)
> >   {
> > -    struct msm_display_topology topology = {0};
> >       int i, intf_count = 0;
> >       for (i = 0; i < MAX_PHYS_ENCODERS_PER_VIRTUAL; i++)
> > @@ -567,16 +567,16 @@ static struct msm_display_topology 
> > dpu_encoder_get_topology(
> >        * Add dspps to the reservation requirements if ctm is requested
> >        */
> >       if (intf_count == 2)
> > -        topology.num_lm = 2;
> > +        topology->num_lm = 2;
> >       else if (!dpu_kms->catalog->caps->has_3d_merge)
> > -        topology.num_lm = 1;
> > +        topology->num_lm = 1;
> >       else
> > -        topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
> > +        topology->num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 
> > : 1;
> >       if (crtc_state->ctm)
> > -        topology.num_dspp = topology.num_lm;
> > +        topology->num_dspp = topology->num_lm;
> > -    topology.num_intf = intf_count;
> > +    topology->num_intf = intf_count;
> >       if (dpu_enc->dsc) {
> >           /*
> > @@ -585,12 +585,10 @@ static struct msm_display_topology 
> > dpu_encoder_get_topology(
> >            * this is power optimal and can drive up to (including) 4k
> >            * screens
> >            */
> > -        topology.num_dsc = 2;
> > -        topology.num_lm = 2;
> > -        topology.num_intf = 1;
> > +        topology->num_dsc = 2;
> > +        topology->num_lm = 2;
> > +        topology->num_intf = 1;
> >       }
> > -
> > -    return topology;
> >   }
> >   static int dpu_encoder_virt_atomic_check(
> > @@ -602,7 +600,7 @@ static int dpu_encoder_virt_atomic_check(
> >       struct msm_drm_private *priv;
> >       struct dpu_kms *dpu_kms;
> >       struct drm_display_mode *adj_mode;
> > -    struct msm_display_topology topology;
> > +    struct msm_display_topology *topology;
> >       struct dpu_global_state *global_state;
> >       int i = 0;
> >       int ret = 0;
> > @@ -639,7 +637,9 @@ static int dpu_encoder_virt_atomic_check(
> >           }
> >       }
> > -    topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, 
> > crtc_state);
> > +    topology = &dpu_enc->topology;
> > +    memset(topology, 0, sizeof (*topology));
> > +    dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, 
> > topology);
> >       /*
> >        * Release and Allocate resources on every modeset
> > @@ -650,7 +650,7 @@ static int dpu_encoder_virt_atomic_check(
> >           if (!crtc_state->active_changed || crtc_state->enable)
> >               ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> > -                    drm_enc, crtc_state, topology);
> > +                    drm_enc, crtc_state, *topology);
> >       }
> >       trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);