[v4] drm/msm/dpu: improve DSC allocation

  At DSC V1.1 DCE (Display Compression Engine) contains a DSC encoder.
However, at DSC V1.2 DCE consists of two DSC encoders, one has an odd
index and another one has an even index. Each encoder can work
independently. But only two DSC encoders from same DCE can be paired
to work together to support DSC merge mode at DSC V1.2. For DSC V1.1
two consecutive DSC encoders (start with even index) have to be paired
to support DSC merge mode.  In addition, the DSC with even index have
to be mapped to even PINGPONG index and DSC with odd index have to be
mapped to odd PINGPONG index at its data path in regardless of DSC
V1.1 or V1.2. This patch improves DSC allocation mechanism with
consideration of those factors.

Changes in V4:
-- rework commit message
-- use reserved_by_other()
-- add _dpu_rm_pingpong_next_index()
-- revise _dpu_rm_pingpong_dsc_check()

Changes in V3:
-- add dpu_rm_pingpong_dsc_check()
-- for pair allocation use i += 2 at for loop

Changes in V2:
    -- split _dpu_rm_reserve_dsc() into _dpu_rm_reserve_dsc_single() and
       _dpu_rm_reserve_dsc_pair()

Fixes: f2803ee91a41 ("drm/msm/disp/dpu1: Add DSC support in RM")
Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 178 ++++++++++++++++++++++++++++++---
 1 file changed, 163 insertions(+), 15 deletions(-)
  

On Tue, 12 Dec 2023 at 23:37, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> At DSC V1.1 DCE (Display Compression Engine) contains a DSC encoder.
> However, at DSC V1.2 DCE consists of two DSC encoders, one has an odd
> index and another one has an even index. Each encoder can work
> independently. But only two DSC encoders from same DCE can be paired
> to work together to support DSC merge mode at DSC V1.2. For DSC V1.1
> two consecutive DSC encoders (start with even index) have to be paired
> to support DSC merge mode.  In addition, the DSC with even index have
> to be mapped to even PINGPONG index and DSC with odd index have to be
> mapped to odd PINGPONG index at its data path in regardless of DSC
> V1.1 or V1.2. This patch improves DSC allocation mechanism with
> consideration of those factors.
>
> Changes in V4:
> -- rework commit message
> -- use reserved_by_other()
> -- add _dpu_rm_pingpong_next_index()
> -- revise _dpu_rm_pingpong_dsc_check()
>
> Changes in V3:
> -- add dpu_rm_pingpong_dsc_check()
> -- for pair allocation use i += 2 at for loop
>
> Changes in V2:
>     -- split _dpu_rm_reserve_dsc() into _dpu_rm_reserve_dsc_single() and
>        _dpu_rm_reserve_dsc_pair()
>
> Fixes: f2803ee91a41 ("drm/msm/disp/dpu1: Add DSC support in RM")
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 178 ++++++++++++++++++++++++++++++---
>  1 file changed, 163 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index f9215643..15317b6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -461,29 +461,177 @@ static int _dpu_rm_reserve_ctls(
>         return 0;
>  }
>
> -static int _dpu_rm_reserve_dsc(struct dpu_rm *rm,
> +static int _dpu_rm_pingpong_next_index(int start,
> +                                uint32_t enc_id,
> +                                uint32_t *pp_to_enc_id,
> +                                int pp_max)

Wrong indentation.

> +{
> +       int pp_ndx;
> +
> +       for (pp_ndx = start; pp_ndx < pp_max; pp_ndx++) {

git grep _ndx returns nearly nothing for the whole kernel. I think 'i'
is more than enough both here and in the loops over dsc_to_enc_id.

> +               if (pp_to_enc_id[pp_ndx] == enc_id)
> +                       return pp_ndx;
> +       }
> +
> +       return -ENAVAIL;
> +}
> +
> +static int _dpu_rm_pingpong_dsc_check(int dsc_ndx, int pp_ndx)

Same here. It is _idx or _index.

> +{
> +
> +       /*
> +        * dsc even index must be mapped to pingpong even index

DSC, PINGPONG. I think I had this comment for v3. You fixed the commit
message. What stopped you from fixing the comment?

DSC with even index must be used with the PINGPONG with even index

> +        * dsc odd index must be mapped to pingpong odd index
> +        */
> +       if ((dsc_ndx & 0x01) != (pp_ndx & 0x01))
> +               return -ENAVAIL;
> +
> +       return 0;
> +}
> +
> +static int _dpu_rm_reserve_dsc_single(struct dpu_rm *rm,

Still _single. Even though it can get a request for two DSC blocks.

>                                struct dpu_global_state *global_state,
> -                              struct drm_encoder *enc,
> +                              uint32_t enc_id,
> +                              int *dsc_id,
>                                const struct msm_display_topology *top)

Wrong indentation.

>  {
> -       int num_dsc = top->num_dsc;
> -       int i;
> +       int num_dsc = 0;
> +       uint32_t *pp_to_enc_id = global_state->pingpong_to_enc_id;
> +       uint32_t *dsc_enc_id = global_state->dsc_to_enc_id;
> +       int pp_max = PINGPONG_MAX - PINGPONG_0;
> +       int pp_ndx;
> +       int dsc_ndx;
> +       int ret;
>
> -       /* check if DSC required are allocated or not */
> -       for (i = 0; i < num_dsc; i++) {
> -               if (!rm->dsc_blks[i]) {
> -                       DPU_ERROR("DSC %d does not exist\n", i);
> -                       return -EIO;
> -               }
> +       for (dsc_ndx = 0; dsc_ndx < ARRAY_SIZE(rm->dsc_blks); dsc_ndx++) {
> +               if (!rm->dsc_blks[dsc_ndx])
> +                       continue;
> +
> +               if (reserved_by_other(dsc_enc_id, dsc_ndx, enc_id))
> +                       continue;
> +
> +               pp_ndx = _dpu_rm_pingpong_next_index(0, enc_id, pp_to_enc_id, pp_max);

Error here.

> +               if (pp_ndx < 0)
> +                       return -ENAVAIL;
> +
> +               ret = _dpu_rm_pingpong_dsc_check(dsc_ndx, pp_ndx);
> +               if (ret)
> +                       return -ENAVAIL;
> +
> +               dsc_id[num_dsc++] = DSC_0 + dsc_ndx;    /* found */

The comment is useless.

> +
> +               if (num_dsc >= top->num_dsc)
> +                       break;

Can this go to the loop condition please.

> +       }
>
> -               if (global_state->dsc_to_enc_id[i]) {
> -                       DPU_ERROR("DSC %d is already allocated\n", i);
> -                       return -EIO;
> +       if (num_dsc < top->num_dsc) {
> +               DPU_ERROR("DSC allocation failed num_dsc=%d required=%d\n",
> +                                               num_dsc, top->num_dsc);
> +               return -ENAVAIL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int _dpu_rm_reserve_dsc_pair(struct dpu_rm *rm,
> +                              struct dpu_global_state *global_state,
> +                              uint32_t enc_id,
> +                              int *dsc_id,
> +                              const struct msm_display_topology *top)

Wrong indentation

> +{
> +       int num_dsc = 0;
> +       uint32_t *pp_to_enc_id = global_state->pingpong_to_enc_id;
> +       uint32_t *dsc_enc_id = global_state->dsc_to_enc_id;
> +       int pp_max = PINGPONG_MAX - PINGPONG_0;
> +       int start_pp_ndx = 0;
> +       int dsc_ndx, pp_ndx;
> +       int ret;
> +
> +       /* only start from even dsc index */
> +       for (dsc_ndx = 0; dsc_ndx < ARRAY_SIZE(rm->dsc_blks); dsc_ndx += 2) {
> +               if (!rm->dsc_blks[dsc_ndx] || !rm->dsc_blks[dsc_ndx + 1])

Newline after ||

> +                       continue;
> +
> +               /* consective dsc index to be paired */
> +               if (reserved_by_other(dsc_enc_id, dsc_ndx, enc_id) ||
> +                               reserved_by_other(dsc_enc_id, dsc_ndx + 1, enc_id))

Wrong indentation

> +                       continue;
> +
> +               pp_ndx = _dpu_rm_pingpong_next_index(start_pp_ndx, enc_id, pp_to_enc_id, pp_max);

Inline start_pp_ndx here.

> +               if (pp_ndx < 0)
> +                       return -ENAVAIL;
> +
> +               ret = _dpu_rm_pingpong_dsc_check(dsc_ndx, pp_ndx);
> +               if (ret) {
> +                       pp_ndx = 0;
> +                       continue;
>                 }
> +
> +               pp_ndx = _dpu_rm_pingpong_next_index(pp_ndx + 1, enc_id, pp_to_enc_id, pp_max);
> +               if (pp_ndx < 0)
> +                       return -ENAVAIL;
> +
> +               /* pair found */
> +               dsc_id[num_dsc++] = DSC_0 + dsc_ndx;
> +               dsc_id[num_dsc++] = DSC_0 + dsc_ndx + 1;
> +
> +               start_pp_ndx = pp_ndx + 1;      /* start for next pair */
> +
> +               if (num_dsc >= top->num_dsc)
> +                       break;

Can this go the loop condition, please.

> +       }
> +
> +       if (num_dsc < top->num_dsc) {
> +               DPU_ERROR("DSC allocation failed num_dsc=%d required=%d\n",
> +                                               num_dsc, top->num_dsc);
> +               return -ENAVAIL;
>         }
>
> -       for (i = 0; i < num_dsc; i++)
> -               global_state->dsc_to_enc_id[i] = enc->base.id;
> +       return 0;
> +}
> +
> +static int _dpu_rm_reserve_dsc(struct dpu_rm *rm,
> +                              struct dpu_global_state *global_state,
> +                              struct drm_encoder *enc,
> +                              const struct msm_display_topology *top)
> +{
> +       uint32_t enc_id = enc->base.id;
> +       int dsc_id[DSC_MAX - DSC_0];

Drop the dsc_id.

> +       int i, ret;
> +
> +       if (!top->num_dsc || !top->num_intf)
> +               return 0;
> +
> +       memset(dsc_id, 0, sizeof(dsc_id));
> +
> +       /*
> +        * Truth:
> +        * 1) every layer mixer only connects to one interface

Hmm. I missed this before. Bonded DSI mode. Each LM is used for both
interfaces, isn't it?

> +        * 2) no pingpong split -- which is two layer mixers shared one pingpong
> +        * 3) DSC pair start from even index, such as index(0,1), index (2,3), etc
> +        * 4) odd pingpong connect to odd dsc
> +        * 5) even pingpong connect to even dsc

You know the drill.

> +        * 6) pair: encoder +--> pp_idx_0 --> dsc_idx_0
> +                           +--> pp_idx_1 --> dsc_idx_1
> +        */
> +
> +       /* num_dsc should be either 1, 2 or 4 */
> +       if (top->num_dsc > top->num_intf)       /* merge mode */
> +               ret =  _dpu_rm_reserve_dsc_pair(rm, global_state, enc_id, dsc_id, top);
> +       else
> +               ret =  _dpu_rm_reserve_dsc_single(rm, global_state, enc_id, dsc_id, top);
> +
> +       if (ret)
> +               return ret;
> +
> +       /* everything is good proceed to allocate dsc */
> +       for (i = 0; i < top->num_dsc; i++) {
> +               int id;
> +
> +               id = dsc_id[i];
> +               if (id >= DSC_0)
> +                       global_state->dsc_to_enc_id[id - DSC_0] = enc_id;
> +       }
>
>         return 0;
>  }
> --
> 2.7.4
>