| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <dab2beed-78de-6638-8389-d3e03c8cfc44@quicinc.com>
Date: Thu, 14 Dec 2023 09:34:05 -0800
From: Kuogee Hsieh <quic_khsieh@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: <dri-devel@...ts.freedesktop.org>, <robdclark@...il.com>,
<sean@...rly.run>, <swboyd@...omium.org>, <dianders@...omium.org>,
<vkoul@...nel.org>, <daniel@...ll.ch>, <airlied@...il.com>,
<agross@...nel.org>, <andersson@...nel.org>,
<quic_abhinavk@...cinc.com>, <quic_jesszhan@...cinc.com>,
<quic_sbillaka@...cinc.com>, <marijn.suijten@...ainline.org>,
<freedreno@...ts.freedesktop.org>, <linux-arm-msm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5] drm/msm/dpu: improve DSC allocation
On 12/13/2023 3:00 PM, Dmitry Baryshkov wrote:
> On Wed, 13 Dec 2023 at 20:58, Kuogee Hsieh <quic_khsieh@...cinc.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@...cinc.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?
>
Powered by blists - more mailing lists