On Tue, 12 Dec 2023 at 23:37, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
> 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
>
--
With best wishes
Dmitry
