Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <705f16dd-2d66-1d1a-41ed-1a9fb8e9a568@quicinc.com>
Date:   Wed, 26 Apr 2023 09:09:29 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v1 1/5] drm/msm/dpu: add support for DSC encoder v1.2
 engine
Content-Language: en-US
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        <dri-devel@lists.freedesktop.org>, <robdclark@gmail.com>,
        <sean@poorly.run>, <swboyd@chromium.org>, <dianders@chromium.org>,
        <vkoul@kernel.org>, <daniel@ffwll.ch>, <airlied@gmail.com>,
        <agross@kernel.org>, <andersson@kernel.org>
CC:     <quic_abhinavk@quicinc.com>, <quic_sbillaka@quicinc.com>,
        <marijn.suijten@somainline.org>, <freedreno@lists.freedesktop.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <1682033114-28483-1-git-send-email-quic_khsieh@quicinc.com>
 <1682033114-28483-2-git-send-email-quic_khsieh@quicinc.com>
 <5a9ae243-47ce-9238-8e2d-4942054e8a13@linaro.org>
From:   Kuogee Hsieh <quic_khsieh@quicinc.com>
In-Reply-To: <5a9ae243-47ce-9238-8e2d-4942054e8a13@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 4/20/2023 4:54 PM, Dmitry Baryshkov wrote:
> On 21/04/2023 02:25, Kuogee Hsieh wrote:
>> Add support for DSC 1.2 by providing the necessary hooks to program
>> the DPU DSC 1.2 encoder.
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/Makefile                   |   1 +
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  38 ++-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h     |  17 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c | 388 
>> +++++++++++++++++++++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c         |   7 +-
>>   5 files changed, 444 insertions(+), 7 deletions(-)
>>   create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
>>
>> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
>> index b814fc8..b9af5e4 100644
>> --- a/drivers/gpu/drm/msm/Makefile
>> +++ b/drivers/gpu/drm/msm/Makefile
>> @@ -65,6 +65,7 @@ msm-$(CONFIG_DRM_MSM_DPU) += \
>>       disp/dpu1/dpu_hw_catalog.o \
>>       disp/dpu1/dpu_hw_ctl.o \
>>       disp/dpu1/dpu_hw_dsc.o \
>> +    disp/dpu1/dpu_hw_dsc_1_2.o \
>>       disp/dpu1/dpu_hw_interrupts.o \
>>       disp/dpu1/dpu_hw_intf.o \
>>       disp/dpu1/dpu_hw_lm.o \
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> index 71584cd..22421b9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -1,6 +1,6 @@
>>   /* SPDX-License-Identifier: GPL-2.0-only */
>>   /*
>> - * Copyright (c) 2022. Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + * Copyright (c) 2022-2023, Qualcomm Innovation Center, Inc. All 
>> rights reserved.
>>    * Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights 
>> reserved.
>>    */
>>   @@ -241,12 +241,20 @@ enum {
>>   };
>>     /**
>> - * DSC features
>> - * @DPU_DSC_OUTPUT_CTRL       Configure which PINGPONG block gets
>> - *                            the pixel output from this DSC.
>> + * DSC sub-blocks/features
>> + * @DPU_DSC_OUTPUT_CTRL         Configure which PINGPONG block gets
>> + *                              the pixel output from this DSC.
>
> Any reason to change the alignment
>
>> + * @DPU_DSC_HW_REV_1_1          DSC block supports dsc 1.1 only
>> + * @DPU_DSC_HW_REV_1_2          DSC block supports dsc 1.1 and 1.2
>> + * @DPU_DSC_NATIVE_422_EN       Supports native422 and native420 
>> encoding
>> + * @DPU_DSC_MAX
>>    */
>>   enum {
>>       DPU_DSC_OUTPUT_CTRL = 0x1,
>> +    DPU_DSC_HW_REV_1_1,
>> +    DPU_DSC_HW_REV_1_2,
>> +    DPU_DSC_NATIVE_422_EN,
>> +    DPU_DSC_MAX
>>   };
>>     /**
>> @@ -311,6 +319,14 @@ struct dpu_pp_blk {
>>   };
>>     /**
>> + * struct dpu_dsc_blk - DSC Encoder sub-blk information
>> + * @info:   HW register and features supported by this sub-blk
>> + */
>> +struct dpu_dsc_blk {
>> +    DPU_HW_SUBBLK_INFO;
>> +};
>> +
>> +/**
>>    * enum dpu_qos_lut_usage - define QoS LUT use cases
>>    */
>>   enum dpu_qos_lut_usage {
>> @@ -459,6 +475,17 @@ struct dpu_pingpong_sub_blks {
>>   };
>>     /**
>> + * struct dpu_dsc_sub_blks - DSC sub-blks
>> + * @enc: DSC encoder sub block
>> + * @ctl: DSC controller sub block
>> + *
>> + */
>> +struct dpu_dsc_sub_blks {
>> +    struct dpu_dsc_blk enc;
>> +    struct dpu_dsc_blk ctl;
>> +};
>> +
>> +/**
>>    * dpu_clk_ctrl_type - Defines top level clock control signals
>>    */
>>   enum dpu_clk_ctrl_type {
>> @@ -612,10 +639,13 @@ struct dpu_merge_3d_cfg  {
>>    * struct dpu_dsc_cfg - information of DSC blocks
>>    * @id                 enum identifying this block
>>    * @base               register offset of this block
>> + * @len:               length of hardware block
>>    * @features           bit mask identifying sub-blocks/features
>> + * @sblk               sub-blocks information
>>    */
>>   struct dpu_dsc_cfg {
>>       DPU_HW_BLK_INFO;
>> +    const struct dpu_dsc_sub_blks *sblk;
>>   };
>>     /**
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
>> index ae9b5db..d0f8b8b 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
>> @@ -1,5 +1,8 @@
>>   /* SPDX-License-Identifier: GPL-2.0-only */
>> -/* Copyright (c) 2020-2022, Linaro Limited */
>> +/*
>> + * Copyright (c) 2020-2022, Linaro Limited
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>> reserved
>> + */
>>     #ifndef _DPU_HW_DSC_H
>>   #define _DPU_HW_DSC_H
>> @@ -61,7 +64,7 @@ struct dpu_hw_dsc {
>>   };
>>     /**
>> - * dpu_hw_dsc_init - initializes the dsc block for the passed dsc idx.
>> + * dpu_hw_dsc_init - initializes the v1.1 dsc block for the passed 
>> dsc idx.
>>    * @idx:  DSC index for which driver object is required
>>    * @addr: Mapped register io address of MDP
>>    * @m:    Pointer to mdss catalog data
>> @@ -71,6 +74,16 @@ struct dpu_hw_dsc *dpu_hw_dsc_init(enum dpu_dsc 
>> idx, void __iomem *addr,
>>                      const struct dpu_mdss_cfg *m);
>>     /**
>> + * dpu_hw_dsc_init_1_2 - initializes the v1.2 dsc block for the 
>> passed dsc idx.
>> + * @idx:  DSC index for which driver object is required
>> + * @addr: Mapped register io address of MDP
>> + * @m:    Pointer to mdss catalog data
>> + * Returns: Error code or allocated dpu_hw_dsc context
>> + */
>> +struct dpu_hw_dsc *dpu_hw_dsc_init_1_2(enum dpu_dsc idx, void 
>> __iomem *addr,
>> +                   const struct dpu_mdss_cfg *m);
>> +
>> +/**
>>    * dpu_hw_dsc_destroy - destroys dsc driver context
>>    * @dsc:   Pointer to dsc driver context returned by dpu_hw_dsc_init
>>    */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
>> new file mode 100644
>> index 00000000..455d7f2
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
>> @@ -0,0 +1,388 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>> reserved
>> + */
>> +
>> +#include <drm/display/drm_dsc_helper.h>
>> +
>> +#include "dpu_kms.h"
>> +#include "dpu_hw_catalog.h"
>> +#include "dpu_hwio.h"
>> +#include "dpu_hw_mdss.h"
>> +#include "dpu_hw_dsc.h"
>> +
>> +
>> +#define DSC_CMN_MAIN_CNF           0x00
>> +
>> +/* DPU_DSC_ENC register offsets */
>> +#define ENC_DF_CTRL                0x00
>> +#define ENC_GENERAL_STATUS         0x04
>> +#define ENC_HSLICE_STATUS          0x08
>> +#define ENC_OUT_STATUS             0x0C
>> +#define ENC_INT_STAT               0x10
>> +#define ENC_INT_CLR                0x14
>> +#define ENC_INT_MASK               0x18
>> +#define DSC_MAIN_CONF              0x30
>> +#define DSC_PICTURE_SIZE           0x34
>> +#define DSC_SLICE_SIZE             0x38
>> +#define DSC_MISC_SIZE              0x3C
>> +#define DSC_HRD_DELAYS             0x40
>> +#define DSC_RC_SCALE               0x44
>> +#define DSC_RC_SCALE_INC_DEC       0x48
>> +#define DSC_RC_OFFSETS_1           0x4C
>> +#define DSC_RC_OFFSETS_2           0x50
>> +#define DSC_RC_OFFSETS_3           0x54
>> +#define DSC_RC_OFFSETS_4           0x58
>> +#define DSC_FLATNESS_QP            0x5C
>> +#define DSC_RC_MODEL_SIZE          0x60
>> +#define DSC_RC_CONFIG              0x64
>> +#define DSC_RC_BUF_THRESH_0        0x68
>> +#define DSC_RC_BUF_THRESH_1        0x6C
>> +#define DSC_RC_BUF_THRESH_2        0x70
>> +#define DSC_RC_BUF_THRESH_3        0x74
>> +#define DSC_RC_MIN_QP_0            0x78
>> +#define DSC_RC_MIN_QP_1            0x7C
>> +#define DSC_RC_MIN_QP_2            0x80
>> +#define DSC_RC_MAX_QP_0            0x84
>> +#define DSC_RC_MAX_QP_1            0x88
>> +#define DSC_RC_MAX_QP_2             0x8C
>
> As you are adding new definitions, could please make them aligned from 
> the beginning?
>
>> +#define DSC_RC_RANGE_BPG_OFFSETS_0  0x90
>> +#define DSC_RC_RANGE_BPG_OFFSETS_1  0x94
>> +#define DSC_RC_RANGE_BPG_OFFSETS_2  0x98
>> +
>> +/* DPU_DSC_CTL register offsets */
>> +#define DSC_CTL                    0x00
>> +#define DSC_CFG                    0x04
>> +#define DSC_DATA_IN_SWAP           0x08
>> +#define DSC_CLK_CTRL               0x0C
>> +
>> +/*
>> + * @DPU_DSC_ENC                 DSC encoder sub block
>> + * @DPU_DSC_CTL                 DSC controller sub block
>> + */
>> +enum {
>> +    DPU_DSC_ENC,
>> +    DPU_DSC_CTL,
>> +};
>> +
>> +static int _dsc_calc_ob_max_addr(struct dpu_hw_dsc *hw_dsc, int num_ss)
>
> If this is used from a single place, please inline this function.
>
>> +{
>> +    int max_addr = 2400 / num_ss;
>> +
>> +    if (hw_dsc->caps->features & BIT(DPU_DSC_NATIVE_422_EN))
>> +        max_addr /= 2;
>
> Is this really guarded by the hw block feature or by native_422 being 
> enabled?
>
>> +
>> +    return max_addr - 1;
>> +};
>> +
>>
>
> [skipped]
>
>> +static void dpu_hw_dsc_config_thresh_1_2(struct dpu_hw_dsc *hw_dsc,
>> +                    struct drm_dsc_config *dsc)
>> +{
>> +    struct dpu_hw_blk_reg_map *hw;
>> +    u32 offset, off;
>> +    int i, j = 0;
>> +    struct drm_dsc_rc_range_parameters *rc;
>> +    u32 data = 0, min_qp = 0, max_qp = 0, bpg_off = 0;
>> +
>> +    if (!hw_dsc || !dsc)
>> +        return;
>> +
>> +    _dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &offset);
>> +
>> +    hw = &hw_dsc->hw;
>> +
>> +    rc = dsc->rc_range_params;
>> +
>> +    off = 0;
>> +    for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) {
>> +        data |= dsc->rc_buf_thresh[i] << (8 * j);
>> +        j++;
>> +        if ((j == 4) || (i == DSC_NUM_BUF_RANGES - 2)) {
>> +            DPU_REG_WRITE(hw, offset + DSC_RC_BUF_THRESH_0 + off,
>> +                    data);
>> +            off += 4;
>> +            j = 0;
>> +            data = 0;
>> +        }
>> +    }
>> +
>> +    off = 0;
>> +    for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
>> +        min_qp |= rc[i].range_min_qp << (5 * j);
>> +        max_qp |= rc[i].range_max_qp << (5 * j);
>> +        bpg_off |= rc[i].range_bpg_offset << (6 * j);
>> +        j++;
>> +        if (j == 5) {
>> +            DPU_REG_WRITE(hw, offset + DSC_RC_MIN_QP_0 + off,
>> +                    min_qp);
>> +            DPU_REG_WRITE(hw, offset + DSC_RC_MAX_QP_0 + off,
>> +                    max_qp);
>> +            DPU_REG_WRITE(hw,
>> +                    offset + DSC_RC_RANGE_BPG_OFFSETS_0 + off,
>> +                    bpg_off);
>> +            off += 4;
>> +            j = 0;
>> +            min_qp = 0;
>> +            max_qp = 0;
>> +            bpg_off = 0;
>> +        }
>> +    }
>
> Unrolling these loops would make it easier to understand them.

it is kind of difficult to unrolling the for loop.

I will add comments to explain the loop.


>
>> +}
>> +
>> +static void dpu_hw_dsc_bind_pingpong_blk_1_2(
>> +        struct dpu_hw_dsc *hw_dsc,
>> +        bool enable,
>> +        const enum dpu_pingpong pp)
>> +{
>> +    struct dpu_hw_blk_reg_map *hw;
>> +    int offset;
>> +    int mux_cfg = 0xf; /* Disabled */
>> +
>> +    _dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &offset);
>> +
>> +    hw = &hw_dsc->hw;
>> +    if (enable)
>> +        mux_cfg = (pp - PINGPONG_0) & 0x7;
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_CTL, mux_cfg);
>> +}
>> +
>> +static const struct dpu_dsc_cfg *_dsc_offset(enum dpu_dsc dsc,
>> +                       const struct dpu_mdss_cfg *m,
>> +                       void __iomem *addr,
>> +                       struct dpu_hw_blk_reg_map *b)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < m->dsc_count; i++) {
>> +        if (dsc == m->dsc[i].id) {
>> +            b->blk_addr = addr + m->dsc[i].base;
>> +            b->log_mask = DPU_DBG_MASK_DSC;
>> +            return &m->dsc[i];
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +static void _setup_dcs_ops_1_2(struct dpu_hw_dsc_ops *ops,
>> +        const unsigned long features)
>> +{
>> +    ops->dsc_disable = dpu_hw_dsc_disable_1_2;
>> +    ops->dsc_config = dpu_hw_dsc_config_1_2;
>> +    ops->dsc_config_thresh = dpu_hw_dsc_config_thresh_1_2;
>> +    ops->dsc_bind_pingpong_blk = dpu_hw_dsc_bind_pingpong_blk_1_2;
>> +}
>> +
>> +struct dpu_hw_dsc *dpu_hw_dsc_init_1_2(enum dpu_dsc idx, void 
>> __iomem *addr,
>> +                   const struct dpu_mdss_cfg *m)
>> +{
>
> Please rebase on top of https://patchwork.freedesktop.org/series/116615/
>
>> +    struct dpu_hw_dsc *c;
>> +    const struct dpu_dsc_cfg *cfg;
>> +
>> +    c = kzalloc(sizeof(*c), GFP_KERNEL);
>> +    if (!c)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    cfg = _dsc_offset(idx, m, addr, &c->hw);
>> +    if (IS_ERR_OR_NULL(cfg)) {
>> +        kfree(c);
>> +        return ERR_PTR(-EINVAL);
>> +    }
>> +
>> +    c->idx = idx;
>> +    c->caps = cfg;
>> +
>> +    _setup_dcs_ops_1_2(&c->ops, c->caps->features);
>> +
>> +    return c;
>> +}
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> index f4dda88..f7014f5 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> @@ -1,6 +1,7 @@
>>   // SPDX-License-Identifier: GPL-2.0-only
>>   /*
>>    * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>>    */
>>     #define pr_fmt(fmt)    "[drm:%s] " fmt, __func__
>> @@ -250,7 +251,11 @@ int dpu_rm_init(struct dpu_rm *rm,
>>           struct dpu_hw_dsc *hw;
>>           const struct dpu_dsc_cfg *dsc = &cat->dsc[i];
>>   -        hw = dpu_hw_dsc_init(dsc->id, mmio, cat);
>> +        if (test_bit(DPU_DSC_HW_REV_1_2, &dsc->features))
>> +            hw = dpu_hw_dsc_init_1_2(dsc->id, mmio, cat);
>> +        else
>> +            hw = dpu_hw_dsc_init(dsc->id, mmio, cat);
>> +
>>           if (IS_ERR_OR_NULL(hw)) {
>>               rc = PTR_ERR(hw);
>>               DPU_ERROR("failed dsc object creation: err %d\n", rc);
>