Messages in this thread | | | Date | Thu, 14 Dec 2023 09:34:05 -0800 | Subject | Re: [PATCH v5] drm/msm/dpu: improve DSC allocation | From | Kuogee Hsieh <> |
| |
On 12/13/2023 3:00 PM, Dmitry Baryshkov wrote: > On Wed, 13 Dec 2023 at 20:58, 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 V5: >> -- delete dsc_id[] >> -- update to global_state->dsc_to_enc_id[] directly >> -- replace ndx with idx >> -- fix indentation at function declaration >> -- only one for loop at _dpu_rm_reserve_dsc_single() >> >> 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 | 162 +++++++++++++++++++++++++++++---- >> 1 file changed, 146 insertions(+), 16 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..7c7a88f 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> @@ -461,29 +461,159 @@ static int _dpu_rm_reserve_ctls( >> 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) >> +static int _dpu_rm_pingpong_next_index(int start, >> + uint32_t enc_id, >> + uint32_t *pp_to_enc_id, >> + int pp_max) >> { >> - int num_dsc = top->num_dsc; >> int i; >> >> - /* 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 (i = start; i < pp_max; i++) { >> + if (pp_to_enc_id[i] == enc_id) >> + return i; >> + } >> + >> + return -ENAVAIL; >> +} >> + >> +static int _dpu_rm_pingpong_dsc_check(int dsc_idx, int pp_idx) >> +{ >> + > CHECK: Blank lines aren't necessary after an open brace '{' > #85: FILE: drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c:481: > >> + /* >> + * DSC with even index must be used with the PINGPONG with even index >> + * DSC with odd index must be used with the PINGPONG with odd index >> + */ >> + if ((dsc_idx & 0x01) != (pp_idx & 0x01)) >> + return -ENAVAIL; >> + >> + return 0; >> +} >> + >> +static int _dpu_rm_reserve_dsc_single(struct dpu_rm *rm, >> + struct dpu_global_state *global_state, >> + uint32_t enc_id, >> + const struct msm_display_topology *top) >> +{ >> + 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_idx; >> + int dsc_idx; >> + int ret; >> + >> + for (dsc_idx = 0; dsc_idx < ARRAY_SIZE(rm->dsc_blks) && >> + num_dsc < 1; dsc_idx++) { > The condition is wrong here. Also it is misaligned.
i will remove checking num_dsc < 1 here and add break at end of body of for loop since it only allocate one dsc
> >> + if (!rm->dsc_blks[dsc_idx]) >> + continue; >> + >> + if (reserved_by_other(dsc_enc_id, dsc_idx, enc_id)) >> + continue; >> + >> + pp_idx = _dpu_rm_pingpong_next_index(0, enc_id, > And this is wrong too. You should start relatively to your previous PP index.
It does not have previous pp_index since it only allocate on dsc.
> >> + pp_to_enc_id, pp_max); >> + if (pp_idx < 0) >> + return -ENAVAIL; >> + >> + ret = _dpu_rm_pingpong_dsc_check(dsc_idx, pp_idx); >> + if (ret) >> + return -ENAVAIL; >> + >> + dsc_enc_id[dsc_idx] = enc_id; >> + num_dsc++; >> + } >> + >> + if (!num_dsc) { >> + DPU_ERROR("DSC allocation failed num_dsc=%d\n", num_dsc); >> + return -ENAVAIL; >> + } >> >> - if (global_state->dsc_to_enc_id[i]) { >> - DPU_ERROR("DSC %d is already allocated\n", i); >> - return -EIO; >> + return 0; >> +} >> + >> +static int _dpu_rm_reserve_dsc_pair(struct dpu_rm *rm, >> + struct dpu_global_state *global_state, >> + uint32_t enc_id, >> + const struct msm_display_topology *top) >> +{ >> + 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; > No need for these anymore. Please inline them. Or simply pass > global_state to _dpu_rm_pingpong_next_index(). > Other functions in dpu_rm.c don't define local variables for these > arrays. I don't see why this patch should deviate from that. > >> + int pp_max = PINGPONG_MAX - PINGPONG_0; >> + int start_pp_idx = 0; >> + int dsc_idx, pp_idx; >> + int ret; >> + >> + /* only start from even dsc index */ >> + for (dsc_idx = 0; dsc_idx < ARRAY_SIZE(rm->dsc_blks) && >> + num_dsc < top->num_dsc; dsc_idx += 2) { > Misaligned > >> + if (!rm->dsc_blks[dsc_idx] || >> + !rm->dsc_blks[dsc_idx + 1]) >> + continue; >> + >> + /* consective dsc index to be paired */ >> + if (reserved_by_other(dsc_enc_id, dsc_idx, enc_id) || >> + reserved_by_other(dsc_enc_id, dsc_idx + 1, enc_id)) >> + continue; >> + >> + pp_idx = _dpu_rm_pingpong_next_index(start_pp_idx, enc_id, >> + pp_to_enc_id, pp_max); >> + if (pp_idx < 0) >> + return -ENAVAIL; >> + >> + ret = _dpu_rm_pingpong_dsc_check(dsc_idx, pp_idx); >> + if (ret) { >> + pp_idx = 0; >> + continue; >> } >> + >> + pp_idx = _dpu_rm_pingpong_next_index(pp_idx + 1, enc_id, >> + pp_to_enc_id, pp_max); >> + if (pp_idx < 0) >> + return -ENAVAIL; > Fresh pp_idx has to be checked against dsc_idx + 1. > > Let me also have a suggestion for you. The pp_max is a constant. You > don't have to pass it to _dpu_rm_pingpong_next_index() at all! Also if > you change the function to accept enum dpu_pingpong, you can start > with PINGPONG_NONE and move +1 into the function, making the callers > simpler, removing the need or start_pp_idx (which I asked to do in v4) > etc. > >> + >> + dsc_enc_id[dsc_idx] = enc_id; >> + dsc_enc_id[dsc_idx + 1] = enc_id; >> + num_dsc += 2; >> + >> + start_pp_idx = pp_idx + 1; /* start for next pair */ >> } >> >> - for (i = 0; i < num_dsc; i++) >> - global_state->dsc_to_enc_id[i] = enc->base.id; >> + if (num_dsc < top->num_dsc) { >> + DPU_ERROR("DSC allocation failed num_dsc=%d required=%d\n", >> + num_dsc, top->num_dsc); > Misaligned > >> + return -ENAVAIL; >> + } >> + >> + 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; >> + >> + if (!top->num_dsc || !top->num_intf) >> + return 0; >> + >> + /* >> + * Facts: >> + * 1) DSCs ouput to an interface > WARNING: 'ouput' may be misspelled - perhaps 'output'? > > Also, what does it bring to us? > >> + * 2) no pingpong split (two layer mixers shared one pingpong) >> + * 3) DSC pair start from even index, such as index(0,1), (2,3), etc > starts > >> + * 4) even PINGPONG connects to even DSC >> + * 5) odd PINGPONG connects to odd DSC >> + * 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 */ >> + return _dpu_rm_reserve_dsc_pair(rm, global_state, enc_id, top); >> + else >> + return _dpu_rm_reserve_dsc_single(rm, global_state, enc_id, top); >> >> return 0; >> } >> -- >> 2.7.4 >> > Kuogee, we value your patches. But could you please fix your editor > settings to properly align C statements? E.g. Vim has the "set > cino=(0" setting, which does most of the work. I suspect that your > code editor should also have a similar setting. Also could you please > establish a practice of using checkpatch.pl at least until we stop > hitting obvious issues there? >
| |