drm/msm/dpu: Support different number of DSC

[PATCH 2/2] drm/msm/dpu: configure DSC per number in use

Posted by Jun Nie 1 year, 4 months ago

Only 2 DSC engines are allowed, or no DSC is involved currently.
We need 4 DSC in quad-pipe topology in future. So let's only configure
DSC engines in use, instread of maximum number of DSC engines.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 39700b13e92f3..e8400b494687c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1871,10 +1871,13 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_ctl *ctl,
 		ctl->ops.update_pending_flush_dsc(ctl, hw_dsc->idx);
 }
 
-static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc,
-				 struct drm_dsc_config *dsc)
+static void dpu_encoder_prep_dsc(struct drm_encoder *drm_enc)
 {
 	/* coding only for 2LM, 2enc, 1 dsc config */
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+	struct dpu_crtc_state *cstate = to_dpu_crtc_state(drm_enc->crtc->state);
+	struct drm_dsc_config *dsc = dpu_enc->dsc;
+	int num_dsc = cstate->num_dscs;
 	struct dpu_encoder_phys *enc_master = dpu_enc->cur_master;
 	struct dpu_hw_ctl *ctl = enc_master->hw_ctl;
 	struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC];
@@ -1886,7 +1889,7 @@ static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc,
 	u32 initial_lines;
 	int i;
 
-	for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
+	for (i = 0; i < num_dsc; i++) {
 		hw_pp[i] = dpu_enc->hw_pp[i];
 		hw_dsc[i] = dpu_enc->hw_dsc[i];
 
@@ -1915,7 +1918,7 @@ static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc,
 	enc_ip_w = intf_ip_w / 2;
 	initial_lines = dpu_encoder_dsc_initial_line_calc(dsc, enc_ip_w);
 
-	for (i = 0; i < MAX_CHANNELS_PER_ENC; i++)
+	for (i = 0; i < num_dsc; i++)
 		dpu_encoder_dsc_pipe_cfg(ctl, hw_dsc[i], hw_pp[i],
 					 dsc, dsc_common_mode, initial_lines);
 }
@@ -1953,7 +1956,7 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc)
 	}
 
 	if (dpu_enc->dsc)
-		dpu_encoder_prep_dsc(dpu_enc, dpu_enc->dsc);
+		dpu_encoder_prep_dsc(drm_enc);
 }
 
 bool dpu_encoder_is_valid_for_commit(struct drm_encoder *drm_enc)

-- 
2.34.1

Re: [PATCH 2/2] drm/msm/dpu: configure DSC per number in use

Posted by Dmitry Baryshkov 1 year, 4 months ago

On Wed, Oct 09, 2024 at 02:38:43PM GMT, Jun Nie wrote:
> Only 2 DSC engines are allowed, or no DSC is involved currently.
> We need 4 DSC in quad-pipe topology in future. So let's only configure
> DSC engines in use, instread of maximum number of DSC engines.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 39700b13e92f3..e8400b494687c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1871,10 +1871,13 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_ctl *ctl,
>  		ctl->ops.update_pending_flush_dsc(ctl, hw_dsc->idx);
>  }
>  
> -static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc,
> -				 struct drm_dsc_config *dsc)
> +static void dpu_encoder_prep_dsc(struct drm_encoder *drm_enc)
>  {
>  	/* coding only for 2LM, 2enc, 1 dsc config */
> +	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> +	struct dpu_crtc_state *cstate = to_dpu_crtc_state(drm_enc->crtc->state);
> +	struct drm_dsc_config *dsc = dpu_enc->dsc;
> +	int num_dsc = cstate->num_dscs;

I have been thinking about this part for a while. Please move num_dscs
to the dpu_encoder_virt structure. The DSC blocks are logically related
to the encoder, so having this field in dpu_crtc_state seems incorrect.

>  	struct dpu_encoder_phys *enc_master = dpu_enc->cur_master;
>  	struct dpu_hw_ctl *ctl = enc_master->hw_ctl;
>  	struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC];

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] drm/msm/dpu: configure DSC per number in use

Posted by Dmitry Baryshkov 1 year, 4 months ago

On Wed, 9 Oct 2024 at 09:39, Jun Nie <jun.nie@linaro.org> wrote:
>
> Only 2 DSC engines are allowed, or no DSC is involved currently.

Can't parse this phrase.

> We need 4 DSC in quad-pipe topology in future. So let's only configure
> DSC engines in use, instread of maximum number of DSC engines.

Nit: instead

>
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 39700b13e92f3..e8400b494687c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1871,10 +1871,13 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_ctl *ctl,
>                 ctl->ops.update_pending_flush_dsc(ctl, hw_dsc->idx);
>  }
>
> -static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc,
> -                                struct drm_dsc_config *dsc)
> +static void dpu_encoder_prep_dsc(struct drm_encoder *drm_enc)
>  {
>         /* coding only for 2LM, 2enc, 1 dsc config */
> +       struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> +       struct dpu_crtc_state *cstate = to_dpu_crtc_state(drm_enc->crtc->state);
> +       struct drm_dsc_config *dsc = dpu_enc->dsc;

Why? This doesn't seem to be related to num_dscs introduction.

> +       int num_dsc = cstate->num_dscs;
>         struct dpu_encoder_phys *enc_master = dpu_enc->cur_master;
>         struct dpu_hw_ctl *ctl = enc_master->hw_ctl;
>         struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC];

[...]

> @@ -1953,7 +1956,7 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc)
>         }
>
>         if (dpu_enc->dsc)
> -               dpu_encoder_prep_dsc(dpu_enc, dpu_enc->dsc);
> +               dpu_encoder_prep_dsc(drm_enc);
>  }
>
>  bool dpu_encoder_is_valid_for_commit(struct drm_encoder *drm_enc)
>
> --
> 2.34.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/2] drm/msm/dpu: configure DSC per number in use

Posted by Jun Nie 1 year, 4 months ago

Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于2024年10月10日周四 06:10写道：
>
> On Wed, 9 Oct 2024 at 09:39, Jun Nie <jun.nie@linaro.org> wrote:
> >
> > Only 2 DSC engines are allowed, or no DSC is involved currently.
>
> Can't parse this phrase.

How about this:
If DSC is enabled, the only case is with 2 DSC engines so far.


>
> > We need 4 DSC in quad-pipe topology in future. So let's only configure
> > DSC engines in use, instread of maximum number of DSC engines.
>
> Nit: instead

Yep.
>
> >
> > Signed-off-by: Jun Nie <jun.nie@linaro.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 39700b13e92f3..e8400b494687c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -1871,10 +1871,13 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_ctl *ctl,
> >                 ctl->ops.update_pending_flush_dsc(ctl, hw_dsc->idx);
> >  }
> >
> > -static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc,
> > -                                struct drm_dsc_config *dsc)
> > +static void dpu_encoder_prep_dsc(struct drm_encoder *drm_enc)
> >  {
> >         /* coding only for 2LM, 2enc, 1 dsc config */
> > +       struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> > +       struct dpu_crtc_state *cstate = to_dpu_crtc_state(drm_enc->crtc->state);
> > +       struct drm_dsc_config *dsc = dpu_enc->dsc;
>
> Why? This doesn't seem to be related to num_dscs introduction.

You mean the comments above these 3 lines? Yeah, it should be removed.
>
> > +       int num_dsc = cstate->num_dscs;
> >         struct dpu_encoder_phys *enc_master = dpu_enc->cur_master;
> >         struct dpu_hw_ctl *ctl = enc_master->hw_ctl;
> >         struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC];
>
> [...]
>
> > @@ -1953,7 +1956,7 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc)
> >         }
> >
> >         if (dpu_enc->dsc)
> > -               dpu_encoder_prep_dsc(dpu_enc, dpu_enc->dsc);
> > +               dpu_encoder_prep_dsc(drm_enc);
> >  }
> >
> >  bool dpu_encoder_is_valid_for_commit(struct drm_encoder *drm_enc)
> >
> > --
> > 2.34.1
> >
>
>
> --
> With best wishes
> Dmitry

Re: [PATCH 2/2] drm/msm/dpu: configure DSC per number in use

Posted by Dmitry Baryshkov 1 year, 4 months ago

On Thu, 10 Oct 2024 at 04:47, Jun Nie <jun.nie@linaro.org> wrote:
>
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于2024年10月10日周四 06:10写道：
> >
> > On Wed, 9 Oct 2024 at 09:39, Jun Nie <jun.nie@linaro.org> wrote:
> > >
> > > Only 2 DSC engines are allowed, or no DSC is involved currently.
> >
> > Can't parse this phrase.
>
> How about this:
> If DSC is enabled, the only case is with 2 DSC engines so far.

Just:

Currently if DSC support is requested, the driver only supports using
2 DSC blocks.

>
>
> >
> > > We need 4 DSC in quad-pipe topology in future. So let's only configure
> > > DSC engines in use, instread of maximum number of DSC engines.
> >
> > Nit: instead
>
> Yep.
> >
> > >
> > > Signed-off-by: Jun Nie <jun.nie@linaro.org>
> > > ---
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++++-----
> > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > index 39700b13e92f3..e8400b494687c 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > @@ -1871,10 +1871,13 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_ctl *ctl,
> > >                 ctl->ops.update_pending_flush_dsc(ctl, hw_dsc->idx);
> > >  }
> > >
> > > -static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc,
> > > -                                struct drm_dsc_config *dsc)
> > > +static void dpu_encoder_prep_dsc(struct drm_encoder *drm_enc)
> > >  {
> > >         /* coding only for 2LM, 2enc, 1 dsc config */
> > > +       struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> > > +       struct dpu_crtc_state *cstate = to_dpu_crtc_state(drm_enc->crtc->state);
> > > +       struct drm_dsc_config *dsc = dpu_enc->dsc;
> >
> > Why? This doesn't seem to be related to num_dscs introduction.
>
> You mean the comments above these 3 lines? Yeah, it should be removed.

No, I mean that this whole chunk isn't related to the num_dsc support.
There is no need to change function arguments.

> >
> > > +       int num_dsc = cstate->num_dscs;
> > >         struct dpu_encoder_phys *enc_master = dpu_enc->cur_master;
> > >         struct dpu_hw_ctl *ctl = enc_master->hw_ctl;
> > >         struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC];
> >
> > [...]
> >
> > > @@ -1953,7 +1956,7 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc)
> > >         }
> > >
> > >         if (dpu_enc->dsc)
> > > -               dpu_encoder_prep_dsc(dpu_enc, dpu_enc->dsc);
> > > +               dpu_encoder_prep_dsc(drm_enc);
> > >  }
> > >
> > >  bool dpu_encoder_is_valid_for_commit(struct drm_encoder *drm_enc)
> > >
> > > --
> > > 2.34.1
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry



-- 
With best wishes
Dmitry