| 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
| ||
|
Date: Fri, 24 Feb 2023 15:51:46 -0800
From: Kuogee Hsieh <quic_khsieh@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
<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>
CC: <quic_abhinavk@...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: [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions
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@...cinc.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.
>
>> + {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},
>> +};
>> +
>> +/*
>> + * Rate control - Max QP values for each ratio type in
>> dpu_dsc_ratio_type
>> + */
>> +static char
>> dpu_dsc_rc_range_max_qp[DSC_RATIO_TYPE_MAX][DSC_NUM_BUF_RANGES] = {
>> + /* DSC v1.1 */
>> + {4, 4, 5, 6, 7, 7, 7, 8, 9, 10, 11, 12, 13, 13, 15},
>> + {4, 8, 9, 10, 11, 11, 11, 12, 13, 14, 15, 16, 17, 17, 19},
>> + {7, 8, 9, 10, 11, 11, 11, 12, 13, 13, 14, 14, 15, 15, 16},
>> + /* DSC v1.1 SCR and DSC v1.2 RGB 444 */
>> + {4, 4, 5, 6, 7, 7, 7, 8, 9, 10, 10, 11, 11, 12, 13},
>> + {8, 8, 9, 10, 11, 11, 11, 12, 13, 14, 14, 15, 15, 16, 17},
>> + {7, 8, 9, 10, 11, 11, 11, 12, 13, 13, 14, 14, 15, 15, 16},
>> +};
>> +
>> +/*
>> + * Rate control - bpg offset values for each ratio type in
>> dpu_dsc_ratio_type
>> + */
>> +static char
>> dpu_dsc_rc_range_bpg[DSC_RATIO_TYPE_MAX][DSC_NUM_BUF_RANGES] = {
>> + /* DSC v1.1 */
>> + {2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -12, -12, -12, -12},
>> + {2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -12, -12, -12, -12},
>> + {2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -10, -12, -12, -12},
>> + /* DSC v1.1 SCR and DSC V1.2 RGB 444 */
>> + {2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -12, -12, -12, -12},
>> + {2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -12, -12, -12, -12},
>> + {2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -10, -12, -12, -12},
>> +};
>> +
>> +static struct dpu_dsc_rc_init_params_lut {
>> + u32 rc_quant_incr_limit0;
>> + u32 rc_quant_incr_limit1;
>> + u32 initial_fullness_offset;
>> + u32 initial_xmit_delay;
>> + u32 second_line_bpg_offset;
>> + u32 second_line_offset_adj;
>> + u32 flatness_min_qp;
>> + u32 flatness_max_qp;
>> +} dpu_dsc_rc_init_param_lut[] = {
>> + /* DSC v1.1 */
>> + {11, 11, 6144, 512, 0, 0, 3, 12}, /* DSC_V11_8BPC_8BPP */
>> + {15, 15, 6144, 512, 0, 0, 7, 16}, /* DSC_V11_10BPC_8BPP */
>> + {15, 15, 5632, 410, 0, 0, 7, 16}, /* DSC_V11_10BPC_10BPP */
>> + /* DSC v1.1 SCR and DSC v1.2 RGB 444 */
>> + {11, 11, 6144, 512, 0, 0, 3, 12}, /* DSC_V12_444_8BPC_8BPP or
>> DSC_V11_SCR1_8BPC_8BPP */
>> + {15, 15, 6144, 512, 0, 0, 7, 16}, /* DSC_V12_444_10BPC_8BPP or
>> DSC_V11_SCR1_10BPC_8BPP */
>> + {15, 15, 5632, 410, 0, 0, 7, 16}, /* DSC_V12_444_10BPC_10BPP or
>> DSC_V11_SCR1_10BPC_10BPP */
>> +};
>> +
>> +/**
>> + * Maps to lookup the dpu_dsc_ratio_type index used in rate control
>> tables
>> + */
>> +static struct dpu_dsc_table_index_lut {
>> + u32 fmt;
>> + u32 scr_ver;
>> + u32 minor_ver;
>> + u32 bpc;
>> + u32 bpp;
>> + u32 type;
>> +} dpu_dsc_index_map[] = {
>> + /* DSC 1.1 formats - scr version is considered */
>> + {DPU_DSC_CHROMA_444, 0, 1, 8, 8, DSC_V11_8BPC_8BPP},
>> + {DPU_DSC_CHROMA_444, 0, 1, 10, 8, DSC_V11_10BPC_8BPP},
>> + {DPU_DSC_CHROMA_444, 0, 1, 10, 10, DSC_V11_10BPC_10BPP},
>> + {DPU_DSC_CHROMA_444, 1, 1, 8, 8, DSC_V11_SCR1_8BPC_8BPP},
>> + {DPU_DSC_CHROMA_444, 1, 1, 10, 8, DSC_V11_SCR1_10BPC_8BPP},
>> + {DPU_DSC_CHROMA_444, 1, 1, 10, 10, DSC_V11_SCR1_10BPC_10BPP},
>> +};
>> +
>> +static int _get_rc_table_index(struct drm_dsc_config *dsc, int scr_ver)
>> +{
>> + u32 bpp, bpc, i, fmt = DPU_DSC_CHROMA_444;
>> +
>> + if (dsc->dsc_version_major != 0x1) {
>> + DPU_ERROR("unsupported major version %d\n",
>> + dsc->dsc_version_major);
>> + return -EINVAL;
>> + }
>> +
>> + bpc = dsc->bits_per_component;
>> + bpp = DSC_BPP(*dsc);
>
> Just inline the macro.
>
>> +
>> + if (dsc->native_422)
>> + fmt = DPU_DSC_CHROMA_422;
>> + else if (dsc->native_420)
>> + fmt = DPU_DSC_CHROMA_420;
>> +
>> +
>> + for (i = 0; i < ARRAY_SIZE(dpu_dsc_index_map); i++) {
>> + if (dsc->dsc_version_minor == dpu_dsc_index_map[i].minor_ver &&
>> + fmt == dpu_dsc_index_map[i].fmt &&
>> + bpc == dpu_dsc_index_map[i].bpc &&
>> + bpp == dpu_dsc_index_map[i].bpp &&
>> + (dsc->dsc_version_minor != 0x1 ||
>> + scr_ver == dpu_dsc_index_map[i].scr_ver))
>> + return dpu_dsc_index_map[i].type;
>> + }
>> +
>> + DPU_ERROR("unsupported DSC v%d.%dr%d, bpc:%d, bpp:%d, fmt:0x%x\n",
>> + dsc->dsc_version_major, dsc->dsc_version_minor,
>> + scr_ver, bpc, bpp, fmt);
>> + return -EINVAL;
>> +}
>> +
>> +int dpu_dsc_populate_dsc_config(struct drm_dsc_config *dsc, int
>> scr_ver)
>> +{
>> + int bpp, bpc;
>> + struct dpu_dsc_rc_init_params_lut *rc_param_lut;
>> + int i, ratio_idx;
>> +
>> + dsc->rc_model_size = 8192;
>> +
>> + if ((dsc->dsc_version_major == 0x1) &&
>> + (dsc->dsc_version_minor == 0x1)) {
>
> indent to the opening bracket please, so that '(dsc' on both lines
> start on the same position.
>
>> + if (scr_ver == 0x1)
>> + dsc->first_line_bpg_offset = 15;
>> + else
>> + dsc->first_line_bpg_offset = 12;
>> + }
>> +
>> + dsc->rc_edge_factor = 6;
>> + dsc->rc_tgt_offset_high = 3;
>> + dsc->rc_tgt_offset_low = 3;
>> + dsc->simple_422 = 0;
>> + dsc->convert_rgb = !(dsc->native_422 | dsc->native_420);
>> + dsc->vbr_enable = 0;
>> +
>> + bpp = DSC_BPP(*dsc);
>
> inline the macro.
>
>> + bpc = dsc->bits_per_component;
>> +
>> + ratio_idx = _get_rc_table_index(dsc, scr_ver);
>> + if ((ratio_idx < 0) || (ratio_idx >= DSC_RATIO_TYPE_MAX))
>> + return -EINVAL;
>> +
>> +
>> + for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++)
>> + dsc->rc_buf_thresh[i] = dpu_dsc_rc_buf_thresh[i];
>
> Can we use memcpy?
>
>> +
>> + for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
>> + dsc->rc_range_params[i].range_min_qp =
>> + dpu_dsc_rc_range_min_qp[ratio_idx][i];
>> + dsc->rc_range_params[i].range_max_qp =
>> + dpu_dsc_rc_range_max_qp[ratio_idx][i];
>> + dsc->rc_range_params[i].range_bpg_offset =
>> + dpu_dsc_rc_range_bpg[ratio_idx][i];
>> + }
>> +
>> + rc_param_lut = &dpu_dsc_rc_init_param_lut[ratio_idx];
>> + dsc->rc_quant_incr_limit0 = rc_param_lut->rc_quant_incr_limit0;
>> + dsc->rc_quant_incr_limit1 = rc_param_lut->rc_quant_incr_limit1;
>> + dsc->initial_offset = rc_param_lut->initial_fullness_offset;
>> + dsc->initial_xmit_delay = rc_param_lut->initial_xmit_delay;
>> + dsc->second_line_bpg_offset = rc_param_lut->second_line_bpg_offset;
>> + dsc->second_line_offset_adj = rc_param_lut->second_line_offset_adj;
>> + dsc->flatness_min_qp = rc_param_lut->flatness_min_qp;
>> + dsc->flatness_max_qp = rc_param_lut->flatness_max_qp;
>> +
>> +
>> + dsc->line_buf_depth = bpc + 1;
>> + dsc->mux_word_size = bpc > 10 ? DSC_MUX_WORD_SIZE_12_BPC :
>> DSC_MUX_WORD_SIZE_8_10_BPC;
>> +
>> + dsc->initial_scale_value = 8 * dsc->rc_model_size /
>> + (dsc->rc_model_size - dsc->initial_offset);
>> +
>> + return drm_dsc_compute_rc_parameters(dsc);
>
> Indentation is wrong
>
>> +}
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.h
>> new file mode 100644
>> index 00000000..4a23e02
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.h
>> @@ -0,0 +1,34 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2023. Qualcomm Innovation Center, Inc. All rights
>> reserved
>> + */
>> +
>> +#ifndef __DPU_DSC_HELPER_H__
>> +#define __DPU_DSC_HELPER_H__
>> +
>> +#include "msm_drv.h"
>> +
>> +#define DSC_1_1_PPS_PARAMETER_SET_ELEMENTS 88
>
> What is this? Does it need to be global?
>
>> +
>> +/**
>> + * Bits/pixel target >> 4 (removing the fractional bits)
>> + * returns the integer bpp value from the drm_dsc_config struct
>> + */
>> +#define DSC_BPP(config) ((config).bits_per_pixel >> 4)
>> +
>> +enum dpu_dsc_chroma {
>> + DPU_DSC_CHROMA_444,
>> + DPU_DSC_CHROMA_422,
>> + DPU_DSC_CHROMA_420,
>> +};
>
> I think this enum is also not used outside of your helpers.
>
>> +
>> +int dpu_dsc_populate_dsc_config(struct drm_dsc_config *dsc, int
>> scr_ver);
>> +
>> +bool dpu_dsc_ich_reset_override_needed(bool pu_en, struct
>> drm_dsc_config *dsc);
>
> Unused
>
>> +
>> +int dpu_dsc_initial_line_calc( u32 num_active_ss_per_enc,
>> + struct drm_dsc_config *dsc,
>> + int enc_ip_width, int dsc_cmn_mode);
>
> Unused
>
>> +
>> +#endif /* __DPU_DSC_HELPER_H__ */
>> +
>
Powered by blists - more mailing lists