MIME-Version: 1.0
References: <1677267647-28672-1-git-send-email-quic_khsieh@quicinc.com>
 <1677267647-28672-2-git-send-email-quic_khsieh@quicinc.com>
 <42b3c193-8897-cfe9-1cae-2f9a66f7983a@linaro.org> <1b5afec9-454d-e1b9-0274-f0476edb4d21@quicinc.com>
In-Reply-To: <1b5afec9-454d-e1b9-0274-f0476edb4d21@quicinc.com>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date:   Sat, 25 Feb 2023 01:57:01 +0200
Message-ID: <CAA8EJprowFyBMdW5QC2zG0gYUtxJ-hHXqhPfYTct2GzYJKLSHg@mail.gmail.com>
Subject: Re: [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions
To:     Kuogee Hsieh <quic_khsieh@quicinc.com>
Cc:     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, 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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, 25 Feb 2023 at 01:51, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 2/24/2023 1:13 PM, Dmitry Baryshkov wrote:
> > On 24/02/2023 21:40, Kuogee Hsieh wrote:
> >> Add DSC helper functions based on DSC configuration profiles to produce
> >> DSC related runtime parameters through both table look up and runtime
> >> calculation to support DSC on DPU.
> >>
> >> There are 6 different DSC configuration profiles are supported
> >> currently.
> >> DSC configuration profiles are differiented by 5 keys, DSC version
> >> (V1.1),
> >> chroma (444/422/420), colorspace (RGB/YUV), bpc(8/10),
> >> bpp (6/7/7.5/8/9/10/12/15) and SCR (0/1).
> >>
> >> Only DSC version V1.1 added and V1.2 will be added later.
> >
> > These helpers should go to drivers/gpu/drm/display/drm_dsc_helper.c
> > Also please check that they can be used for i915 or for amdgpu
> > (ideally for both of them).
> >
> > I didn't check the tables against the standard (or against the current
> > source code), will do that later.
> >
> >>
> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/Makefile                   |   1 +
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.c | 209
> >> +++++++++++++++++++++++++
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.h |  34 ++++
> >>   3 files changed, 244 insertions(+)
> >>   create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.c
> >>   create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.h
> >>
> >> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> >> index 7274c412..28cf52b 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_dsc_helper.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_dsc_helper.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.c
> >> new file mode 100644
> >> index 00000000..88207e9
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.c
> >> @@ -0,0 +1,209 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) 2023. Qualcomm Innovation Center, Inc. All rights
> >> reserved
> >> + */
> >> +
> >> +#include <drm/display/drm_dsc_helper.h>
> >> +#include "msm_drv.h"
> >> +#include "dpu_kms.h"
> >> +#include "dpu_hw_dsc.h"
> >> +#include "dpu_dsc_helper.h"
> >> +
> >> +
> >
> > Extra empty line
> >
> >> +#define DPU_DSC_PPS_SIZE       128
> >> +
> >> +enum dpu_dsc_ratio_type {
> >> +    DSC_V11_8BPC_8BPP,
> >> +    DSC_V11_10BPC_8BPP,
> >> +    DSC_V11_10BPC_10BPP,
> >> +    DSC_V11_SCR1_8BPC_8BPP,
> >> +    DSC_V11_SCR1_10BPC_8BPP,
> >> +    DSC_V11_SCR1_10BPC_10BPP,
> >> +    DSC_RATIO_TYPE_MAX
> >> +};
> >> +
> >> +
> >> +static u16 dpu_dsc_rc_buf_thresh[DSC_NUM_BUF_RANGES - 1] = {
> >> +        0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54,
> >> +        0x62, 0x69, 0x70, 0x77, 0x79, 0x7b, 0x7d, 0x7e
> >
> > Weird indentation
> >
> >> +};
> >> +
> >> +/*
> >> + * Rate control - Min QP values for each ratio type in
> >> dpu_dsc_ratio_type
> >> + */
> >> +static char
> >> dpu_dsc_rc_range_min_qp[DSC_RATIO_TYPE_MAX][DSC_NUM_BUF_RANGES] = {
> >> +    /* DSC v1.1 */
> >> +    {0, 0, 1, 1, 3, 3, 3, 3, 3, 3, 5, 5, 5, 7, 13},
> >> +    {0, 4, 5, 5, 7, 7, 7, 7, 7, 7, 9, 9, 9, 11, 17},
> >> +    {0, 4, 5, 6, 7, 7, 7, 7, 7, 7, 9, 9, 9, 11, 15},
> >> +    /* DSC v1.1 SCR and DSC v1.2 RGB 444 */
> >
> > What is SCR? Is there any reason to use older min/max Qp params
> > instead of always using the ones from the VESA-DSC-1.1 standard?
>
> Standards change request, some vendors may use scr to work with their panel.
>
> These table value are provided by system team.

So, what will happen if we use values from 1.2 standard (aka 1.1 SCR
1) with the older panel?

> >> +    {0, 0, 1, 1, 3, 3, 3, 3, 3, 3, 5, 5, 5, 9, 12},
> >> +    {0, 4, 5, 5, 7, 7, 7, 7, 7, 7, 9, 9, 9, 13, 16},
> >> +    {0, 4, 5, 6, 7, 7, 7, 7, 7, 7, 9, 9, 9, 11, 15},


-- 
With best wishes
Dmitry