diff mbox series

[RFC,1/2] drm/msm/dpu: add dsc helper functions

Message ID 1677267647-28672-2-git-send-email-quic_khsieh@quicinc.com (mailing list archive)
State Not Applicable
Headers show
Series Add DPU DSC helper module | expand

Commit Message

Kuogee Hsieh Feb. 24, 2023, 7:40 p.m. UTC
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.

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

Comments

Dmitry Baryshkov Feb. 24, 2023, 9:13 p.m. UTC | #1
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?

> +	{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__ */
> +
Abhinav Kumar Feb. 24, 2023, 9:23 p.m. UTC | #2
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).
> 

No, it cannot. So each DSC encoder parameter is calculated based on the 
HW core which is being used.

They all get packed to the same DSC structure which is the struct 
drm_dsc_config but the way the parameters are computed is specific to 
the HW.

This DPU file helper still uses the drm_dsc_helper's 
drm_dsc_compute_rc_parameters() like all other vendors do but the 
parameters themselves are very HW specific and belong to each vendor's dir.

This is not unique to MSM.

Lets take a few other examples:

AMD: 
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165

i915: 
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379 


All vendors compute the values differently and eventually call 
drm_dsc_compute_rc_parameters()

> 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?
> 
>> +    {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__ */
>> +
>
Dmitry Baryshkov Feb. 24, 2023, 9:36 p.m. UTC | #3
24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar <quic_abhinavk@quicinc.com> пишет:
>
>
>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).
>> 
>
>No, it cannot. So each DSC encoder parameter is calculated based on the HW core which is being used.
>
>They all get packed to the same DSC structure which is the struct drm_dsc_config but the way the parameters are computed is specific to the HW.
>
>This DPU file helper still uses the drm_dsc_helper's drm_dsc_compute_rc_parameters() like all other vendors do but the parameters themselves are very HW specific and belong to each vendor's dir.
>
>This is not unique to MSM.
>
>Lets take a few other examples:
>
>AMD: https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
>
>i915: https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379 

I checked several values here. Intel driver defines more bpc/bpp combinations, but the ones which are defined in intel_vdsc and in this patch seem to match. If there are major differences there, please point me to the exact case.

I remember that AMD driver might have different values.


>
>All vendors compute the values differently and eventually call drm_dsc_compute_rc_parameters()
>
>> 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?
>> 
>>> +    {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__ */
>>> +
>>
Abhinav Kumar Feb. 24, 2023, 10:26 p.m. UTC | #4
On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
> 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar <quic_abhinavk@quicinc.com> пишет:
>>
>>
>> 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).
>>>
>>
>> No, it cannot. So each DSC encoder parameter is calculated based on the HW core which is being used.
>>
>> They all get packed to the same DSC structure which is the struct drm_dsc_config but the way the parameters are computed is specific to the HW.
>>
>> This DPU file helper still uses the drm_dsc_helper's drm_dsc_compute_rc_parameters() like all other vendors do but the parameters themselves are very HW specific and belong to each vendor's dir.
>>
>> This is not unique to MSM.
>>
>> Lets take a few other examples:
>>
>> AMD: https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
>>
>> i915: https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
> 
> I checked several values here. Intel driver defines more bpc/bpp combinations, but the ones which are defined in intel_vdsc and in this patch seem to match. If there are major differences there, please point me to the exact case.
> 
> I remember that AMD driver might have different values.
> 

Some values in the rc_params table do match. But the rc_buf_thresh[] doesnt.

https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40

Vs

+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
+};

I dont know the AMD calculation very well to say that moving this to the 
helper is going to help.

Also, i think its too risky to change other drivers to use whatever math 
we put in the drm_dsc_helper to compute thr RC params because their code 
might be computing and using this tables differently.

Its too much ownership for MSM developers to move this to drm_dsc_helper 
and own that as it might cause breakage of basic DSC even if some values 
are repeated.

I would prefer to keep it in the msm code but in a top level directory 
so that we dont have to make DSI dependent on DPU.


> 
>>
>> All vendors compute the values differently and eventually call drm_dsc_compute_rc_parameters()
>>
>>> 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?
>>>
>>>> +    {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__ */
>>>> +
>>>
>
Kuogee Hsieh Feb. 24, 2023, 11:51 p.m. UTC | #5
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.

>
>> +    {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__ */
>> +
>
Dmitry Baryshkov Feb. 24, 2023, 11:53 p.m. UTC | #6
On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
> > 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar <quic_abhinavk@quicinc.com> пишет:
> >> 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).
> >>>
> >>
> >> No, it cannot. So each DSC encoder parameter is calculated based on the HW core which is being used.
> >>
> >> They all get packed to the same DSC structure which is the struct drm_dsc_config but the way the parameters are computed is specific to the HW.
> >>
> >> This DPU file helper still uses the drm_dsc_helper's drm_dsc_compute_rc_parameters() like all other vendors do but the parameters themselves are very HW specific and belong to each vendor's dir.
> >>
> >> This is not unique to MSM.
> >>
> >> Lets take a few other examples:
> >>
> >> AMD: https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
> >>
> >> i915: https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
> >
> > I checked several values here. Intel driver defines more bpc/bpp combinations, but the ones which are defined in intel_vdsc and in this patch seem to match. If there are major differences there, please point me to the exact case.
> >
> > I remember that AMD driver might have different values.
> >
>
> Some values in the rc_params table do match. But the rc_buf_thresh[] doesnt.

Because later they do:

vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;

>
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
>
> Vs
>
> +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
> +};

I'd prefer to have 896, 1792, etc. here, as those values come from the
standard. As it's done in the Intel driver.

> I dont know the AMD calculation very well to say that moving this to the
> helper is going to help.

Those calculations correspond (more or less) at the first glance to
what intel does for their newer generations. I think that's not our
problem for now.

>
> Also, i think its too risky to change other drivers to use whatever math
> we put in the drm_dsc_helper to compute thr RC params because their code
> might be computing and using this tables differently.
>
> Its too much ownership for MSM developers to move this to drm_dsc_helper
> and own that as it might cause breakage of basic DSC even if some values
> are repeated.

It's time to stop thinking about ownership and start thinking about
shared code. We already have two instances of DSC tables. I don't
think having a third instance, which is a subset of an existing
dataset, would be beneficial to anybody.
AMD has complicated code which supports half-bit bpp and calculates
some of the parameters. But sharing data with the i915 driver is
straightforward.

> I would prefer to keep it in the msm code but in a top level directory
> so that we dont have to make DSI dependent on DPU.

I haven't changed my opinion. Please move relevant i915's code to
helpers, verify data against standards and reuse it.

> >> All vendors compute the values differently and eventually call drm_dsc_compute_rc_parameters()
> >>
> >>> I didn't check the tables against the standard (or against the current source code), will do that later.
Dmitry Baryshkov Feb. 24, 2023, 11:57 p.m. UTC | #7
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},
Abhinav Kumar Feb. 25, 2023, 12:36 a.m. UTC | #8
On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
> On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
>>> 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar <quic_abhinavk@quicinc.com> пишет:
>>>> 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).
>>>>>
>>>>
>>>> No, it cannot. So each DSC encoder parameter is calculated based on the HW core which is being used.
>>>>
>>>> They all get packed to the same DSC structure which is the struct drm_dsc_config but the way the parameters are computed is specific to the HW.
>>>>
>>>> This DPU file helper still uses the drm_dsc_helper's drm_dsc_compute_rc_parameters() like all other vendors do but the parameters themselves are very HW specific and belong to each vendor's dir.
>>>>
>>>> This is not unique to MSM.
>>>>
>>>> Lets take a few other examples:
>>>>
>>>> AMD: https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
>>>>
>>>> i915: https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
>>>
>>> I checked several values here. Intel driver defines more bpc/bpp combinations, but the ones which are defined in intel_vdsc and in this patch seem to match. If there are major differences there, please point me to the exact case.
>>>
>>> I remember that AMD driver might have different values.
>>>
>>
>> Some values in the rc_params table do match. But the rc_buf_thresh[] doesnt.
> 
> Because later they do:
> 
> vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
> 
>>
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
>>
>> Vs
>>
>> +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
>> +};
> 
> I'd prefer to have 896, 1792, etc. here, as those values come from the
> standard. As it's done in the Intel driver.
> 

Got it, thanks

>> I dont know the AMD calculation very well to say that moving this to the
>> helper is going to help.
> 
> Those calculations correspond (more or less) at the first glance to
> what intel does for their newer generations. I think that's not our
> problem for now.
> 

Well, we have to figure out if each value matches and if each of them 
come from the spec for us and i915 and from which section. So it is 
unfortunately our problem.

>>
>> Also, i think its too risky to change other drivers to use whatever math
>> we put in the drm_dsc_helper to compute thr RC params because their code
>> might be computing and using this tables differently.
>>
>> Its too much ownership for MSM developers to move this to drm_dsc_helper
>> and own that as it might cause breakage of basic DSC even if some values
>> are repeated.
> 
> It's time to stop thinking about ownership and start thinking about
> shared code. We already have two instances of DSC tables. I don't
> think having a third instance, which is a subset of an existing
> dataset, would be beneficial to anybody.
> AMD has complicated code which supports half-bit bpp and calculates
> some of the parameters. But sharing data with the i915 driver is
> straightforward.
> 

Sorry, but I would like to get an ack from i915 folks if this is going
to be useful to them if we move this to helper because we have to look 
at every table. Not just one.

Also, this is just 1.1, we will add more tables for 1.2. So we will have 
to end up changing both 1.1 and 1.2 tables as they are different for QC.

So if you look at the DSC spec from where these tables have come it says

"Common Recommended Rate Control-Related Parameter Values"

Its Recommended but its NOT mandated by the spec to follow every value 
to the dot. I have confirmed this point with more folks.

So, if someone from i915 this is useful and safe to move their code to 
the tables, we can try it.

>> I would prefer to keep it in the msm code but in a top level directory
>> so that we dont have to make DSI dependent on DPU.
> 
> I haven't changed my opinion. Please move relevant i915's code to
> helpers, verify data against standards and reuse it.
> 



>>>> All vendors compute the values differently and eventually call drm_dsc_compute_rc_parameters()
>>>>
>>>>> I didn't check the tables against the standard (or against the current source code), will do that later.
>
Dmitry Baryshkov Feb. 25, 2023, 3:23 p.m. UTC | #9
On 25/02/2023 02:36, Abhinav Kumar wrote:
> 
> 
> On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
>> On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar 
>> <quic_abhinavk@quicinc.com> wrote:
>>> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
>>>> 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar 
>>>> <quic_abhinavk@quicinc.com> пишет:
>>>>> 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).
>>>>>>
>>>>>
>>>>> No, it cannot. So each DSC encoder parameter is calculated based on 
>>>>> the HW core which is being used.
>>>>>
>>>>> They all get packed to the same DSC structure which is the struct 
>>>>> drm_dsc_config but the way the parameters are computed is specific 
>>>>> to the HW.
>>>>>
>>>>> This DPU file helper still uses the drm_dsc_helper's 
>>>>> drm_dsc_compute_rc_parameters() like all other vendors do but the 
>>>>> parameters themselves are very HW specific and belong to each 
>>>>> vendor's dir.
>>>>>
>>>>> This is not unique to MSM.
>>>>>
>>>>> Lets take a few other examples:
>>>>>
>>>>> AMD: 
>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
>>>>>
>>>>> i915: 
>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
>>>>
>>>> I checked several values here. Intel driver defines more bpc/bpp 
>>>> combinations, but the ones which are defined in intel_vdsc and in 
>>>> this patch seem to match. If there are major differences there, 
>>>> please point me to the exact case.
>>>>
>>>> I remember that AMD driver might have different values.
>>>>
>>>
>>> Some values in the rc_params table do match. But the rc_buf_thresh[] 
>>> doesnt.
>>
>> Because later they do:
>>
>> vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
>>
>>>
>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
>>>
>>> Vs
>>>
>>> +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
>>> +};
>>
>> I'd prefer to have 896, 1792, etc. here, as those values come from the
>> standard. As it's done in the Intel driver.
>>
> 
> Got it, thanks
> 
>>> I dont know the AMD calculation very well to say that moving this to the
>>> helper is going to help.
>>
>> Those calculations correspond (more or less) at the first glance to
>> what intel does for their newer generations. I think that's not our
>> problem for now.
>>
> 
> Well, we have to figure out if each value matches and if each of them 
> come from the spec for us and i915 and from which section. So it is 
> unfortunately our problem.

Otherwise it will have to be handled by Marijn, me or anybody else 
wanting to hack up the DSC code. Or by anybody adding DSC support to the 
next platform and having to figure out the difference between i915, msm 
and their platform.

> 
>>>
>>> Also, i think its too risky to change other drivers to use whatever math
>>> we put in the drm_dsc_helper to compute thr RC params because their code
>>> might be computing and using this tables differently.
>>>
>>> Its too much ownership for MSM developers to move this to drm_dsc_helper
>>> and own that as it might cause breakage of basic DSC even if some values
>>> are repeated.
>>
>> It's time to stop thinking about ownership and start thinking about
>> shared code. We already have two instances of DSC tables. I don't
>> think having a third instance, which is a subset of an existing
>> dataset, would be beneficial to anybody.
>> AMD has complicated code which supports half-bit bpp and calculates
>> some of the parameters. But sharing data with the i915 driver is
>> straightforward.
>>
> 
> Sorry, but I would like to get an ack from i915 folks if this is going
> to be useful to them if we move this to helper because we have to look 
> at every table. Not just one.

Added i915 maintainers to the CC list for them to be able to answer.

> 
> Also, this is just 1.1, we will add more tables for 1.2. So we will have 
> to end up changing both 1.1 and 1.2 tables as they are different for QC.

I haven't heard back from Kuogee about the possible causes of using 
rc/qp values from 1.2 even for 1.1 panels. Maybe you can comment on 
that? In other words, can we always stick to the values from 1.2 
standard? What will be the drawback?

Otherwise, we'd have to have two different sets of values, like you do 
in your vendor driver.

> So if you look at the DSC spec from where these tables have come it says
> 
> "Common Recommended Rate Control-Related Parameter Values"
> 
> Its Recommended but its NOT mandated by the spec to follow every value 
> to the dot. I have confirmed this point with more folks.
> 
> So, if someone from i915 this is useful and safe to move their code to 
> the tables, we can try it.
> 
>>> I would prefer to keep it in the msm code but in a top level directory
>>> so that we dont have to make DSI dependent on DPU.
>>
>> I haven't changed my opinion. Please move relevant i915's code to
>> helpers, verify data against standards and reuse it.
>>
> 
> 
> 
>>>>> All vendors compute the values differently and eventually call 
>>>>> drm_dsc_compute_rc_parameters()
>>>>>
>>>>>> I didn't check the tables against the standard (or against the 
>>>>>> current source code), will do that later.
>>
Abhinav Kumar Feb. 26, 2023, 12:16 a.m. UTC | #10
Hi Dmitry

On 2/24/2023 3:57 PM, Dmitry Baryshkov wrote:
> 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?
> 

Standards change request means fixing errors/errata for the given 
standard. Those are typically released as a different spec.

So I referred the DSC 1.1 SCR spec, and it does have a few differences 
in the table compared to DSC 1.1 which will get into DSC 1.2.

Hence the table entries are same between DSC 1.1 SCR and DSC 1.2

You are right, ideally DSC 1.2 should be backwards compatible with DSC 
1.1 in terms of the values (thats what the spec says too) but I am not 
sure if we can expect every panel/DP monitor to be forward compatible 
without any SW change because it might need some firmware update (for 
the panel) or SW update to support that especially during transitions of 
the spec revisions (SCR to be precise).

Typically we do below for DP monitors exactly for the same reason:

DSC_ver_to_use = min(what_we_support, what_DP_monitor_supports) and use 
that table.

For DSI panels, typically in the panel spec it should say whether the 
SCR version needs to be used because we have seen that for some panels ( 
I dont remember exactly which one ) based on which panel and which 
revision of the panel, it might not.

Thats why downstream started adding qcom,mdss-dsc-scr-version to the 
devicetree.

>>>> +    {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},
> 
>
Abhinav Kumar Feb. 26, 2023, 12:47 a.m. UTC | #11
Hi Dmitry

On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
> On 25/02/2023 02:36, Abhinav Kumar wrote:
>>
>>
>> On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
>>> On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar 
>>> <quic_abhinavk@quicinc.com> wrote:
>>>> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
>>>>> 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar 
>>>>> <quic_abhinavk@quicinc.com> пишет:
>>>>>> 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).
>>>>>>>
>>>>>>
>>>>>> No, it cannot. So each DSC encoder parameter is calculated based 
>>>>>> on the HW core which is being used.
>>>>>>
>>>>>> They all get packed to the same DSC structure which is the struct 
>>>>>> drm_dsc_config but the way the parameters are computed is specific 
>>>>>> to the HW.
>>>>>>
>>>>>> This DPU file helper still uses the drm_dsc_helper's 
>>>>>> drm_dsc_compute_rc_parameters() like all other vendors do but the 
>>>>>> parameters themselves are very HW specific and belong to each 
>>>>>> vendor's dir.
>>>>>>
>>>>>> This is not unique to MSM.
>>>>>>
>>>>>> Lets take a few other examples:
>>>>>>
>>>>>> AMD: 
>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165 
>>>>>>
>>>>>>
>>>>>> i915: 
>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379 
>>>>>>
>>>>>
>>>>> I checked several values here. Intel driver defines more bpc/bpp 
>>>>> combinations, but the ones which are defined in intel_vdsc and in 
>>>>> this patch seem to match. If there are major differences there, 
>>>>> please point me to the exact case.
>>>>>
>>>>> I remember that AMD driver might have different values.
>>>>>
>>>>
>>>> Some values in the rc_params table do match. But the rc_buf_thresh[] 
>>>> doesnt.
>>>
>>> Because later they do:
>>>
>>> vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
>>>
>>>>
>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40 
>>>>
>>>>
>>>> Vs
>>>>
>>>> +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
>>>> +};
>>>
>>> I'd prefer to have 896, 1792, etc. here, as those values come from the
>>> standard. As it's done in the Intel driver.
>>>
>>
>> Got it, thanks
>>
>>>> I dont know the AMD calculation very well to say that moving this to 
>>>> the
>>>> helper is going to help.
>>>
>>> Those calculations correspond (more or less) at the first glance to
>>> what intel does for their newer generations. I think that's not our
>>> problem for now.
>>>
>>
>> Well, we have to figure out if each value matches and if each of them 
>> come from the spec for us and i915 and from which section. So it is 
>> unfortunately our problem.
> 
> Otherwise it will have to be handled by Marijn, me or anybody else 
> wanting to hack up the DSC code. Or by anybody adding DSC support to the 
> next platform and having to figure out the difference between i915, msm 
> and their platform.
> 

Yes, I wonder why the same doubt didn't arise when the other vendors 
added their support both from other maintainers and others.

Which makes me think that like I wrote in my previous response, these 
are "recommended" values in the spec but its not mandatory.

Moving this to the drm_dsc_helper is generalizing the tables and not 
giving room for the vendors to customize even if they want to (which the 
spec does allow).

So if this has any merit and if you or Marijn would like to take it up, 
go for it. We would do the same thing as either of you would have to in 
terms of figuring out the difference between msm and the i915 code.

This is not a generic API we are trying to put in a helper, these are 
hard-coded tables so there is a difference between looking at these Vs 
looking at some common code which can move to the core.

>>
>>>>
>>>> Also, i think its too risky to change other drivers to use whatever 
>>>> math
>>>> we put in the drm_dsc_helper to compute thr RC params because their 
>>>> code
>>>> might be computing and using this tables differently.
>>>>
>>>> Its too much ownership for MSM developers to move this to 
>>>> drm_dsc_helper
>>>> and own that as it might cause breakage of basic DSC even if some 
>>>> values
>>>> are repeated.
>>>
>>> It's time to stop thinking about ownership and start thinking about
>>> shared code. We already have two instances of DSC tables. I don't
>>> think having a third instance, which is a subset of an existing
>>> dataset, would be beneficial to anybody.
>>> AMD has complicated code which supports half-bit bpp and calculates
>>> some of the parameters. But sharing data with the i915 driver is
>>> straightforward.
>>>
>>
>> Sorry, but I would like to get an ack from i915 folks if this is going
>> to be useful to them if we move this to helper because we have to look 
>> at every table. Not just one.
> 
> Added i915 maintainers to the CC list for them to be able to answer.
> 

Thanks, lets wait to hear from them about where finally these tables 
should go but thats can be taken up as a separate effort too.

>>
>> Also, this is just 1.1, we will add more tables for 1.2. So we will 
>> have to end up changing both 1.1 and 1.2 tables as they are different 
>> for QC.
> 
> I haven't heard back from Kuogee about the possible causes of using 
> rc/qp values from 1.2 even for 1.1 panels. Maybe you can comment on 
> that? In other words, can we always stick to the values from 1.2 
> standard? What will be the drawback?
> 
> Otherwise, we'd have to have two different sets of values, like you do 
> in your vendor driver.
> 

I have responded to this in the other email.

All this being said, even if the rc tables move the drm_dsc_helper 
either now or later on, we will still need MSM specific calculations for 
many of the other encoder parameters (which are again either hard-coded 
or calculated). Please refer to the sde_dsc_populate_dsc_config() 
downstream. And yes, you will not find those in the DP spec directly.

So we will still need a dsc helper for MSM calculations to be common for 
DSI / DP irrespective of where the tables go.

So, lets finalize that first.

>> So if you look at the DSC spec from where these tables have come it says
>>
>> "Common Recommended Rate Control-Related Parameter Values"
>>
>> Its Recommended but its NOT mandated by the spec to follow every value 
>> to the dot. I have confirmed this point with more folks.
>>
>> So, if someone from i915 this is useful and safe to move their code to 
>> the tables, we can try it.
>>
>>>> I would prefer to keep it in the msm code but in a top level directory
>>>> so that we dont have to make DSI dependent on DPU.
>>>
>>> I haven't changed my opinion. Please move relevant i915's code to
>>> helpers, verify data against standards and reuse it.
>>>
>>
>>
>>
>>>>>> All vendors compute the values differently and eventually call 
>>>>>> drm_dsc_compute_rc_parameters()
>>>>>>
>>>>>>> I didn't check the tables against the standard (or against the 
>>>>>>> current source code), will do that later.
>>>
>
Dmitry Baryshkov Feb. 26, 2023, 1:09 p.m. UTC | #12
On 26/02/2023 02:47, Abhinav Kumar wrote:
> Hi Dmitry
> 
> On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
>> On 25/02/2023 02:36, Abhinav Kumar wrote:
>>>
>>>
>>> On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
>>>> On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar 
>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
>>>>>> 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar 
>>>>>> <quic_abhinavk@quicinc.com> пишет:
>>>>>>> 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).
>>>>>>>>
>>>>>>>
>>>>>>> No, it cannot. So each DSC encoder parameter is calculated based 
>>>>>>> on the HW core which is being used.
>>>>>>>
>>>>>>> They all get packed to the same DSC structure which is the struct 
>>>>>>> drm_dsc_config but the way the parameters are computed is 
>>>>>>> specific to the HW.
>>>>>>>
>>>>>>> This DPU file helper still uses the drm_dsc_helper's 
>>>>>>> drm_dsc_compute_rc_parameters() like all other vendors do but the 
>>>>>>> parameters themselves are very HW specific and belong to each 
>>>>>>> vendor's dir.
>>>>>>>
>>>>>>> This is not unique to MSM.
>>>>>>>
>>>>>>> Lets take a few other examples:
>>>>>>>
>>>>>>> AMD: 
>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
>>>>>>>
>>>>>>> i915: 
>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
>>>>>>
>>>>>> I checked several values here. Intel driver defines more bpc/bpp 
>>>>>> combinations, but the ones which are defined in intel_vdsc and in 
>>>>>> this patch seem to match. If there are major differences there, 
>>>>>> please point me to the exact case.
>>>>>>
>>>>>> I remember that AMD driver might have different values.
>>>>>>
>>>>>
>>>>> Some values in the rc_params table do match. But the 
>>>>> rc_buf_thresh[] doesnt.
>>>>
>>>> Because later they do:
>>>>
>>>> vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
>>>>
>>>>>
>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
>>>>>
>>>>> Vs
>>>>>
>>>>> +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
>>>>> +};
>>>>
>>>> I'd prefer to have 896, 1792, etc. here, as those values come from the
>>>> standard. As it's done in the Intel driver.
>>>>
>>>
>>> Got it, thanks
>>>
>>>>> I dont know the AMD calculation very well to say that moving this 
>>>>> to the
>>>>> helper is going to help.
>>>>
>>>> Those calculations correspond (more or less) at the first glance to
>>>> what intel does for their newer generations. I think that's not our
>>>> problem for now.
>>>>
>>>
>>> Well, we have to figure out if each value matches and if each of them 
>>> come from the spec for us and i915 and from which section. So it is 
>>> unfortunately our problem.
>>
>> Otherwise it will have to be handled by Marijn, me or anybody else 
>> wanting to hack up the DSC code. Or by anybody adding DSC support to 
>> the next platform and having to figure out the difference between 
>> i915, msm and their platform.
>>
> 
> Yes, I wonder why the same doubt didn't arise when the other vendors 
> added their support both from other maintainers and others.
> 
> Which makes me think that like I wrote in my previous response, these 
> are "recommended" values in the spec but its not mandatory.

I think, it is because there were no other drivers to compare. In other 
words, for a first driver it is pretty logical to have everything 
handled on its own. As soon as we start getting other implementations of 
a feature, it becomes logical to think if the code can be generalized. 
This is what we see we with the HDCP series or with the code being moved 
to DP helpers.

> 
> Moving this to the drm_dsc_helper is generalizing the tables and not 
> giving room for the vendors to customize even if they want to (which the 
> spec does allow).

That depends on the API you select. For example, in 
intel_dsc_compute_params() I see customization being applied to 
rc_buf_thresh in 6bpp case. I'd leave that to the i915 driver.

In case the driver needs to perform customization of the params, nothing 
stops it drop applying after filling all the RC params in the 
drm_dsc_config struct via the generic helper.


> So if this has any merit and if you or Marijn would like to take it up, 
> go for it. We would do the same thing as either of you would have to in 
> terms of figuring out the difference between msm and the i915 code.
> 
> This is not a generic API we are trying to put in a helper, these are 
> hard-coded tables so there is a difference between looking at these Vs 
> looking at some common code which can move to the core.
> 
>>>
>>>>>
>>>>> Also, i think its too risky to change other drivers to use whatever 
>>>>> math
>>>>> we put in the drm_dsc_helper to compute thr RC params because their 
>>>>> code
>>>>> might be computing and using this tables differently.
>>>>>
>>>>> Its too much ownership for MSM developers to move this to 
>>>>> drm_dsc_helper
>>>>> and own that as it might cause breakage of basic DSC even if some 
>>>>> values
>>>>> are repeated.
>>>>
>>>> It's time to stop thinking about ownership and start thinking about
>>>> shared code. We already have two instances of DSC tables. I don't
>>>> think having a third instance, which is a subset of an existing
>>>> dataset, would be beneficial to anybody.
>>>> AMD has complicated code which supports half-bit bpp and calculates
>>>> some of the parameters. But sharing data with the i915 driver is
>>>> straightforward.
>>>>
>>>
>>> Sorry, but I would like to get an ack from i915 folks if this is going
>>> to be useful to them if we move this to helper because we have to 
>>> look at every table. Not just one.
>>
>> Added i915 maintainers to the CC list for them to be able to answer.
>>
> 
> Thanks, lets wait to hear from them about where finally these tables 
> should go but thats can be taken up as a separate effort too.
> 
>>>
>>> Also, this is just 1.1, we will add more tables for 1.2. So we will 
>>> have to end up changing both 1.1 and 1.2 tables as they are different 
>>> for QC.
>>
>> I haven't heard back from Kuogee about the possible causes of using 
>> rc/qp values from 1.2 even for 1.1 panels. Maybe you can comment on 
>> that? In other words, can we always stick to the values from 1.2 
>> standard? What will be the drawback?
>>
>> Otherwise, we'd have to have two different sets of values, like you do 
>> in your vendor driver.
>>
> 
> I have responded to this in the other email.
> 
> All this being said, even if the rc tables move the drm_dsc_helper 
> either now or later on, we will still need MSM specific calculations for 
> many of the other encoder parameters (which are again either hard-coded 
> or calculated). Please refer to the sde_dsc_populate_dsc_config() 
> downstream. And yes, you will not find those in the DP spec directly.
> 
> So we will still need a dsc helper for MSM calculations to be common for 
> DSI / DP irrespective of where the tables go.
> 
> So, lets finalize that first.

I went on and trimmed sde_dsc_populate_dsc_config() to remove 
duplication with the drm_dsc_compute_rc_parameters() (which we already 
use for the MSM DSI DSC).

Not much is left:

dsc->first_line_bpg_offset set via the switch

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;

if ((dsc->dsc_version_minor == 0x2) && (dsc->native_420))
     dsc->nsl_bpg_offset = (2048 *
              (DIV_ROUND_UP(dsc->second_line_bpg_offset,
                                 (dsc->slice_height - 1))));

dsc->initial_scale_value = 8 * dsc->rc_model_size /
                         (dsc->rc_model_size - dsc->initial_offset);


mux_word_size comes from the standard (must)
initial_scale_value calculation is recommended, but not required
nsl_bpg_offset follows the standard (must), also see below (*).

first_line_bpg_offset calculation differs between three drivers. The 
standard also provides a recommended formulas. I think we can leave it 
as is for now.

I think, that mux_word_size and nsl_bpg_offset calculation should be 
moved to drm_dsc_compute_rc_parameters(), while leaving 
initial_scale_value in place (in the driver code).

* I think nsl_bpg_offset is slightly incorrectly calculated. Standard 
demands that it is set to 'second_line_bpg_offset / (slice_height - 1), 
rounded up to 16 fraction bits', while SDE driver code sets it to the 
value rounded up to the next integer (having 16 fraction bits 
representation).

In my opinion correct calculation should be:
dsc->nsl_bpg_offset = DIV_ROUND_UP(2048 * dsc->second_line_bpg_offset,
                                 (dsc->slice_height - 1));

Could you please check, which one is correct according to the standard?
Dmitry Baryshkov Feb. 26, 2023, 1:13 p.m. UTC | #13
On 26/02/2023 02:16, Abhinav Kumar wrote:
> Hi Dmitry
> 
> On 2/24/2023 3:57 PM, Dmitry Baryshkov wrote:
>> 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?
>>
> 
> Standards change request means fixing errors/errata for the given 
> standard. Those are typically released as a different spec.
> 
> So I referred the DSC 1.1 SCR spec, and it does have a few differences 
> in the table compared to DSC 1.1 which will get into DSC 1.2.
> 
> Hence the table entries are same between DSC 1.1 SCR and DSC 1.2
> 
> You are right, ideally DSC 1.2 should be backwards compatible with DSC 
> 1.1 in terms of the values (thats what the spec says too) but I am not 
> sure if we can expect every panel/DP monitor to be forward compatible 
> without any SW change because it might need some firmware update (for 
> the panel) or SW update to support that especially during transitions of 
> the spec revisions (SCR to be precise).
> 
> Typically we do below for DP monitors exactly for the same reason:
> 
> DSC_ver_to_use = min(what_we_support, what_DP_monitor_supports) and use 
> that table.
> 
> For DSI panels, typically in the panel spec it should say whether the 
> SCR version needs to be used because we have seen that for some panels ( 
> I dont remember exactly which one ) based on which panel and which 
> revision of the panel, it might not.

So, what happens if we use DSC 1.1 SCR (= DSC 1.2) values with older 
panel? Does it result in the broken image?

I'm asking here, because I think that these parameters tune the 
_encoder_. The decoder should be able to handle different compressed 
streams as long as values fit into the required 'profile'.

> 
> Thats why downstream started adding qcom,mdss-dsc-scr-version to the 
> devicetree.
> 
>>>>> +    {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},
>>
>>
Abhinav Kumar Feb. 26, 2023, 11:49 p.m. UTC | #14
On 2/26/2023 5:09 AM, Dmitry Baryshkov wrote:
> On 26/02/2023 02:47, Abhinav Kumar wrote:
>> Hi Dmitry
>>
>> On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
>>> On 25/02/2023 02:36, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
>>>>> On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar 
>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
>>>>>>> 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar 
>>>>>>> <quic_abhinavk@quicinc.com> пишет:
>>>>>>>> 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).
>>>>>>>>>
>>>>>>>>
>>>>>>>> No, it cannot. So each DSC encoder parameter is calculated based 
>>>>>>>> on the HW core which is being used.
>>>>>>>>
>>>>>>>> They all get packed to the same DSC structure which is the 
>>>>>>>> struct drm_dsc_config but the way the parameters are computed is 
>>>>>>>> specific to the HW.
>>>>>>>>
>>>>>>>> This DPU file helper still uses the drm_dsc_helper's 
>>>>>>>> drm_dsc_compute_rc_parameters() like all other vendors do but 
>>>>>>>> the parameters themselves are very HW specific and belong to 
>>>>>>>> each vendor's dir.
>>>>>>>>
>>>>>>>> This is not unique to MSM.
>>>>>>>>
>>>>>>>> Lets take a few other examples:
>>>>>>>>
>>>>>>>> AMD: 
>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165 
>>>>>>>>
>>>>>>>>
>>>>>>>> i915: 
>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379 
>>>>>>>>
>>>>>>>
>>>>>>> I checked several values here. Intel driver defines more bpc/bpp 
>>>>>>> combinations, but the ones which are defined in intel_vdsc and in 
>>>>>>> this patch seem to match. If there are major differences there, 
>>>>>>> please point me to the exact case.
>>>>>>>
>>>>>>> I remember that AMD driver might have different values.
>>>>>>>
>>>>>>
>>>>>> Some values in the rc_params table do match. But the 
>>>>>> rc_buf_thresh[] doesnt.
>>>>>
>>>>> Because later they do:
>>>>>
>>>>> vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
>>>>>
>>>>>>
>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40 
>>>>>>
>>>>>>
>>>>>> Vs
>>>>>>
>>>>>> +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
>>>>>> +};
>>>>>
>>>>> I'd prefer to have 896, 1792, etc. here, as those values come from the
>>>>> standard. As it's done in the Intel driver.
>>>>>
>>>>
>>>> Got it, thanks
>>>>
>>>>>> I dont know the AMD calculation very well to say that moving this 
>>>>>> to the
>>>>>> helper is going to help.
>>>>>
>>>>> Those calculations correspond (more or less) at the first glance to
>>>>> what intel does for their newer generations. I think that's not our
>>>>> problem for now.
>>>>>
>>>>
>>>> Well, we have to figure out if each value matches and if each of 
>>>> them come from the spec for us and i915 and from which section. So 
>>>> it is unfortunately our problem.
>>>
>>> Otherwise it will have to be handled by Marijn, me or anybody else 
>>> wanting to hack up the DSC code. Or by anybody adding DSC support to 
>>> the next platform and having to figure out the difference between 
>>> i915, msm and their platform.
>>>
>>
>> Yes, I wonder why the same doubt didn't arise when the other vendors 
>> added their support both from other maintainers and others.
>>
>> Which makes me think that like I wrote in my previous response, these 
>> are "recommended" values in the spec but its not mandatory.
> 
> I think, it is because there were no other drivers to compare. In other 
> words, for a first driver it is pretty logical to have everything 
> handled on its own. As soon as we start getting other implementations of 
> a feature, it becomes logical to think if the code can be generalized. 
> This is what we see we with the HDCP series or with the code being moved 
> to DP helpers.
> 

We were not the second, MSM was/is the third to add support for DSC afer 
i915 and AMD. Thats what made me think why whoever was the second didnt 
end up generalizing. Was it just missed out or was it intentionally left 
in the vendor driver.

>>
>> Moving this to the drm_dsc_helper is generalizing the tables and not 
>> giving room for the vendors to customize even if they want to (which 
>> the spec does allow).
> 
> That depends on the API you select. For example, in 
> intel_dsc_compute_params() I see customization being applied to 
> rc_buf_thresh in 6bpp case. I'd leave that to the i915 driver.
> 

Thanks for going through the i915 to figure out that the 6bpp is handled 
in a customized way. So what you are saying is let the helper first fill 
up the recommended values of the spec, whatever is changed from that let 
the vendor driver override that.

Thats where the case-by-case handling comes.

Why not we do this way? Like you mentioned lets move these tables to the 
drm_dsc_helper and let MSM driver first use those.

Then in a separate patchset if i915 and AMD would like to move to that, 
let them handle it for their respective drivers instead of MSM going 
through whats customized for each calculation and doing it.

I am hesitant to take up that effort.

If the recommended values work for the vendor, they can clean it up and 
move to the drm_dsc_helper themselves and preserving their 
customizations rather than one vendor doing it for all of them.

> In case the driver needs to perform customization of the params, nothing 
> stops it drop applying after filling all the RC params in the 
> drm_dsc_config struct via the generic helper.
> 
> 
>> So if this has any merit and if you or Marijn would like to take it 
>> up, go for it. We would do the same thing as either of you would have 
>> to in terms of figuring out the difference between msm and the i915 code.
>>
>> This is not a generic API we are trying to put in a helper, these are 
>> hard-coded tables so there is a difference between looking at these Vs 
>> looking at some common code which can move to the core.
>>
>>>>
>>>>>>
>>>>>> Also, i think its too risky to change other drivers to use 
>>>>>> whatever math
>>>>>> we put in the drm_dsc_helper to compute thr RC params because 
>>>>>> their code
>>>>>> might be computing and using this tables differently.
>>>>>>
>>>>>> Its too much ownership for MSM developers to move this to 
>>>>>> drm_dsc_helper
>>>>>> and own that as it might cause breakage of basic DSC even if some 
>>>>>> values
>>>>>> are repeated.
>>>>>
>>>>> It's time to stop thinking about ownership and start thinking about
>>>>> shared code. We already have two instances of DSC tables. I don't
>>>>> think having a third instance, which is a subset of an existing
>>>>> dataset, would be beneficial to anybody.
>>>>> AMD has complicated code which supports half-bit bpp and calculates
>>>>> some of the parameters. But sharing data with the i915 driver is
>>>>> straightforward.
>>>>>
>>>>
>>>> Sorry, but I would like to get an ack from i915 folks if this is going
>>>> to be useful to them if we move this to helper because we have to 
>>>> look at every table. Not just one.
>>>
>>> Added i915 maintainers to the CC list for them to be able to answer.
>>>
>>
>> Thanks, lets wait to hear from them about where finally these tables 
>> should go but thats can be taken up as a separate effort too.
>>
>>>>
>>>> Also, this is just 1.1, we will add more tables for 1.2. So we will 
>>>> have to end up changing both 1.1 and 1.2 tables as they are 
>>>> different for QC.
>>>
>>> I haven't heard back from Kuogee about the possible causes of using 
>>> rc/qp values from 1.2 even for 1.1 panels. Maybe you can comment on 
>>> that? In other words, can we always stick to the values from 1.2 
>>> standard? What will be the drawback?
>>>
>>> Otherwise, we'd have to have two different sets of values, like you 
>>> do in your vendor driver.
>>>
>>
>> I have responded to this in the other email.
>>
>> All this being said, even if the rc tables move the drm_dsc_helper 
>> either now or later on, we will still need MSM specific calculations 
>> for many of the other encoder parameters (which are again either 
>> hard-coded or calculated). Please refer to the 
>> sde_dsc_populate_dsc_config() downstream. And yes, you will not find 
>> those in the DP spec directly.
>>
>> So we will still need a dsc helper for MSM calculations to be common 
>> for DSI / DP irrespective of where the tables go.
>>
>> So, lets finalize that first.
> 
> I went on and trimmed sde_dsc_populate_dsc_config() to remove 
> duplication with the drm_dsc_compute_rc_parameters() (which we already 
> use for the MSM DSI DSC).
> 
> Not much is left:
> 
> dsc->first_line_bpg_offset set via the switch
> 
> 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;
> 
> if ((dsc->dsc_version_minor == 0x2) && (dsc->native_420))
>      dsc->nsl_bpg_offset = (2048 *
>               (DIV_ROUND_UP(dsc->second_line_bpg_offset,
>                                  (dsc->slice_height - 1))));
> 
> dsc->initial_scale_value = 8 * dsc->rc_model_size /
>                          (dsc->rc_model_size - dsc->initial_offset);
> 
> 
> mux_word_size comes from the standard (must)
> initial_scale_value calculation is recommended, but not required
> nsl_bpg_offset follows the standard (must), also see below (*).
> 
> first_line_bpg_offset calculation differs between three drivers. The 
> standard also provides a recommended formulas. I think we can leave it 
> as is for now.
> 
> I think, that mux_word_size and nsl_bpg_offset calculation should be 
> moved to drm_dsc_compute_rc_parameters(), while leaving 
> initial_scale_value in place (in the driver code).
> 
> * I think nsl_bpg_offset is slightly incorrectly calculated. Standard 
> demands that it is set to 'second_line_bpg_offset / (slice_height - 1), 
> rounded up to 16 fraction bits', while SDE driver code sets it to the 
> value rounded up to the next integer (having 16 fraction bits 
> representation).
> 
> In my opinion correct calculation should be:
> dsc->nsl_bpg_offset = DIV_ROUND_UP(2048 * dsc->second_line_bpg_offset,
>                                  (dsc->slice_height - 1));
> 
> Could you please check, which one is correct according to the standard?
> 
> 

Sure, i will check about nsl_bpg_offset. But sorry if I was not more 
clear about this but sde_dsc_populate_dsc_config() is only one example 
which from your analysis can be moved to the drm_dsc_helper() but not 
the initial line calculation _dce_dsc_initial_line_calc(), 
_dce_dsc_ich_reset_override_needed() , _dce_dsc_setup_helper().

All of these are again common between DSI and DP.

So in addition to thinking about what can be moved to the drm_dsc_helper 
also think about what is specific to MSM but common to DSI and DP modules.

That was the bigger picture I was trying to convey.
Abhinav Kumar Feb. 27, 2023, 12:15 a.m. UTC | #15
On 2/26/2023 5:13 AM, Dmitry Baryshkov wrote:
> On 26/02/2023 02:16, Abhinav Kumar wrote:
>> Hi Dmitry
>>
>> On 2/24/2023 3:57 PM, Dmitry Baryshkov wrote:
>>> 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?
>>>
>>
>> Standards change request means fixing errors/errata for the given 
>> standard. Those are typically released as a different spec.
>>
>> So I referred the DSC 1.1 SCR spec, and it does have a few differences 
>> in the table compared to DSC 1.1 which will get into DSC 1.2.
>>
>> Hence the table entries are same between DSC 1.1 SCR and DSC 1.2
>>
>> You are right, ideally DSC 1.2 should be backwards compatible with DSC 
>> 1.1 in terms of the values (thats what the spec says too) but I am not 
>> sure if we can expect every panel/DP monitor to be forward compatible 
>> without any SW change because it might need some firmware update (for 
>> the panel) or SW update to support that especially during transitions 
>> of the spec revisions (SCR to be precise).
>>
>> Typically we do below for DP monitors exactly for the same reason:
>>
>> DSC_ver_to_use = min(what_we_support, what_DP_monitor_supports) and 
>> use that table.
>>
>> For DSI panels, typically in the panel spec it should say whether the 
>> SCR version needs to be used because we have seen that for some panels 
>> ( I dont remember exactly which one ) based on which panel and which 
>> revision of the panel, it might not.
> 
> So, what happens if we use DSC 1.1 SCR (= DSC 1.2) values with older 
> panel? Does it result in the broken image?
> 
> I'm asking here, because I think that these parameters tune the 
> _encoder_. The decoder should be able to handle different compressed 
> streams as long as values fit into the required 'profile'.
> 
Yes, this can cause screen corruption issues.

The RC parameters table is used both in the encoder and in the PPS too 
and will be used to decode too.

If we use the DSC 1.2 tables for a monitor/panel which advertizes that 
it supports only 1.1, we cannot be certain it will work.


>>
>> Thats why downstream started adding qcom,mdss-dsc-scr-version to the 
>> devicetree.
>>
>>>>>> +    {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},
>>>
>>>
>
Dmitry Baryshkov Feb. 27, 2023, 12:33 p.m. UTC | #16
On Mon, 27 Feb 2023 at 02:15, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 2/26/2023 5:13 AM, Dmitry Baryshkov wrote:
> > On 26/02/2023 02:16, Abhinav Kumar wrote:
> >> Hi Dmitry
> >>
> >> On 2/24/2023 3:57 PM, Dmitry Baryshkov wrote:
> >>> 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?
> >>>
> >>
> >> Standards change request means fixing errors/errata for the given
> >> standard. Those are typically released as a different spec.
> >>
> >> So I referred the DSC 1.1 SCR spec, and it does have a few differences
> >> in the table compared to DSC 1.1 which will get into DSC 1.2.
> >>
> >> Hence the table entries are same between DSC 1.1 SCR and DSC 1.2
> >>
> >> You are right, ideally DSC 1.2 should be backwards compatible with DSC
> >> 1.1 in terms of the values (thats what the spec says too) but I am not
> >> sure if we can expect every panel/DP monitor to be forward compatible
> >> without any SW change because it might need some firmware update (for
> >> the panel) or SW update to support that especially during transitions
> >> of the spec revisions (SCR to be precise).
> >>
> >> Typically we do below for DP monitors exactly for the same reason:
> >>
> >> DSC_ver_to_use = min(what_we_support, what_DP_monitor_supports) and
> >> use that table.
> >>
> >> For DSI panels, typically in the panel spec it should say whether the
> >> SCR version needs to be used because we have seen that for some panels
> >> ( I dont remember exactly which one ) based on which panel and which
> >> revision of the panel, it might not.
> >
> > So, what happens if we use DSC 1.1 SCR (= DSC 1.2) values with older
> > panel? Does it result in the broken image?
> >
> > I'm asking here, because I think that these parameters tune the
> > _encoder_. The decoder should be able to handle different compressed
> > streams as long as values fit into the required 'profile'.
> >
> Yes, this can cause screen corruption issues.
>
> The RC parameters table is used both in the encoder and in the PPS too
> and will be used to decode too.
>
> If we use the DSC 1.2 tables for a monitor/panel which advertizes that
> it supports only 1.1, we cannot be certain it will work.

But aren't those tables the recommended ones? So the panels should
work with a broader range of possible settings, shouldn't they?

Anyway, probably you have observed some kind of image corruption when
using DSC 1.1 SCR parameters with older panels.

>
>
> >>
> >> Thats why downstream started adding qcom,mdss-dsc-scr-version to the
> >> devicetree.
> >>
> >>>>>> +    {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},
> >>>
> >>>
> >
Dmitry Baryshkov Feb. 27, 2023, 12:45 p.m. UTC | #17
On Mon, 27 Feb 2023 at 01:49, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 2/26/2023 5:09 AM, Dmitry Baryshkov wrote:
> > On 26/02/2023 02:47, Abhinav Kumar wrote:
> >> Hi Dmitry
> >>
> >> On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
> >>> On 25/02/2023 02:36, Abhinav Kumar wrote:
> >>>>
> >>>>
> >>>> On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
> >>>>> On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar
> >>>>> <quic_abhinavk@quicinc.com> wrote:
> >>>>>> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
> >>>>>>> 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar
> >>>>>>> <quic_abhinavk@quicinc.com> пишет:
> >>>>>>>> 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).
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> No, it cannot. So each DSC encoder parameter is calculated based
> >>>>>>>> on the HW core which is being used.
> >>>>>>>>
> >>>>>>>> They all get packed to the same DSC structure which is the
> >>>>>>>> struct drm_dsc_config but the way the parameters are computed is
> >>>>>>>> specific to the HW.
> >>>>>>>>
> >>>>>>>> This DPU file helper still uses the drm_dsc_helper's
> >>>>>>>> drm_dsc_compute_rc_parameters() like all other vendors do but
> >>>>>>>> the parameters themselves are very HW specific and belong to
> >>>>>>>> each vendor's dir.
> >>>>>>>>
> >>>>>>>> This is not unique to MSM.
> >>>>>>>>
> >>>>>>>> Lets take a few other examples:
> >>>>>>>>
> >>>>>>>> AMD:
> >>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> i915:
> >>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
> >>>>>>>>
> >>>>>>>
> >>>>>>> I checked several values here. Intel driver defines more bpc/bpp
> >>>>>>> combinations, but the ones which are defined in intel_vdsc and in
> >>>>>>> this patch seem to match. If there are major differences there,
> >>>>>>> please point me to the exact case.
> >>>>>>>
> >>>>>>> I remember that AMD driver might have different values.
> >>>>>>>
> >>>>>>
> >>>>>> Some values in the rc_params table do match. But the
> >>>>>> rc_buf_thresh[] doesnt.
> >>>>>
> >>>>> Because later they do:
> >>>>>
> >>>>> vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
> >>>>>
> >>>>>>
> >>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
> >>>>>>
> >>>>>>
> >>>>>> Vs
> >>>>>>
> >>>>>> +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
> >>>>>> +};
> >>>>>
> >>>>> I'd prefer to have 896, 1792, etc. here, as those values come from the
> >>>>> standard. As it's done in the Intel driver.
> >>>>>
> >>>>
> >>>> Got it, thanks
> >>>>
> >>>>>> I dont know the AMD calculation very well to say that moving this
> >>>>>> to the
> >>>>>> helper is going to help.
> >>>>>
> >>>>> Those calculations correspond (more or less) at the first glance to
> >>>>> what intel does for their newer generations. I think that's not our
> >>>>> problem for now.
> >>>>>
> >>>>
> >>>> Well, we have to figure out if each value matches and if each of
> >>>> them come from the spec for us and i915 and from which section. So
> >>>> it is unfortunately our problem.
> >>>
> >>> Otherwise it will have to be handled by Marijn, me or anybody else
> >>> wanting to hack up the DSC code. Or by anybody adding DSC support to
> >>> the next platform and having to figure out the difference between
> >>> i915, msm and their platform.
> >>>
> >>
> >> Yes, I wonder why the same doubt didn't arise when the other vendors
> >> added their support both from other maintainers and others.
> >>
> >> Which makes me think that like I wrote in my previous response, these
> >> are "recommended" values in the spec but its not mandatory.
> >
> > I think, it is because there were no other drivers to compare. In other
> > words, for a first driver it is pretty logical to have everything
> > handled on its own. As soon as we start getting other implementations of
> > a feature, it becomes logical to think if the code can be generalized.
> > This is what we see we with the HDCP series or with the code being moved
> > to DP helpers.
> >
>
> We were not the second, MSM was/is the third to add support for DSC afer
> i915 and AMD. Thats what made me think why whoever was the second didnt
> end up generalizing. Was it just missed out or was it intentionally left
> in the vendor driver.

I didn't count AMD here, since it calculates some of the params rather
than using the fixed ones from the model.

>
> >>
> >> Moving this to the drm_dsc_helper is generalizing the tables and not
> >> giving room for the vendors to customize even if they want to (which
> >> the spec does allow).
> >
> > That depends on the API you select. For example, in
> > intel_dsc_compute_params() I see customization being applied to
> > rc_buf_thresh in 6bpp case. I'd leave that to the i915 driver.
> >
>
> Thanks for going through the i915 to figure out that the 6bpp is handled
> in a customized way. So what you are saying is let the helper first fill
> up the recommended values of the spec, whatever is changed from that let
> the vendor driver override that.
>
> Thats where the case-by-case handling comes.
>
> Why not we do this way? Like you mentioned lets move these tables to the
> drm_dsc_helper and let MSM driver first use those.
>
> Then in a separate patchset if i915 and AMD would like to move to that,
> let them handle it for their respective drivers instead of MSM going
> through whats customized for each calculation and doing it.
>
> I am hesitant to take up that effort.

Writing a tool to convert model's rc_Nbpc_Mbpp_foo.cfg into C
languages structures used by Intel code took 15-20 minutes. Plugging
generated structures took another 5 minutes. I will send the patches
later today or tomorrow, as I find a time slot to clean them. Thank
you for spending more time on arguing than it took me to generate &
verify the data.

>
> If the recommended values work for the vendor, they can clean it up and
> move to the drm_dsc_helper themselves and preserving their
> customizations rather than one vendor doing it for all of them.
>
> > In case the driver needs to perform customization of the params, nothing
> > stops it drop applying after filling all the RC params in the
> > drm_dsc_config struct via the generic helper.
> >
> >
> >> So if this has any merit and if you or Marijn would like to take it
> >> up, go for it. We would do the same thing as either of you would have
> >> to in terms of figuring out the difference between msm and the i915 code.
> >>
> >> This is not a generic API we are trying to put in a helper, these are
> >> hard-coded tables so there is a difference between looking at these Vs
> >> looking at some common code which can move to the core.
> >>
> >>>>
> >>>>>>
> >>>>>> Also, i think its too risky to change other drivers to use
> >>>>>> whatever math
> >>>>>> we put in the drm_dsc_helper to compute thr RC params because
> >>>>>> their code
> >>>>>> might be computing and using this tables differently.
> >>>>>>
> >>>>>> Its too much ownership for MSM developers to move this to
> >>>>>> drm_dsc_helper
> >>>>>> and own that as it might cause breakage of basic DSC even if some
> >>>>>> values
> >>>>>> are repeated.
> >>>>>
> >>>>> It's time to stop thinking about ownership and start thinking about
> >>>>> shared code. We already have two instances of DSC tables. I don't
> >>>>> think having a third instance, which is a subset of an existing
> >>>>> dataset, would be beneficial to anybody.
> >>>>> AMD has complicated code which supports half-bit bpp and calculates
> >>>>> some of the parameters. But sharing data with the i915 driver is
> >>>>> straightforward.
> >>>>>
> >>>>
> >>>> Sorry, but I would like to get an ack from i915 folks if this is going
> >>>> to be useful to them if we move this to helper because we have to
> >>>> look at every table. Not just one.
> >>>
> >>> Added i915 maintainers to the CC list for them to be able to answer.
> >>>
> >>
> >> Thanks, lets wait to hear from them about where finally these tables
> >> should go but thats can be taken up as a separate effort too.
> >>
> >>>>
> >>>> Also, this is just 1.1, we will add more tables for 1.2. So we will
> >>>> have to end up changing both 1.1 and 1.2 tables as they are
> >>>> different for QC.
> >>>
> >>> I haven't heard back from Kuogee about the possible causes of using
> >>> rc/qp values from 1.2 even for 1.1 panels. Maybe you can comment on
> >>> that? In other words, can we always stick to the values from 1.2
> >>> standard? What will be the drawback?
> >>>
> >>> Otherwise, we'd have to have two different sets of values, like you
> >>> do in your vendor driver.
> >>>
> >>
> >> I have responded to this in the other email.
> >>
> >> All this being said, even if the rc tables move the drm_dsc_helper
> >> either now or later on, we will still need MSM specific calculations
> >> for many of the other encoder parameters (which are again either
> >> hard-coded or calculated). Please refer to the
> >> sde_dsc_populate_dsc_config() downstream. And yes, you will not find
> >> those in the DP spec directly.
> >>
> >> So we will still need a dsc helper for MSM calculations to be common
> >> for DSI / DP irrespective of where the tables go.
> >>
> >> So, lets finalize that first.
> >
> > I went on and trimmed sde_dsc_populate_dsc_config() to remove
> > duplication with the drm_dsc_compute_rc_parameters() (which we already
> > use for the MSM DSI DSC).
> >
> > Not much is left:
> >
> > dsc->first_line_bpg_offset set via the switch
> >
> > 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;
> >
> > if ((dsc->dsc_version_minor == 0x2) && (dsc->native_420))
> >      dsc->nsl_bpg_offset = (2048 *
> >               (DIV_ROUND_UP(dsc->second_line_bpg_offset,
> >                                  (dsc->slice_height - 1))));
> >
> > dsc->initial_scale_value = 8 * dsc->rc_model_size /
> >                          (dsc->rc_model_size - dsc->initial_offset);
> >
> >
> > mux_word_size comes from the standard (must)
> > initial_scale_value calculation is recommended, but not required
> > nsl_bpg_offset follows the standard (must), also see below (*).
> >
> > first_line_bpg_offset calculation differs between three drivers. The
> > standard also provides a recommended formulas. I think we can leave it
> > as is for now.
> >
> > I think, that mux_word_size and nsl_bpg_offset calculation should be
> > moved to drm_dsc_compute_rc_parameters(), while leaving
> > initial_scale_value in place (in the driver code).
> >
> > * I think nsl_bpg_offset is slightly incorrectly calculated. Standard
> > demands that it is set to 'second_line_bpg_offset / (slice_height - 1),
> > rounded up to 16 fraction bits', while SDE driver code sets it to the
> > value rounded up to the next integer (having 16 fraction bits
> > representation).
> >
> > In my opinion correct calculation should be:
> > dsc->nsl_bpg_offset = DIV_ROUND_UP(2048 * dsc->second_line_bpg_offset,
> >                                  (dsc->slice_height - 1));
> >
> > Could you please check, which one is correct according to the standard?
> >
> >
>
> Sure, i will check about nsl_bpg_offset. But sorry if I was not more
> clear about this but sde_dsc_populate_dsc_config() is only one example
> which from your analysis can be moved to the drm_dsc_helper() but not
> the initial line calculation _dce_dsc_initial_line_calc(),
> _dce_dsc_ich_reset_override_needed() , _dce_dsc_setup_helper().

The initial_line is already calculated in dpu_encoder.c. As for the
_dce_dsc_ich_reset_override_needed(), I don't think we support partial
updates in the upstream driver.

>
> All of these are again common between DSI and DP.
>
> So in addition to thinking about what can be moved to the drm_dsc_helper
> also think about what is specific to MSM but common to DSI and DP modules.
>
> That was the bigger picture I was trying to convey.
Jani Nikula Feb. 27, 2023, 12:48 p.m. UTC | #18
On Sun, 26 Feb 2023, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> On 2/26/2023 5:09 AM, Dmitry Baryshkov wrote:
>> On 26/02/2023 02:47, Abhinav Kumar wrote:
>>> Hi Dmitry
>>>
>>> On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
>>>> On 25/02/2023 02:36, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
>>>>>> On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar 
>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
>>>>>>>> 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar 
>>>>>>>> <quic_abhinavk@quicinc.com> пишет:
>>>>>>>>> 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).
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> No, it cannot. So each DSC encoder parameter is calculated based 
>>>>>>>>> on the HW core which is being used.
>>>>>>>>>
>>>>>>>>> They all get packed to the same DSC structure which is the 
>>>>>>>>> struct drm_dsc_config but the way the parameters are computed is 
>>>>>>>>> specific to the HW.
>>>>>>>>>
>>>>>>>>> This DPU file helper still uses the drm_dsc_helper's 
>>>>>>>>> drm_dsc_compute_rc_parameters() like all other vendors do but 
>>>>>>>>> the parameters themselves are very HW specific and belong to 
>>>>>>>>> each vendor's dir.
>>>>>>>>>
>>>>>>>>> This is not unique to MSM.
>>>>>>>>>
>>>>>>>>> Lets take a few other examples:
>>>>>>>>>
>>>>>>>>> AMD: 
>>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> i915: 
>>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379 
>>>>>>>>>
>>>>>>>>
>>>>>>>> I checked several values here. Intel driver defines more bpc/bpp 
>>>>>>>> combinations, but the ones which are defined in intel_vdsc and in 
>>>>>>>> this patch seem to match. If there are major differences there, 
>>>>>>>> please point me to the exact case.
>>>>>>>>
>>>>>>>> I remember that AMD driver might have different values.
>>>>>>>>
>>>>>>>
>>>>>>> Some values in the rc_params table do match. But the 
>>>>>>> rc_buf_thresh[] doesnt.
>>>>>>
>>>>>> Because later they do:
>>>>>>
>>>>>> vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
>>>>>>
>>>>>>>
>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40 
>>>>>>>
>>>>>>>
>>>>>>> Vs
>>>>>>>
>>>>>>> +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
>>>>>>> +};
>>>>>>
>>>>>> I'd prefer to have 896, 1792, etc. here, as those values come from the
>>>>>> standard. As it's done in the Intel driver.
>>>>>>
>>>>>
>>>>> Got it, thanks
>>>>>
>>>>>>> I dont know the AMD calculation very well to say that moving this 
>>>>>>> to the
>>>>>>> helper is going to help.
>>>>>>
>>>>>> Those calculations correspond (more or less) at the first glance to
>>>>>> what intel does for their newer generations. I think that's not our
>>>>>> problem for now.
>>>>>>
>>>>>
>>>>> Well, we have to figure out if each value matches and if each of 
>>>>> them come from the spec for us and i915 and from which section. So 
>>>>> it is unfortunately our problem.
>>>>
>>>> Otherwise it will have to be handled by Marijn, me or anybody else 
>>>> wanting to hack up the DSC code. Or by anybody adding DSC support to 
>>>> the next platform and having to figure out the difference between 
>>>> i915, msm and their platform.
>>>>
>>>
>>> Yes, I wonder why the same doubt didn't arise when the other vendors 
>>> added their support both from other maintainers and others.
>>>
>>> Which makes me think that like I wrote in my previous response, these 
>>> are "recommended" values in the spec but its not mandatory.
>> 
>> I think, it is because there were no other drivers to compare. In other 
>> words, for a first driver it is pretty logical to have everything 
>> handled on its own. As soon as we start getting other implementations of 
>> a feature, it becomes logical to think if the code can be generalized. 
>> This is what we see we with the HDCP series or with the code being moved 
>> to DP helpers.
>> 
>
> We were not the second, MSM was/is the third to add support for DSC afer 
> i915 and AMD. Thats what made me think why whoever was the second didnt 
> end up generalizing. Was it just missed out or was it intentionally left 
> in the vendor driver.
>
>>>
>>> Moving this to the drm_dsc_helper is generalizing the tables and not 
>>> giving room for the vendors to customize even if they want to (which 
>>> the spec does allow).
>> 
>> That depends on the API you select. For example, in 
>> intel_dsc_compute_params() I see customization being applied to 
>> rc_buf_thresh in 6bpp case. I'd leave that to the i915 driver.
>> 
>
> Thanks for going through the i915 to figure out that the 6bpp is handled 
> in a customized way. So what you are saying is let the helper first fill 
> up the recommended values of the spec, whatever is changed from that let 
> the vendor driver override that.
>
> Thats where the case-by-case handling comes.
>
> Why not we do this way? Like you mentioned lets move these tables to the 
> drm_dsc_helper and let MSM driver first use those.
>
> Then in a separate patchset if i915 and AMD would like to move to that, 
> let them handle it for their respective drivers instead of MSM going 
> through whats customized for each calculation and doing it.
>
> I am hesitant to take up that effort.
>
> If the recommended values work for the vendor, they can clean it up and 
> move to the drm_dsc_helper themselves and preserving their 
> customizations rather than one vendor doing it for all of them.

I think the thing to do is to define *interfaces* for accessing the
various parameters in a reasonable manner.

Even in your code, it'll be helpful for any future conversion to shared
arrays to not access the local arrays directly.

If this gets added to kms helpers, with the spec values verbatim and
reasonable interfaces, i915 will happily convert to using them.

BR,
Jani.



>
>> In case the driver needs to perform customization of the params, nothing 
>> stops it drop applying after filling all the RC params in the 
>> drm_dsc_config struct via the generic helper.
>> 
>> 
>>> So if this has any merit and if you or Marijn would like to take it 
>>> up, go for it. We would do the same thing as either of you would have 
>>> to in terms of figuring out the difference between msm and the i915 code.
>>>
>>> This is not a generic API we are trying to put in a helper, these are 
>>> hard-coded tables so there is a difference between looking at these Vs 
>>> looking at some common code which can move to the core.
>>>
>>>>>
>>>>>>>
>>>>>>> Also, i think its too risky to change other drivers to use 
>>>>>>> whatever math
>>>>>>> we put in the drm_dsc_helper to compute thr RC params because 
>>>>>>> their code
>>>>>>> might be computing and using this tables differently.
>>>>>>>
>>>>>>> Its too much ownership for MSM developers to move this to 
>>>>>>> drm_dsc_helper
>>>>>>> and own that as it might cause breakage of basic DSC even if some 
>>>>>>> values
>>>>>>> are repeated.
>>>>>>
>>>>>> It's time to stop thinking about ownership and start thinking about
>>>>>> shared code. We already have two instances of DSC tables. I don't
>>>>>> think having a third instance, which is a subset of an existing
>>>>>> dataset, would be beneficial to anybody.
>>>>>> AMD has complicated code which supports half-bit bpp and calculates
>>>>>> some of the parameters. But sharing data with the i915 driver is
>>>>>> straightforward.
>>>>>>
>>>>>
>>>>> Sorry, but I would like to get an ack from i915 folks if this is going
>>>>> to be useful to them if we move this to helper because we have to 
>>>>> look at every table. Not just one.
>>>>
>>>> Added i915 maintainers to the CC list for them to be able to answer.
>>>>
>>>
>>> Thanks, lets wait to hear from them about where finally these tables 
>>> should go but thats can be taken up as a separate effort too.
>>>
>>>>>
>>>>> Also, this is just 1.1, we will add more tables for 1.2. So we will 
>>>>> have to end up changing both 1.1 and 1.2 tables as they are 
>>>>> different for QC.
>>>>
>>>> I haven't heard back from Kuogee about the possible causes of using 
>>>> rc/qp values from 1.2 even for 1.1 panels. Maybe you can comment on 
>>>> that? In other words, can we always stick to the values from 1.2 
>>>> standard? What will be the drawback?
>>>>
>>>> Otherwise, we'd have to have two different sets of values, like you 
>>>> do in your vendor driver.
>>>>
>>>
>>> I have responded to this in the other email.
>>>
>>> All this being said, even if the rc tables move the drm_dsc_helper 
>>> either now or later on, we will still need MSM specific calculations 
>>> for many of the other encoder parameters (which are again either 
>>> hard-coded or calculated). Please refer to the 
>>> sde_dsc_populate_dsc_config() downstream. And yes, you will not find 
>>> those in the DP spec directly.
>>>
>>> So we will still need a dsc helper for MSM calculations to be common 
>>> for DSI / DP irrespective of where the tables go.
>>>
>>> So, lets finalize that first.
>> 
>> I went on and trimmed sde_dsc_populate_dsc_config() to remove 
>> duplication with the drm_dsc_compute_rc_parameters() (which we already 
>> use for the MSM DSI DSC).
>> 
>> Not much is left:
>> 
>> dsc->first_line_bpg_offset set via the switch
>> 
>> 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;
>> 
>> if ((dsc->dsc_version_minor == 0x2) && (dsc->native_420))
>>      dsc->nsl_bpg_offset = (2048 *
>>               (DIV_ROUND_UP(dsc->second_line_bpg_offset,
>>                                  (dsc->slice_height - 1))));
>> 
>> dsc->initial_scale_value = 8 * dsc->rc_model_size /
>>                          (dsc->rc_model_size - dsc->initial_offset);
>> 
>> 
>> mux_word_size comes from the standard (must)
>> initial_scale_value calculation is recommended, but not required
>> nsl_bpg_offset follows the standard (must), also see below (*).
>> 
>> first_line_bpg_offset calculation differs between three drivers. The 
>> standard also provides a recommended formulas. I think we can leave it 
>> as is for now.
>> 
>> I think, that mux_word_size and nsl_bpg_offset calculation should be 
>> moved to drm_dsc_compute_rc_parameters(), while leaving 
>> initial_scale_value in place (in the driver code).
>> 
>> * I think nsl_bpg_offset is slightly incorrectly calculated. Standard 
>> demands that it is set to 'second_line_bpg_offset / (slice_height - 1), 
>> rounded up to 16 fraction bits', while SDE driver code sets it to the 
>> value rounded up to the next integer (having 16 fraction bits 
>> representation).
>> 
>> In my opinion correct calculation should be:
>> dsc->nsl_bpg_offset = DIV_ROUND_UP(2048 * dsc->second_line_bpg_offset,
>>                                  (dsc->slice_height - 1));
>> 
>> Could you please check, which one is correct according to the standard?
>> 
>> 
>
> Sure, i will check about nsl_bpg_offset. But sorry if I was not more 
> clear about this but sde_dsc_populate_dsc_config() is only one example 
> which from your analysis can be moved to the drm_dsc_helper() but not 
> the initial line calculation _dce_dsc_initial_line_calc(), 
> _dce_dsc_ich_reset_override_needed() , _dce_dsc_setup_helper().
>
> All of these are again common between DSI and DP.
>
> So in addition to thinking about what can be moved to the drm_dsc_helper 
> also think about what is specific to MSM but common to DSI and DP modules.
>
> That was the bigger picture I was trying to convey.
Abhinav Kumar Feb. 27, 2023, 5:59 p.m. UTC | #19
On 2/27/2023 4:45 AM, Dmitry Baryshkov wrote:
> On Mon, 27 Feb 2023 at 01:49, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 2/26/2023 5:09 AM, Dmitry Baryshkov wrote:
>>> On 26/02/2023 02:47, Abhinav Kumar wrote:
>>>> Hi Dmitry
>>>>
>>>> On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
>>>>> On 25/02/2023 02:36, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
>>>>>>> On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar
>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
>>>>>>>>> 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar
>>>>>>>>> <quic_abhinavk@quicinc.com> пишет:
>>>>>>>>>> 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).
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> No, it cannot. So each DSC encoder parameter is calculated based
>>>>>>>>>> on the HW core which is being used.
>>>>>>>>>>
>>>>>>>>>> They all get packed to the same DSC structure which is the
>>>>>>>>>> struct drm_dsc_config but the way the parameters are computed is
>>>>>>>>>> specific to the HW.
>>>>>>>>>>
>>>>>>>>>> This DPU file helper still uses the drm_dsc_helper's
>>>>>>>>>> drm_dsc_compute_rc_parameters() like all other vendors do but
>>>>>>>>>> the parameters themselves are very HW specific and belong to
>>>>>>>>>> each vendor's dir.
>>>>>>>>>>
>>>>>>>>>> This is not unique to MSM.
>>>>>>>>>>
>>>>>>>>>> Lets take a few other examples:
>>>>>>>>>>
>>>>>>>>>> AMD:
>>>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> i915:
>>>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I checked several values here. Intel driver defines more bpc/bpp
>>>>>>>>> combinations, but the ones which are defined in intel_vdsc and in
>>>>>>>>> this patch seem to match. If there are major differences there,
>>>>>>>>> please point me to the exact case.
>>>>>>>>>
>>>>>>>>> I remember that AMD driver might have different values.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Some values in the rc_params table do match. But the
>>>>>>>> rc_buf_thresh[] doesnt.
>>>>>>>
>>>>>>> Because later they do:
>>>>>>>
>>>>>>> vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
>>>>>>>
>>>>>>>>
>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
>>>>>>>>
>>>>>>>>
>>>>>>>> Vs
>>>>>>>>
>>>>>>>> +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
>>>>>>>> +};
>>>>>>>
>>>>>>> I'd prefer to have 896, 1792, etc. here, as those values come from the
>>>>>>> standard. As it's done in the Intel driver.
>>>>>>>
>>>>>>
>>>>>> Got it, thanks
>>>>>>
>>>>>>>> I dont know the AMD calculation very well to say that moving this
>>>>>>>> to the
>>>>>>>> helper is going to help.
>>>>>>>
>>>>>>> Those calculations correspond (more or less) at the first glance to
>>>>>>> what intel does for their newer generations. I think that's not our
>>>>>>> problem for now.
>>>>>>>
>>>>>>
>>>>>> Well, we have to figure out if each value matches and if each of
>>>>>> them come from the spec for us and i915 and from which section. So
>>>>>> it is unfortunately our problem.
>>>>>
>>>>> Otherwise it will have to be handled by Marijn, me or anybody else
>>>>> wanting to hack up the DSC code. Or by anybody adding DSC support to
>>>>> the next platform and having to figure out the difference between
>>>>> i915, msm and their platform.
>>>>>
>>>>
>>>> Yes, I wonder why the same doubt didn't arise when the other vendors
>>>> added their support both from other maintainers and others.
>>>>
>>>> Which makes me think that like I wrote in my previous response, these
>>>> are "recommended" values in the spec but its not mandatory.
>>>
>>> I think, it is because there were no other drivers to compare. In other
>>> words, for a first driver it is pretty logical to have everything
>>> handled on its own. As soon as we start getting other implementations of
>>> a feature, it becomes logical to think if the code can be generalized.
>>> This is what we see we with the HDCP series or with the code being moved
>>> to DP helpers.
>>>
>>
>> We were not the second, MSM was/is the third to add support for DSC afer
>> i915 and AMD. Thats what made me think why whoever was the second didnt
>> end up generalizing. Was it just missed out or was it intentionally left
>> in the vendor driver.
> 
> I didn't count AMD here, since it calculates some of the params rather
> than using the fixed ones from the model.
> 
>>
>>>>
>>>> Moving this to the drm_dsc_helper is generalizing the tables and not
>>>> giving room for the vendors to customize even if they want to (which
>>>> the spec does allow).
>>>
>>> That depends on the API you select. For example, in
>>> intel_dsc_compute_params() I see customization being applied to
>>> rc_buf_thresh in 6bpp case. I'd leave that to the i915 driver.
>>>
>>
>> Thanks for going through the i915 to figure out that the 6bpp is handled
>> in a customized way. So what you are saying is let the helper first fill
>> up the recommended values of the spec, whatever is changed from that let
>> the vendor driver override that.
>>
>> Thats where the case-by-case handling comes.
>>
>> Why not we do this way? Like you mentioned lets move these tables to the
>> drm_dsc_helper and let MSM driver first use those.
>>
>> Then in a separate patchset if i915 and AMD would like to move to that,
>> let them handle it for their respective drivers instead of MSM going
>> through whats customized for each calculation and doing it.
>>
>> I am hesitant to take up that effort.
> 
> Writing a tool to convert model's rc_Nbpc_Mbpp_foo.cfg into C
> languages structures used by Intel code took 15-20 minutes. Plugging
> generated structures took another 5 minutes. I will send the patches
> later today or tomorrow, as I find a time slot to clean them. Thank
> you for spending more time on arguing than it took me to generate &
> verify the data.
> 

Great, we will wait for your patches. We didnt intend to spend time on 
this at this point. We always wanted to take it up in a separate series 
of moving the tables.

You preferred not to wait. Upto you.

So thanks for doing it.

>>
>> If the recommended values work for the vendor, they can clean it up and
>> move to the drm_dsc_helper themselves and preserving their
>> customizations rather than one vendor doing it for all of them.
>>
>>> In case the driver needs to perform customization of the params, nothing
>>> stops it drop applying after filling all the RC params in the
>>> drm_dsc_config struct via the generic helper.
>>>
>>>
>>>> So if this has any merit and if you or Marijn would like to take it
>>>> up, go for it. We would do the same thing as either of you would have
>>>> to in terms of figuring out the difference between msm and the i915 code.
>>>>
>>>> This is not a generic API we are trying to put in a helper, these are
>>>> hard-coded tables so there is a difference between looking at these Vs
>>>> looking at some common code which can move to the core.
>>>>
>>>>>>
>>>>>>>>
>>>>>>>> Also, i think its too risky to change other drivers to use
>>>>>>>> whatever math
>>>>>>>> we put in the drm_dsc_helper to compute thr RC params because
>>>>>>>> their code
>>>>>>>> might be computing and using this tables differently.
>>>>>>>>
>>>>>>>> Its too much ownership for MSM developers to move this to
>>>>>>>> drm_dsc_helper
>>>>>>>> and own that as it might cause breakage of basic DSC even if some
>>>>>>>> values
>>>>>>>> are repeated.
>>>>>>>
>>>>>>> It's time to stop thinking about ownership and start thinking about
>>>>>>> shared code. We already have two instances of DSC tables. I don't
>>>>>>> think having a third instance, which is a subset of an existing
>>>>>>> dataset, would be beneficial to anybody.
>>>>>>> AMD has complicated code which supports half-bit bpp and calculates
>>>>>>> some of the parameters. But sharing data with the i915 driver is
>>>>>>> straightforward.
>>>>>>>
>>>>>>
>>>>>> Sorry, but I would like to get an ack from i915 folks if this is going
>>>>>> to be useful to them if we move this to helper because we have to
>>>>>> look at every table. Not just one.
>>>>>
>>>>> Added i915 maintainers to the CC list for them to be able to answer.
>>>>>
>>>>
>>>> Thanks, lets wait to hear from them about where finally these tables
>>>> should go but thats can be taken up as a separate effort too.
>>>>
>>>>>>
>>>>>> Also, this is just 1.1, we will add more tables for 1.2. So we will
>>>>>> have to end up changing both 1.1 and 1.2 tables as they are
>>>>>> different for QC.
>>>>>
>>>>> I haven't heard back from Kuogee about the possible causes of using
>>>>> rc/qp values from 1.2 even for 1.1 panels. Maybe you can comment on
>>>>> that? In other words, can we always stick to the values from 1.2
>>>>> standard? What will be the drawback?
>>>>>
>>>>> Otherwise, we'd have to have two different sets of values, like you
>>>>> do in your vendor driver.
>>>>>
>>>>
>>>> I have responded to this in the other email.
>>>>
>>>> All this being said, even if the rc tables move the drm_dsc_helper
>>>> either now or later on, we will still need MSM specific calculations
>>>> for many of the other encoder parameters (which are again either
>>>> hard-coded or calculated). Please refer to the
>>>> sde_dsc_populate_dsc_config() downstream. And yes, you will not find
>>>> those in the DP spec directly.
>>>>
>>>> So we will still need a dsc helper for MSM calculations to be common
>>>> for DSI / DP irrespective of where the tables go.
>>>>
>>>> So, lets finalize that first.
>>>
>>> I went on and trimmed sde_dsc_populate_dsc_config() to remove
>>> duplication with the drm_dsc_compute_rc_parameters() (which we already
>>> use for the MSM DSI DSC).
>>>
>>> Not much is left:
>>>
>>> dsc->first_line_bpg_offset set via the switch
>>>
>>> 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;
>>>
>>> if ((dsc->dsc_version_minor == 0x2) && (dsc->native_420))
>>>       dsc->nsl_bpg_offset = (2048 *
>>>                (DIV_ROUND_UP(dsc->second_line_bpg_offset,
>>>                                   (dsc->slice_height - 1))));
>>>
>>> dsc->initial_scale_value = 8 * dsc->rc_model_size /
>>>                           (dsc->rc_model_size - dsc->initial_offset);
>>>
>>>
>>> mux_word_size comes from the standard (must)
>>> initial_scale_value calculation is recommended, but not required
>>> nsl_bpg_offset follows the standard (must), also see below (*).
>>>
>>> first_line_bpg_offset calculation differs between three drivers. The
>>> standard also provides a recommended formulas. I think we can leave it
>>> as is for now.
>>>
>>> I think, that mux_word_size and nsl_bpg_offset calculation should be
>>> moved to drm_dsc_compute_rc_parameters(), while leaving
>>> initial_scale_value in place (in the driver code).
>>>
>>> * I think nsl_bpg_offset is slightly incorrectly calculated. Standard
>>> demands that it is set to 'second_line_bpg_offset / (slice_height - 1),
>>> rounded up to 16 fraction bits', while SDE driver code sets it to the
>>> value rounded up to the next integer (having 16 fraction bits
>>> representation).
>>>
>>> In my opinion correct calculation should be:
>>> dsc->nsl_bpg_offset = DIV_ROUND_UP(2048 * dsc->second_line_bpg_offset,
>>>                                   (dsc->slice_height - 1));
>>>
>>> Could you please check, which one is correct according to the standard?
>>>
>>>
>>
>> Sure, i will check about nsl_bpg_offset. But sorry if I was not more
>> clear about this but sde_dsc_populate_dsc_config() is only one example
>> which from your analysis can be moved to the drm_dsc_helper() but not
>> the initial line calculation _dce_dsc_initial_line_calc(),
>> _dce_dsc_ich_reset_override_needed() , _dce_dsc_setup_helper().
> 
> The initial_line is already calculated in dpu_encoder.c. As for the
> _dce_dsc_ich_reset_override_needed(), I don't think we support partial
> updates in the upstream driver.
> 
>>
>> All of these are again common between DSI and DP.
>>
>> So in addition to thinking about what can be moved to the drm_dsc_helper
>> also think about what is specific to MSM but common to DSI and DP modules.
>>
>> That was the bigger picture I was trying to convey.
> 

_dce_dsc_initial_line_calc which will get expanded with v1.2 gets added 
has much more than whats there in upstream today.

Dumping everything in dpu_encoder is not the solution. Sorry.

> 
>
Dmitry Baryshkov Feb. 27, 2023, 7:25 p.m. UTC | #20
27 февраля 2023 г. 19:59:35 GMT+02:00, Abhinav Kumar <quic_abhinavk@quicinc.com> пишет:
>
>
>On 2/27/2023 4:45 AM, Dmitry Baryshkov wrote:
>> On Mon, 27 Feb 2023 at 01:49, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>> 
>>> 
>>> 
>>> On 2/26/2023 5:09 AM, Dmitry Baryshkov wrote:
>>>> On 26/02/2023 02:47, Abhinav Kumar wrote:
>>>>> Hi Dmitry
>>>>> 
>>>>> On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
>>>>>> On 25/02/2023 02:36, Abhinav Kumar wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar
>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
>>>>>>>>>> 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar
>>>>>>>>>> <quic_abhinavk@quicinc.com> пишет:
>>>>>>>>>>> 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).
>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> No, it cannot. So each DSC encoder parameter is calculated based
>>>>>>>>>>> on the HW core which is being used.
>>>>>>>>>>> 
>>>>>>>>>>> They all get packed to the same DSC structure which is the
>>>>>>>>>>> struct drm_dsc_config but the way the parameters are computed is
>>>>>>>>>>> specific to the HW.
>>>>>>>>>>> 
>>>>>>>>>>> This DPU file helper still uses the drm_dsc_helper's
>>>>>>>>>>> drm_dsc_compute_rc_parameters() like all other vendors do but
>>>>>>>>>>> the parameters themselves are very HW specific and belong to
>>>>>>>>>>> each vendor's dir.
>>>>>>>>>>> 
>>>>>>>>>>> This is not unique to MSM.
>>>>>>>>>>> 
>>>>>>>>>>> Lets take a few other examples:
>>>>>>>>>>> 
>>>>>>>>>>> AMD:
>>>>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> i915:
>>>>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
>>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> I checked several values here. Intel driver defines more bpc/bpp
>>>>>>>>>> combinations, but the ones which are defined in intel_vdsc and in
>>>>>>>>>> this patch seem to match. If there are major differences there,
>>>>>>>>>> please point me to the exact case.
>>>>>>>>>> 
>>>>>>>>>> I remember that AMD driver might have different values.
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Some values in the rc_params table do match. But the
>>>>>>>>> rc_buf_thresh[] doesnt.
>>>>>>>> 
>>>>>>>> Because later they do:
>>>>>>>> 
>>>>>>>> vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Vs
>>>>>>>>> 
>>>>>>>>> +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
>>>>>>>>> +};
>>>>>>>> 
>>>>>>>> I'd prefer to have 896, 1792, etc. here, as those values come from the
>>>>>>>> standard. As it's done in the Intel driver.
>>>>>>>> 
>>>>>>> 
>>>>>>> Got it, thanks
>>>>>>> 
>>>>>>>>> I dont know the AMD calculation very well to say that moving this
>>>>>>>>> to the
>>>>>>>>> helper is going to help.
>>>>>>>> 
>>>>>>>> Those calculations correspond (more or less) at the first glance to
>>>>>>>> what intel does for their newer generations. I think that's not our
>>>>>>>> problem for now.
>>>>>>>> 
>>>>>>> 
>>>>>>> Well, we have to figure out if each value matches and if each of
>>>>>>> them come from the spec for us and i915 and from which section. So
>>>>>>> it is unfortunately our problem.
>>>>>> 
>>>>>> Otherwise it will have to be handled by Marijn, me or anybody else
>>>>>> wanting to hack up the DSC code. Or by anybody adding DSC support to
>>>>>> the next platform and having to figure out the difference between
>>>>>> i915, msm and their platform.
>>>>>> 
>>>>> 
>>>>> Yes, I wonder why the same doubt didn't arise when the other vendors
>>>>> added their support both from other maintainers and others.
>>>>> 
>>>>> Which makes me think that like I wrote in my previous response, these
>>>>> are "recommended" values in the spec but its not mandatory.
>>>> 
>>>> I think, it is because there were no other drivers to compare. In other
>>>> words, for a first driver it is pretty logical to have everything
>>>> handled on its own. As soon as we start getting other implementations of
>>>> a feature, it becomes logical to think if the code can be generalized.
>>>> This is what we see we with the HDCP series or with the code being moved
>>>> to DP helpers.
>>>> 
>>> 
>>> We were not the second, MSM was/is the third to add support for DSC afer
>>> i915 and AMD. Thats what made me think why whoever was the second didnt
>>> end up generalizing. Was it just missed out or was it intentionally left
>>> in the vendor driver.
>> 
>> I didn't count AMD here, since it calculates some of the params rather
>> than using the fixed ones from the model.
>> 
>>> 
>>>>> 
>>>>> Moving this to the drm_dsc_helper is generalizing the tables and not
>>>>> giving room for the vendors to customize even if they want to (which
>>>>> the spec does allow).
>>>> 
>>>> That depends on the API you select. For example, in
>>>> intel_dsc_compute_params() I see customization being applied to
>>>> rc_buf_thresh in 6bpp case. I'd leave that to the i915 driver.
>>>> 
>>> 
>>> Thanks for going through the i915 to figure out that the 6bpp is handled
>>> in a customized way. So what you are saying is let the helper first fill
>>> up the recommended values of the spec, whatever is changed from that let
>>> the vendor driver override that.
>>> 
>>> Thats where the case-by-case handling comes.
>>> 
>>> Why not we do this way? Like you mentioned lets move these tables to the
>>> drm_dsc_helper and let MSM driver first use those.
>>> 
>>> Then in a separate patchset if i915 and AMD would like to move to that,
>>> let them handle it for their respective drivers instead of MSM going
>>> through whats customized for each calculation and doing it.
>>> 
>>> I am hesitant to take up that effort.
>> 
>> Writing a tool to convert model's rc_Nbpc_Mbpp_foo.cfg into C
>> languages structures used by Intel code took 15-20 minutes. Plugging
>> generated structures took another 5 minutes. I will send the patches
>> later today or tomorrow, as I find a time slot to clean them. Thank
>> you for spending more time on arguing than it took me to generate &
>> verify the data.
>> 
>
>Great, we will wait for your patches. We didnt intend to spend time on this at this point. We always wanted to take it up in a separate series of moving the tables.

Getting rid of msm_display_dsc_config and then making use of drm_dsc_compute_rc_parameters() was bad enough. So, let's get things done in a good way now, rather than at some random point later.


>
>You preferred not to wait. Upto you.
>
>So thanks for doing it.
>
>>> 
>>> If the recommended values work for the vendor, they can clean it up and
>>> move to the drm_dsc_helper themselves and preserving their
>>> customizations rather than one vendor doing it for all of them.
>>> 
>>>> In case the driver needs to perform customization of the params, nothing
>>>> stops it drop applying after filling all the RC params in the
>>>> drm_dsc_config struct via the generic helper.
>>>> 
>>>> 
>>>>> So if this has any merit and if you or Marijn would like to take it
>>>>> up, go for it. We would do the same thing as either of you would have
>>>>> to in terms of figuring out the difference between msm and the i915 code.
>>>>> 
>>>>> This is not a generic API we are trying to put in a helper, these are
>>>>> hard-coded tables so there is a difference between looking at these Vs
>>>>> looking at some common code which can move to the core.
>>>>> 
>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Also, i think its too risky to change other drivers to use
>>>>>>>>> whatever math
>>>>>>>>> we put in the drm_dsc_helper to compute thr RC params because
>>>>>>>>> their code
>>>>>>>>> might be computing and using this tables differently.
>>>>>>>>> 
>>>>>>>>> Its too much ownership for MSM developers to move this to
>>>>>>>>> drm_dsc_helper
>>>>>>>>> and own that as it might cause breakage of basic DSC even if some
>>>>>>>>> values
>>>>>>>>> are repeated.
>>>>>>>> 
>>>>>>>> It's time to stop thinking about ownership and start thinking about
>>>>>>>> shared code. We already have two instances of DSC tables. I don't
>>>>>>>> think having a third instance, which is a subset of an existing
>>>>>>>> dataset, would be beneficial to anybody.
>>>>>>>> AMD has complicated code which supports half-bit bpp and calculates
>>>>>>>> some of the parameters. But sharing data with the i915 driver is
>>>>>>>> straightforward.
>>>>>>>> 
>>>>>>> 
>>>>>>> Sorry, but I would like to get an ack from i915 folks if this is going
>>>>>>> to be useful to them if we move this to helper because we have to
>>>>>>> look at every table. Not just one.
>>>>>> 
>>>>>> Added i915 maintainers to the CC list for them to be able to answer.
>>>>>> 
>>>>> 
>>>>> Thanks, lets wait to hear from them about where finally these tables
>>>>> should go but thats can be taken up as a separate effort too.
>>>>> 
>>>>>>> 
>>>>>>> Also, this is just 1.1, we will add more tables for 1.2. So we will
>>>>>>> have to end up changing both 1.1 and 1.2 tables as they are
>>>>>>> different for QC.
>>>>>> 
>>>>>> I haven't heard back from Kuogee about the possible causes of using
>>>>>> rc/qp values from 1.2 even for 1.1 panels. Maybe you can comment on
>>>>>> that? In other words, can we always stick to the values from 1.2
>>>>>> standard? What will be the drawback?
>>>>>> 
>>>>>> Otherwise, we'd have to have two different sets of values, like you
>>>>>> do in your vendor driver.
>>>>>> 
>>>>> 
>>>>> I have responded to this in the other email.
>>>>> 
>>>>> All this being said, even if the rc tables move the drm_dsc_helper
>>>>> either now or later on, we will still need MSM specific calculations
>>>>> for many of the other encoder parameters (which are again either
>>>>> hard-coded or calculated). Please refer to the
>>>>> sde_dsc_populate_dsc_config() downstream. And yes, you will not find
>>>>> those in the DP spec directly.
>>>>> 
>>>>> So we will still need a dsc helper for MSM calculations to be common
>>>>> for DSI / DP irrespective of where the tables go.
>>>>> 
>>>>> So, lets finalize that first.
>>>> 
>>>> I went on and trimmed sde_dsc_populate_dsc_config() to remove
>>>> duplication with the drm_dsc_compute_rc_parameters() (which we already
>>>> use for the MSM DSI DSC).
>>>> 
>>>> Not much is left:
>>>> 
>>>> dsc->first_line_bpg_offset set via the switch
>>>> 
>>>> 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;
>>>> 
>>>> if ((dsc->dsc_version_minor == 0x2) && (dsc->native_420))
>>>>       dsc->nsl_bpg_offset = (2048 *
>>>>                (DIV_ROUND_UP(dsc->second_line_bpg_offset,
>>>>                                   (dsc->slice_height - 1))));
>>>> 
>>>> dsc->initial_scale_value = 8 * dsc->rc_model_size /
>>>>                           (dsc->rc_model_size - dsc->initial_offset);
>>>> 
>>>> 
>>>> mux_word_size comes from the standard (must)
>>>> initial_scale_value calculation is recommended, but not required
>>>> nsl_bpg_offset follows the standard (must), also see below (*).
>>>> 
>>>> first_line_bpg_offset calculation differs between three drivers. The
>>>> standard also provides a recommended formulas. I think we can leave it
>>>> as is for now.
>>>> 
>>>> I think, that mux_word_size and nsl_bpg_offset calculation should be
>>>> moved to drm_dsc_compute_rc_parameters(), while leaving
>>>> initial_scale_value in place (in the driver code).
>>>> 
>>>> * I think nsl_bpg_offset is slightly incorrectly calculated. Standard
>>>> demands that it is set to 'second_line_bpg_offset / (slice_height - 1),
>>>> rounded up to 16 fraction bits', while SDE driver code sets it to the
>>>> value rounded up to the next integer (having 16 fraction bits
>>>> representation).
>>>> 
>>>> In my opinion correct calculation should be:
>>>> dsc->nsl_bpg_offset = DIV_ROUND_UP(2048 * dsc->second_line_bpg_offset,
>>>>                                   (dsc->slice_height - 1));
>>>> 
>>>> Could you please check, which one is correct according to the standard?
>>>> 
>>>> 
>>> 
>>> Sure, i will check about nsl_bpg_offset. But sorry if I was not more
>>> clear about this but sde_dsc_populate_dsc_config() is only one example
>>> which from your analysis can be moved to the drm_dsc_helper() but not
>>> the initial line calculation _dce_dsc_initial_line_calc(),
>>> _dce_dsc_ich_reset_override_needed() , _dce_dsc_setup_helper().
>> 
>> The initial_line is already calculated in dpu_encoder.c. As for the
>> _dce_dsc_ich_reset_override_needed(), I don't think we support partial
>> updates in the upstream driver.
>> 
>>> 
>>> All of these are again common between DSI and DP.
>>> 
>>> So in addition to thinking about what can be moved to the drm_dsc_helper
>>> also think about what is specific to MSM but common to DSI and DP modules.
>>> 
>>> That was the bigger picture I was trying to convey.
>> 
>
>_dce_dsc_initial_line_calc which will get expanded with v1.2 gets added has much more than whats there in upstream today.
>
>Dumping everything in dpu_encoder is not the solution. Sorry.

But it is still the DPU thing. So, no problems.

>
>> 
>>
Abhinav Kumar Feb. 27, 2023, 9:24 p.m. UTC | #21
On 2/27/2023 11:25 AM, Dmitry Baryshkov wrote:
> 27 февраля 2023 г. 19:59:35 GMT+02:00, Abhinav Kumar <quic_abhinavk@quicinc.com> пишет:
>>
>>
>> On 2/27/2023 4:45 AM, Dmitry Baryshkov wrote:
>>> On Mon, 27 Feb 2023 at 01:49, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2/26/2023 5:09 AM, Dmitry Baryshkov wrote:
>>>>> On 26/02/2023 02:47, Abhinav Kumar wrote:
>>>>>> Hi Dmitry
>>>>>>
>>>>>> On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
>>>>>>> On 25/02/2023 02:36, Abhinav Kumar wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar
>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>> 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar
>>>>>>>>>>> <quic_abhinavk@quicinc.com> пишет:
>>>>>>>>>>>> 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).
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> No, it cannot. So each DSC encoder parameter is calculated based
>>>>>>>>>>>> on the HW core which is being used.
>>>>>>>>>>>>
>>>>>>>>>>>> They all get packed to the same DSC structure which is the
>>>>>>>>>>>> struct drm_dsc_config but the way the parameters are computed is
>>>>>>>>>>>> specific to the HW.
>>>>>>>>>>>>
>>>>>>>>>>>> This DPU file helper still uses the drm_dsc_helper's
>>>>>>>>>>>> drm_dsc_compute_rc_parameters() like all other vendors do but
>>>>>>>>>>>> the parameters themselves are very HW specific and belong to
>>>>>>>>>>>> each vendor's dir.
>>>>>>>>>>>>
>>>>>>>>>>>> This is not unique to MSM.
>>>>>>>>>>>>
>>>>>>>>>>>> Lets take a few other examples:
>>>>>>>>>>>>
>>>>>>>>>>>> AMD:
>>>>>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> i915:
>>>>>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I checked several values here. Intel driver defines more bpc/bpp
>>>>>>>>>>> combinations, but the ones which are defined in intel_vdsc and in
>>>>>>>>>>> this patch seem to match. If there are major differences there,
>>>>>>>>>>> please point me to the exact case.
>>>>>>>>>>>
>>>>>>>>>>> I remember that AMD driver might have different values.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Some values in the rc_params table do match. But the
>>>>>>>>>> rc_buf_thresh[] doesnt.
>>>>>>>>>
>>>>>>>>> Because later they do:
>>>>>>>>>
>>>>>>>>> vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Vs
>>>>>>>>>>
>>>>>>>>>> +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
>>>>>>>>>> +};
>>>>>>>>>
>>>>>>>>> I'd prefer to have 896, 1792, etc. here, as those values come from the
>>>>>>>>> standard. As it's done in the Intel driver.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Got it, thanks
>>>>>>>>
>>>>>>>>>> I dont know the AMD calculation very well to say that moving this
>>>>>>>>>> to the
>>>>>>>>>> helper is going to help.
>>>>>>>>>
>>>>>>>>> Those calculations correspond (more or less) at the first glance to
>>>>>>>>> what intel does for their newer generations. I think that's not our
>>>>>>>>> problem for now.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Well, we have to figure out if each value matches and if each of
>>>>>>>> them come from the spec for us and i915 and from which section. So
>>>>>>>> it is unfortunately our problem.
>>>>>>>
>>>>>>> Otherwise it will have to be handled by Marijn, me or anybody else
>>>>>>> wanting to hack up the DSC code. Or by anybody adding DSC support to
>>>>>>> the next platform and having to figure out the difference between
>>>>>>> i915, msm and their platform.
>>>>>>>
>>>>>>
>>>>>> Yes, I wonder why the same doubt didn't arise when the other vendors
>>>>>> added their support both from other maintainers and others.
>>>>>>
>>>>>> Which makes me think that like I wrote in my previous response, these
>>>>>> are "recommended" values in the spec but its not mandatory.
>>>>>
>>>>> I think, it is because there were no other drivers to compare. In other
>>>>> words, for a first driver it is pretty logical to have everything
>>>>> handled on its own. As soon as we start getting other implementations of
>>>>> a feature, it becomes logical to think if the code can be generalized.
>>>>> This is what we see we with the HDCP series or with the code being moved
>>>>> to DP helpers.
>>>>>
>>>>
>>>> We were not the second, MSM was/is the third to add support for DSC afer
>>>> i915 and AMD. Thats what made me think why whoever was the second didnt
>>>> end up generalizing. Was it just missed out or was it intentionally left
>>>> in the vendor driver.
>>>
>>> I didn't count AMD here, since it calculates some of the params rather
>>> than using the fixed ones from the model.
>>>
>>>>
>>>>>>
>>>>>> Moving this to the drm_dsc_helper is generalizing the tables and not
>>>>>> giving room for the vendors to customize even if they want to (which
>>>>>> the spec does allow).
>>>>>
>>>>> That depends on the API you select. For example, in
>>>>> intel_dsc_compute_params() I see customization being applied to
>>>>> rc_buf_thresh in 6bpp case. I'd leave that to the i915 driver.
>>>>>
>>>>
>>>> Thanks for going through the i915 to figure out that the 6bpp is handled
>>>> in a customized way. So what you are saying is let the helper first fill
>>>> up the recommended values of the spec, whatever is changed from that let
>>>> the vendor driver override that.
>>>>
>>>> Thats where the case-by-case handling comes.
>>>>
>>>> Why not we do this way? Like you mentioned lets move these tables to the
>>>> drm_dsc_helper and let MSM driver first use those.
>>>>
>>>> Then in a separate patchset if i915 and AMD would like to move to that,
>>>> let them handle it for their respective drivers instead of MSM going
>>>> through whats customized for each calculation and doing it.
>>>>
>>>> I am hesitant to take up that effort.
>>>
>>> Writing a tool to convert model's rc_Nbpc_Mbpp_foo.cfg into C
>>> languages structures used by Intel code took 15-20 minutes. Plugging
>>> generated structures took another 5 minutes. I will send the patches
>>> later today or tomorrow, as I find a time slot to clean them. Thank
>>> you for spending more time on arguing than it took me to generate &
>>> verify the data.
>>>
>>
>> Great, we will wait for your patches. We didnt intend to spend time on this at this point. We always wanted to take it up in a separate series of moving the tables.
> 
> Getting rid of msm_display_dsc_config and then making use of drm_dsc_compute_rc_parameters() was bad enough. So, let's get things done in a good way now, rather than at some random point later.
> 

Alright, we will wait for your change then :)

> 
>>
>> You preferred not to wait. Upto you.
>>
>> So thanks for doing it.
>>
>>>>
>>>> If the recommended values work for the vendor, they can clean it up and
>>>> move to the drm_dsc_helper themselves and preserving their
>>>> customizations rather than one vendor doing it for all of them.
>>>>
>>>>> In case the driver needs to perform customization of the params, nothing
>>>>> stops it drop applying after filling all the RC params in the
>>>>> drm_dsc_config struct via the generic helper.
>>>>>
>>>>>
>>>>>> So if this has any merit and if you or Marijn would like to take it
>>>>>> up, go for it. We would do the same thing as either of you would have
>>>>>> to in terms of figuring out the difference between msm and the i915 code.
>>>>>>
>>>>>> This is not a generic API we are trying to put in a helper, these are
>>>>>> hard-coded tables so there is a difference between looking at these Vs
>>>>>> looking at some common code which can move to the core.
>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Also, i think its too risky to change other drivers to use
>>>>>>>>>> whatever math
>>>>>>>>>> we put in the drm_dsc_helper to compute thr RC params because
>>>>>>>>>> their code
>>>>>>>>>> might be computing and using this tables differently.
>>>>>>>>>>
>>>>>>>>>> Its too much ownership for MSM developers to move this to
>>>>>>>>>> drm_dsc_helper
>>>>>>>>>> and own that as it might cause breakage of basic DSC even if some
>>>>>>>>>> values
>>>>>>>>>> are repeated.
>>>>>>>>>
>>>>>>>>> It's time to stop thinking about ownership and start thinking about
>>>>>>>>> shared code. We already have two instances of DSC tables. I don't
>>>>>>>>> think having a third instance, which is a subset of an existing
>>>>>>>>> dataset, would be beneficial to anybody.
>>>>>>>>> AMD has complicated code which supports half-bit bpp and calculates
>>>>>>>>> some of the parameters. But sharing data with the i915 driver is
>>>>>>>>> straightforward.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Sorry, but I would like to get an ack from i915 folks if this is going
>>>>>>>> to be useful to them if we move this to helper because we have to
>>>>>>>> look at every table. Not just one.
>>>>>>>
>>>>>>> Added i915 maintainers to the CC list for them to be able to answer.
>>>>>>>
>>>>>>
>>>>>> Thanks, lets wait to hear from them about where finally these tables
>>>>>> should go but thats can be taken up as a separate effort too.
>>>>>>
>>>>>>>>
>>>>>>>> Also, this is just 1.1, we will add more tables for 1.2. So we will
>>>>>>>> have to end up changing both 1.1 and 1.2 tables as they are
>>>>>>>> different for QC.
>>>>>>>
>>>>>>> I haven't heard back from Kuogee about the possible causes of using
>>>>>>> rc/qp values from 1.2 even for 1.1 panels. Maybe you can comment on
>>>>>>> that? In other words, can we always stick to the values from 1.2
>>>>>>> standard? What will be the drawback?
>>>>>>>
>>>>>>> Otherwise, we'd have to have two different sets of values, like you
>>>>>>> do in your vendor driver.
>>>>>>>
>>>>>>
>>>>>> I have responded to this in the other email.
>>>>>>
>>>>>> All this being said, even if the rc tables move the drm_dsc_helper
>>>>>> either now or later on, we will still need MSM specific calculations
>>>>>> for many of the other encoder parameters (which are again either
>>>>>> hard-coded or calculated). Please refer to the
>>>>>> sde_dsc_populate_dsc_config() downstream. And yes, you will not find
>>>>>> those in the DP spec directly.
>>>>>>
>>>>>> So we will still need a dsc helper for MSM calculations to be common
>>>>>> for DSI / DP irrespective of where the tables go.
>>>>>>
>>>>>> So, lets finalize that first.
>>>>>
>>>>> I went on and trimmed sde_dsc_populate_dsc_config() to remove
>>>>> duplication with the drm_dsc_compute_rc_parameters() (which we already
>>>>> use for the MSM DSI DSC).
>>>>>
>>>>> Not much is left:
>>>>>
>>>>> dsc->first_line_bpg_offset set via the switch
>>>>>
>>>>> 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;
>>>>>
>>>>> if ((dsc->dsc_version_minor == 0x2) && (dsc->native_420))
>>>>>        dsc->nsl_bpg_offset = (2048 *
>>>>>                 (DIV_ROUND_UP(dsc->second_line_bpg_offset,
>>>>>                                    (dsc->slice_height - 1))));
>>>>>
>>>>> dsc->initial_scale_value = 8 * dsc->rc_model_size /
>>>>>                            (dsc->rc_model_size - dsc->initial_offset);
>>>>>
>>>>>
>>>>> mux_word_size comes from the standard (must)
>>>>> initial_scale_value calculation is recommended, but not required
>>>>> nsl_bpg_offset follows the standard (must), also see below (*).
>>>>>
>>>>> first_line_bpg_offset calculation differs between three drivers. The
>>>>> standard also provides a recommended formulas. I think we can leave it
>>>>> as is for now.
>>>>>
>>>>> I think, that mux_word_size and nsl_bpg_offset calculation should be
>>>>> moved to drm_dsc_compute_rc_parameters(), while leaving
>>>>> initial_scale_value in place (in the driver code).
>>>>>
>>>>> * I think nsl_bpg_offset is slightly incorrectly calculated. Standard
>>>>> demands that it is set to 'second_line_bpg_offset / (slice_height - 1),
>>>>> rounded up to 16 fraction bits', while SDE driver code sets it to the
>>>>> value rounded up to the next integer (having 16 fraction bits
>>>>> representation).
>>>>>
>>>>> In my opinion correct calculation should be:
>>>>> dsc->nsl_bpg_offset = DIV_ROUND_UP(2048 * dsc->second_line_bpg_offset,
>>>>>                                    (dsc->slice_height - 1));
>>>>>
>>>>> Could you please check, which one is correct according to the standard?
>>>>>
>>>>>
>>>>
>>>> Sure, i will check about nsl_bpg_offset. But sorry if I was not more
>>>> clear about this but sde_dsc_populate_dsc_config() is only one example
>>>> which from your analysis can be moved to the drm_dsc_helper() but not
>>>> the initial line calculation _dce_dsc_initial_line_calc(),
>>>> _dce_dsc_ich_reset_override_needed() , _dce_dsc_setup_helper().
>>>
>>> The initial_line is already calculated in dpu_encoder.c. As for the
>>> _dce_dsc_ich_reset_override_needed(), I don't think we support partial
>>> updates in the upstream driver.
>>>
>>>>
>>>> All of these are again common between DSI and DP.
>>>>
>>>> So in addition to thinking about what can be moved to the drm_dsc_helper
>>>> also think about what is specific to MSM but common to DSI and DP modules.
>>>>
>>>> That was the bigger picture I was trying to convey.
>>>
>>
>> _dce_dsc_initial_line_calc which will get expanded with v1.2 gets added has much more than whats there in upstream today.
>>
>> Dumping everything in dpu_encoder is not the solution. Sorry.
> 
> But it is still the DPU thing. So, no problems.
> 

I am not fully convinced. We will wait for your post, then see how the 
code looks.

>>
>>>
>>>
>
Jessica Zhang March 16, 2023, 1:28 a.m. UTC | #22
On 2/27/2023 1:24 PM, Abhinav Kumar wrote:
> 
> 
> On 2/27/2023 11:25 AM, Dmitry Baryshkov wrote:
>> 27 февраля 2023 г. 19:59:35 GMT+02:00, Abhinav Kumar 
>> <quic_abhinavk@quicinc.com> пишет:
>>>
>>>
>>> On 2/27/2023 4:45 AM, Dmitry Baryshkov wrote:
>>>> On Mon, 27 Feb 2023 at 01:49, Abhinav Kumar 
>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2/26/2023 5:09 AM, Dmitry Baryshkov wrote:
>>>>>> On 26/02/2023 02:47, Abhinav Kumar wrote:
>>>>>>> Hi Dmitry
>>>>>>>
>>>>>>> On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
>>>>>>>> On 25/02/2023 02:36, Abhinav Kumar wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar
>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>>> 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar
>>>>>>>>>>>> <quic_abhinavk@quicinc.com> пишет:
>>>>>>>>>>>>> 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).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> No, it cannot. So each DSC encoder parameter is calculated 
>>>>>>>>>>>>> based
>>>>>>>>>>>>> on the HW core which is being used.
>>>>>>>>>>>>>
>>>>>>>>>>>>> They all get packed to the same DSC structure which is the
>>>>>>>>>>>>> struct drm_dsc_config but the way the parameters are 
>>>>>>>>>>>>> computed is
>>>>>>>>>>>>> specific to the HW.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This DPU file helper still uses the drm_dsc_helper's
>>>>>>>>>>>>> drm_dsc_compute_rc_parameters() like all other vendors do but
>>>>>>>>>>>>> the parameters themselves are very HW specific and belong to
>>>>>>>>>>>>> each vendor's dir.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This is not unique to MSM.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Lets take a few other examples:
>>>>>>>>>>>>>
>>>>>>>>>>>>> AMD:
>>>>>>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> i915:
>>>>>>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I checked several values here. Intel driver defines more 
>>>>>>>>>>>> bpc/bpp
>>>>>>>>>>>> combinations, but the ones which are defined in intel_vdsc 
>>>>>>>>>>>> and in
>>>>>>>>>>>> this patch seem to match. If there are major differences there,
>>>>>>>>>>>> please point me to the exact case.
>>>>>>>>>>>>
>>>>>>>>>>>> I remember that AMD driver might have different values.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Some values in the rc_params table do match. But the
>>>>>>>>>>> rc_buf_thresh[] doesnt.
>>>>>>>>>>
>>>>>>>>>> Because later they do:
>>>>>>>>>>
>>>>>>>>>> vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Vs
>>>>>>>>>>>
>>>>>>>>>>> +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
>>>>>>>>>>> +};
>>>>>>>>>>
>>>>>>>>>> I'd prefer to have 896, 1792, etc. here, as those values come 
>>>>>>>>>> from the
>>>>>>>>>> standard. As it's done in the Intel driver.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Got it, thanks
>>>>>>>>>
>>>>>>>>>>> I dont know the AMD calculation very well to say that moving 
>>>>>>>>>>> this
>>>>>>>>>>> to the
>>>>>>>>>>> helper is going to help.
>>>>>>>>>>
>>>>>>>>>> Those calculations correspond (more or less) at the first 
>>>>>>>>>> glance to
>>>>>>>>>> what intel does for their newer generations. I think that's 
>>>>>>>>>> not our
>>>>>>>>>> problem for now.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Well, we have to figure out if each value matches and if each of
>>>>>>>>> them come from the spec for us and i915 and from which section. So
>>>>>>>>> it is unfortunately our problem.
>>>>>>>>
>>>>>>>> Otherwise it will have to be handled by Marijn, me or anybody else
>>>>>>>> wanting to hack up the DSC code. Or by anybody adding DSC 
>>>>>>>> support to
>>>>>>>> the next platform and having to figure out the difference between
>>>>>>>> i915, msm and their platform.
>>>>>>>>
>>>>>>>
>>>>>>> Yes, I wonder why the same doubt didn't arise when the other vendors
>>>>>>> added their support both from other maintainers and others.
>>>>>>>
>>>>>>> Which makes me think that like I wrote in my previous response, 
>>>>>>> these
>>>>>>> are "recommended" values in the spec but its not mandatory.
>>>>>>
>>>>>> I think, it is because there were no other drivers to compare. In 
>>>>>> other
>>>>>> words, for a first driver it is pretty logical to have everything
>>>>>> handled on its own. As soon as we start getting other 
>>>>>> implementations of
>>>>>> a feature, it becomes logical to think if the code can be 
>>>>>> generalized.
>>>>>> This is what we see we with the HDCP series or with the code being 
>>>>>> moved
>>>>>> to DP helpers.
>>>>>>
>>>>>
>>>>> We were not the second, MSM was/is the third to add support for DSC 
>>>>> afer
>>>>> i915 and AMD. Thats what made me think why whoever was the second 
>>>>> didnt
>>>>> end up generalizing. Was it just missed out or was it intentionally 
>>>>> left
>>>>> in the vendor driver.
>>>>
>>>> I didn't count AMD here, since it calculates some of the params rather
>>>> than using the fixed ones from the model.
>>>>
>>>>>
>>>>>>>
>>>>>>> Moving this to the drm_dsc_helper is generalizing the tables and not
>>>>>>> giving room for the vendors to customize even if they want to (which
>>>>>>> the spec does allow).
>>>>>>
>>>>>> That depends on the API you select. For example, in
>>>>>> intel_dsc_compute_params() I see customization being applied to
>>>>>> rc_buf_thresh in 6bpp case. I'd leave that to the i915 driver.
>>>>>>
>>>>>
>>>>> Thanks for going through the i915 to figure out that the 6bpp is 
>>>>> handled
>>>>> in a customized way. So what you are saying is let the helper first 
>>>>> fill
>>>>> up the recommended values of the spec, whatever is changed from 
>>>>> that let
>>>>> the vendor driver override that.
>>>>>
>>>>> Thats where the case-by-case handling comes.
>>>>>
>>>>> Why not we do this way? Like you mentioned lets move these tables 
>>>>> to the
>>>>> drm_dsc_helper and let MSM driver first use those.
>>>>>
>>>>> Then in a separate patchset if i915 and AMD would like to move to 
>>>>> that,
>>>>> let them handle it for their respective drivers instead of MSM going
>>>>> through whats customized for each calculation and doing it.
>>>>>
>>>>> I am hesitant to take up that effort.
>>>>
>>>> Writing a tool to convert model's rc_Nbpc_Mbpp_foo.cfg into C
>>>> languages structures used by Intel code took 15-20 minutes. Plugging
>>>> generated structures took another 5 minutes. I will send the patches
>>>> later today or tomorrow, as I find a time slot to clean them. Thank
>>>> you for spending more time on arguing than it took me to generate &
>>>> verify the data.
>>>>
>>>
>>> Great, we will wait for your patches. We didnt intend to spend time 
>>> on this at this point. We always wanted to take it up in a separate 
>>> series of moving the tables.
>>
>> Getting rid of msm_display_dsc_config and then making use of 
>> drm_dsc_compute_rc_parameters() was bad enough. So, let's get things 
>> done in a good way now, rather than at some random point later.
>>
> 
> Alright, we will wait for your change then :)
> 
>>
>>>
>>> You preferred not to wait. Upto you.
>>>
>>> So thanks for doing it.
>>>
>>>>>
>>>>> If the recommended values work for the vendor, they can clean it up 
>>>>> and
>>>>> move to the drm_dsc_helper themselves and preserving their
>>>>> customizations rather than one vendor doing it for all of them.
>>>>>
>>>>>> In case the driver needs to perform customization of the params, 
>>>>>> nothing
>>>>>> stops it drop applying after filling all the RC params in the
>>>>>> drm_dsc_config struct via the generic helper.
>>>>>>
>>>>>>
>>>>>>> So if this has any merit and if you or Marijn would like to take it
>>>>>>> up, go for it. We would do the same thing as either of you would 
>>>>>>> have
>>>>>>> to in terms of figuring out the difference between msm and the 
>>>>>>> i915 code.
>>>>>>>
>>>>>>> This is not a generic API we are trying to put in a helper, these 
>>>>>>> are
>>>>>>> hard-coded tables so there is a difference between looking at 
>>>>>>> these Vs
>>>>>>> looking at some common code which can move to the core.
>>>>>>>
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Also, i think its too risky to change other drivers to use
>>>>>>>>>>> whatever math
>>>>>>>>>>> we put in the drm_dsc_helper to compute thr RC params because
>>>>>>>>>>> their code
>>>>>>>>>>> might be computing and using this tables differently.
>>>>>>>>>>>
>>>>>>>>>>> Its too much ownership for MSM developers to move this to
>>>>>>>>>>> drm_dsc_helper
>>>>>>>>>>> and own that as it might cause breakage of basic DSC even if 
>>>>>>>>>>> some
>>>>>>>>>>> values
>>>>>>>>>>> are repeated.
>>>>>>>>>>
>>>>>>>>>> It's time to stop thinking about ownership and start thinking 
>>>>>>>>>> about
>>>>>>>>>> shared code. We already have two instances of DSC tables. I don't
>>>>>>>>>> think having a third instance, which is a subset of an existing
>>>>>>>>>> dataset, would be beneficial to anybody.
>>>>>>>>>> AMD has complicated code which supports half-bit bpp and 
>>>>>>>>>> calculates
>>>>>>>>>> some of the parameters. But sharing data with the i915 driver is
>>>>>>>>>> straightforward.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Sorry, but I would like to get an ack from i915 folks if this 
>>>>>>>>> is going
>>>>>>>>> to be useful to them if we move this to helper because we have to
>>>>>>>>> look at every table. Not just one.
>>>>>>>>
>>>>>>>> Added i915 maintainers to the CC list for them to be able to 
>>>>>>>> answer.
>>>>>>>>
>>>>>>>
>>>>>>> Thanks, lets wait to hear from them about where finally these tables
>>>>>>> should go but thats can be taken up as a separate effort too.
>>>>>>>
>>>>>>>>>
>>>>>>>>> Also, this is just 1.1, we will add more tables for 1.2. So we 
>>>>>>>>> will
>>>>>>>>> have to end up changing both 1.1 and 1.2 tables as they are
>>>>>>>>> different for QC.
>>>>>>>>
>>>>>>>> I haven't heard back from Kuogee about the possible causes of using
>>>>>>>> rc/qp values from 1.2 even for 1.1 panels. Maybe you can comment on
>>>>>>>> that? In other words, can we always stick to the values from 1.2
>>>>>>>> standard? What will be the drawback?
>>>>>>>>
>>>>>>>> Otherwise, we'd have to have two different sets of values, like you
>>>>>>>> do in your vendor driver.
>>>>>>>>
>>>>>>>
>>>>>>> I have responded to this in the other email.
>>>>>>>
>>>>>>> All this being said, even if the rc tables move the drm_dsc_helper
>>>>>>> either now or later on, we will still need MSM specific calculations
>>>>>>> for many of the other encoder parameters (which are again either
>>>>>>> hard-coded or calculated). Please refer to the
>>>>>>> sde_dsc_populate_dsc_config() downstream. And yes, you will not find
>>>>>>> those in the DP spec directly.
>>>>>>>
>>>>>>> So we will still need a dsc helper for MSM calculations to be common
>>>>>>> for DSI / DP irrespective of where the tables go.
>>>>>>>
>>>>>>> So, lets finalize that first.
>>>>>>
>>>>>> I went on and trimmed sde_dsc_populate_dsc_config() to remove
>>>>>> duplication with the drm_dsc_compute_rc_parameters() (which we 
>>>>>> already
>>>>>> use for the MSM DSI DSC).
>>>>>>
>>>>>> Not much is left:
>>>>>>
>>>>>> dsc->first_line_bpg_offset set via the switch
>>>>>>
>>>>>> 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;
>>>>>>
>>>>>> if ((dsc->dsc_version_minor == 0x2) && (dsc->native_420))
>>>>>>        dsc->nsl_bpg_offset = (2048 *
>>>>>>                 (DIV_ROUND_UP(dsc->second_line_bpg_offset,
>>>>>>                                    (dsc->slice_height - 1))));
>>>>>>
>>>>>> dsc->initial_scale_value = 8 * dsc->rc_model_size /
>>>>>>                            (dsc->rc_model_size - 
>>>>>> dsc->initial_offset);
>>>>>>
>>>>>>
>>>>>> mux_word_size comes from the standard (must)
>>>>>> initial_scale_value calculation is recommended, but not required
>>>>>> nsl_bpg_offset follows the standard (must), also see below (*).
>>>>>>
>>>>>> first_line_bpg_offset calculation differs between three drivers. The
>>>>>> standard also provides a recommended formulas. I think we can 
>>>>>> leave it
>>>>>> as is for now.
>>>>>>
>>>>>> I think, that mux_word_size and nsl_bpg_offset calculation should be
>>>>>> moved to drm_dsc_compute_rc_parameters(), while leaving
>>>>>> initial_scale_value in place (in the driver code).
>>>>>>
>>>>>> * I think nsl_bpg_offset is slightly incorrectly calculated. Standard
>>>>>> demands that it is set to 'second_line_bpg_offset / (slice_height 
>>>>>> - 1),
>>>>>> rounded up to 16 fraction bits', while SDE driver code sets it to the
>>>>>> value rounded up to the next integer (having 16 fraction bits
>>>>>> representation).
>>>>>>
>>>>>> In my opinion correct calculation should be:
>>>>>> dsc->nsl_bpg_offset = DIV_ROUND_UP(2048 * 
>>>>>> dsc->second_line_bpg_offset,
>>>>>>                                    (dsc->slice_height - 1));
>>>>>>
>>>>>> Could you please check, which one is correct according to the 
>>>>>> standard?
>>>>>>
>>>>>>
>>>>>
>>>>> Sure, i will check about nsl_bpg_offset. But sorry if I was not more
>>>>> clear about this but sde_dsc_populate_dsc_config() is only one example
>>>>> which from your analysis can be moved to the drm_dsc_helper() but not
>>>>> the initial line calculation _dce_dsc_initial_line_calc(),
>>>>> _dce_dsc_ich_reset_override_needed() , _dce_dsc_setup_helper().
>>>>
>>>> The initial_line is already calculated in dpu_encoder.c. As for the
>>>> _dce_dsc_ich_reset_override_needed(), I don't think we support partial
>>>> updates in the upstream driver.
>>>>
>>>>>
>>>>> All of these are again common between DSI and DP.
>>>>>
>>>>> So in addition to thinking about what can be moved to the 
>>>>> drm_dsc_helper
>>>>> also think about what is specific to MSM but common to DSI and DP 
>>>>> modules.
>>>>>
>>>>> That was the bigger picture I was trying to convey.
>>>>
>>>
>>> _dce_dsc_initial_line_calc which will get expanded with v1.2 gets 
>>> added has much more than whats there in upstream today.
>>>
>>> Dumping everything in dpu_encoder is not the solution. Sorry.
>>
>> But it is still the DPU thing. So, no problems.
>>
> 
> I am not fully convinced. We will wait for your post, then see how the 
> code looks.

Hi Dmitry and Abhinav,

Just wanted to follow up on this thread. I've gone over the MSM-specific 
DSC params for DP and DSI and have found a few shared calculations and 
variables between both DSI and DP paths:

- (as mentioned earlier in the thread) almost all the calculations in 
dpu_dsc_populate_dsc_config() match dsi_populate_dsc_params() [1]. The 
only difference in the math I'm seeing is initial_scale_value.

- dsc_extra_pclk_cycle_cnt and dce_bytes_per_line, which were introduced 
in Kuogee's v1 DSC series [2], are used for DSI, DP, and the DPU timing 
engine. dsc_extra_pclk_cycle_cnt is calculated based on pclk_per_line 
(which is calculated differently between DP and DSI), but 
dce_bytes_per_line is calculated the same way between DP and DSI.

To avoid having to duplicate math in 2 different places, I think it 
would help to have these calculations in some msm_dsc_helper.c file. Any 
thoughts on this?

Thanks,

Jessica Zhang

[1] 
https://elixir.bootlin.com/linux/v6.3-rc2/source/drivers/gpu/drm/msm/dsi/dsi_host.c#L1756

[2] https://patchwork.freedesktop.org/patch/519845/?series=113240&rev=1

> 
>>>
>>>>
>>>>
>>
Dmitry Baryshkov March 16, 2023, 4:03 p.m. UTC | #23
Hi,

[removed previous conversation]

>
> Hi Dmitry and Abhinav,
>
> Just wanted to follow up on this thread. I've gone over the MSM-specific
> DSC params for DP and DSI and have found a few shared calculations and
> variables between both DSI and DP paths:
>
> - (as mentioned earlier in the thread) almost all the calculations in
> dpu_dsc_populate_dsc_config() match dsi_populate_dsc_params() [1]. The
> only difference in the math I'm seeing is initial_scale_value.

The value in dsi code is valid for initial_offset = 6144. Please use
the formula from the standard (= sde_dsc_populate_dsc_config) and add
it to drm_dsc_helper.c

If I remember correctly the last remaining item in
dsi_populate_dsc_params() (except mentioned initial_offset) was
line_buf_depth, see [3]. I'm not sure about setting it to bpc+1.
According to the standard it should come from a DSC decoder spec,
which means it should be set by the DSI panel driver or via
drm_dp_dsc_sink_line_buf_depth() in the case of DP output.

> - dsc_extra_pclk_cycle_cnt and dce_bytes_per_line, which were introduced
> in Kuogee's v1 DSC series [2], are used for DSI, DP, and the DPU timing
> engine. dsc_extra_pclk_cycle_cnt is calculated based on pclk_per_line
> (which is calculated differently between DP and DSI), but
> dce_bytes_per_line is calculated the same way between DP and DSI.
>
> To avoid having to duplicate math in 2 different places, I think it
> would help to have these calculations in some msm_dsc_helper.c file. Any
> thoughts on this?

dsc_extra_pclk_cycle_cnt and dce_bytes_per_line are used only in DPU
code, so they can stay in DPU driver.

>
> Thanks,
>
> Jessica Zhang
>
> [1]
> https://elixir.bootlin.com/linux/v6.3-rc2/source/drivers/gpu/drm/msm/dsi/dsi_host.c#L1756
>
> [2] https://patchwork.freedesktop.org/patch/519845/?series=113240&rev=1

[3] https://patchwork.freedesktop.org/patch/525441/?series=114472&rev=2
Abhinav Kumar March 16, 2023, 4:13 p.m. UTC | #24
On 3/16/2023 9:03 AM, Dmitry Baryshkov wrote:
> Hi,
> 
> [removed previous conversation]
> 
>>
>> Hi Dmitry and Abhinav,
>>
>> Just wanted to follow up on this thread. I've gone over the MSM-specific
>> DSC params for DP and DSI and have found a few shared calculations and
>> variables between both DSI and DP paths:
>>
>> - (as mentioned earlier in the thread) almost all the calculations in
>> dpu_dsc_populate_dsc_config() match dsi_populate_dsc_params() [1]. The
>> only difference in the math I'm seeing is initial_scale_value.
> 
> The value in dsi code is valid for initial_offset = 6144. Please use
> the formula from the standard (= sde_dsc_populate_dsc_config) and add
> it to drm_dsc_helper.c
> 
> If I remember correctly the last remaining item in
> dsi_populate_dsc_params() (except mentioned initial_offset) was
> line_buf_depth, see [3]. I'm not sure about setting it to bpc+1.
> According to the standard it should come from a DSC decoder spec,
> which means it should be set by the DSI panel driver or via
> drm_dp_dsc_sink_line_buf_depth() in the case of DP output.
> 
>> - dsc_extra_pclk_cycle_cnt and dce_bytes_per_line, which were introduced
>> in Kuogee's v1 DSC series [2], are used for DSI, DP, and the DPU timing
>> engine. dsc_extra_pclk_cycle_cnt is calculated based on pclk_per_line
>> (which is calculated differently between DP and DSI), but
>> dce_bytes_per_line is calculated the same way between DP and DSI.
>>
>> To avoid having to duplicate math in 2 different places, I think it
>> would help to have these calculations in some msm_dsc_helper.c file. Any
>> thoughts on this?
> 
> dsc_extra_pclk_cycle_cnt and dce_bytes_per_line are used only in DPU
> code, so they can stay in DPU driver.
> 

They can stay in the dpu driver is fine but where?

Like Jessica wrote, this is computed and used in 3 places today :

1) DSI video engine computation
2) DP controller computation
3) timing engine programming

So either we have a helper in a common location somewhere so that these 
3 modules can call that helper and use it OR each module duplicates the 
computation code.

What should be the common location is the discussion here.

It cannot be dpu_encoder.c as the DSI/DP dont call into the encoder methods.

>>
>> Thanks,
>>
>> Jessica Zhang
>>
>> [1]
>> https://elixir.bootlin.com/linux/v6.3-rc2/source/drivers/gpu/drm/msm/dsi/dsi_host.c#L1756
>>
>> [2] https://patchwork.freedesktop.org/patch/519845/?series=113240&rev=1
> 
> [3] https://patchwork.freedesktop.org/patch/525441/?series=114472&rev=2
> 
> 
>
Dmitry Baryshkov March 16, 2023, 4:23 p.m. UTC | #25
On 16/03/2023 18:13, Abhinav Kumar wrote:
> 
> 
> On 3/16/2023 9:03 AM, Dmitry Baryshkov wrote:
>> Hi,
>>
>> [removed previous conversation]
>>
>>>
>>> Hi Dmitry and Abhinav,
>>>
>>> Just wanted to follow up on this thread. I've gone over the MSM-specific
>>> DSC params for DP and DSI and have found a few shared calculations and
>>> variables between both DSI and DP paths:
>>>
>>> - (as mentioned earlier in the thread) almost all the calculations in
>>> dpu_dsc_populate_dsc_config() match dsi_populate_dsc_params() [1]. The
>>> only difference in the math I'm seeing is initial_scale_value.
>>
>> The value in dsi code is valid for initial_offset = 6144. Please use
>> the formula from the standard (= sde_dsc_populate_dsc_config) and add
>> it to drm_dsc_helper.c
>>
>> If I remember correctly the last remaining item in
>> dsi_populate_dsc_params() (except mentioned initial_offset) was
>> line_buf_depth, see [3]. I'm not sure about setting it to bpc+1.
>> According to the standard it should come from a DSC decoder spec,
>> which means it should be set by the DSI panel driver or via
>> drm_dp_dsc_sink_line_buf_depth() in the case of DP output.
>>
>>> - dsc_extra_pclk_cycle_cnt and dce_bytes_per_line, which were introduced
>>> in Kuogee's v1 DSC series [2], are used for DSI, DP, and the DPU timing
>>> engine. dsc_extra_pclk_cycle_cnt is calculated based on pclk_per_line
>>> (which is calculated differently between DP and DSI), but
>>> dce_bytes_per_line is calculated the same way between DP and DSI.
>>>
>>> To avoid having to duplicate math in 2 different places, I think it
>>> would help to have these calculations in some msm_dsc_helper.c file. Any
>>> thoughts on this?
>>
>> dsc_extra_pclk_cycle_cnt and dce_bytes_per_line are used only in DPU
>> code, so they can stay in DPU driver.
>>
> 
> They can stay in the dpu driver is fine but where?
> 
> Like Jessica wrote, this is computed and used in 3 places today :
> 
> 1) DSI video engine computation
> 2) DP controller computation
> 3) timing engine programming

Please excuse me if I'm wrong. I checked both vendor techpack and the 
Kuogee's patches. I see them being used only in the SDE / DPU driver 
code. Could you please point me to the code path that we are discussing?


> So either we have a helper in a common location somewhere so that these 
> 3 modules can call that helper and use it OR each module duplicates the 
> computation code.
> 
> What should be the common location is the discussion here.
> 
> It cannot be dpu_encoder.c as the DSI/DP dont call into the encoder 
> methods.
> 
>>>
>>> Thanks,
>>>
>>> Jessica Zhang
>>>
>>> [1]
>>> https://elixir.bootlin.com/linux/v6.3-rc2/source/drivers/gpu/drm/msm/dsi/dsi_host.c#L1756
>>>
>>> [2] https://patchwork.freedesktop.org/patch/519845/?series=113240&rev=1
>>
>> [3] https://patchwork.freedesktop.org/patch/525441/?series=114472&rev=2
>>
>>
>>
Abhinav Kumar March 16, 2023, 4:36 p.m. UTC | #26
On 3/16/2023 9:23 AM, Dmitry Baryshkov wrote:
> On 16/03/2023 18:13, Abhinav Kumar wrote:
>>
>>
>> On 3/16/2023 9:03 AM, Dmitry Baryshkov wrote:
>>> Hi,
>>>
>>> [removed previous conversation]
>>>
>>>>
>>>> Hi Dmitry and Abhinav,
>>>>
>>>> Just wanted to follow up on this thread. I've gone over the 
>>>> MSM-specific
>>>> DSC params for DP and DSI and have found a few shared calculations and
>>>> variables between both DSI and DP paths:
>>>>
>>>> - (as mentioned earlier in the thread) almost all the calculations in
>>>> dpu_dsc_populate_dsc_config() match dsi_populate_dsc_params() [1]. The
>>>> only difference in the math I'm seeing is initial_scale_value.
>>>
>>> The value in dsi code is valid for initial_offset = 6144. Please use
>>> the formula from the standard (= sde_dsc_populate_dsc_config) and add
>>> it to drm_dsc_helper.c
>>>
>>> If I remember correctly the last remaining item in
>>> dsi_populate_dsc_params() (except mentioned initial_offset) was
>>> line_buf_depth, see [3]. I'm not sure about setting it to bpc+1.
>>> According to the standard it should come from a DSC decoder spec,
>>> which means it should be set by the DSI panel driver or via
>>> drm_dp_dsc_sink_line_buf_depth() in the case of DP output.
>>>
>>>> - dsc_extra_pclk_cycle_cnt and dce_bytes_per_line, which were 
>>>> introduced
>>>> in Kuogee's v1 DSC series [2], are used for DSI, DP, and the DPU timing
>>>> engine. dsc_extra_pclk_cycle_cnt is calculated based on pclk_per_line
>>>> (which is calculated differently between DP and DSI), but
>>>> dce_bytes_per_line is calculated the same way between DP and DSI.
>>>>
>>>> To avoid having to duplicate math in 2 different places, I think it
>>>> would help to have these calculations in some msm_dsc_helper.c file. 
>>>> Any
>>>> thoughts on this?
>>>
>>> dsc_extra_pclk_cycle_cnt and dce_bytes_per_line are used only in DPU
>>> code, so they can stay in DPU driver.
>>>
>>
>> They can stay in the dpu driver is fine but where?
>>
>> Like Jessica wrote, this is computed and used in 3 places today :
>>
>> 1) DSI video engine computation
>> 2) DP controller computation
>> 3) timing engine programming
> 
> Please excuse me if I'm wrong. I checked both vendor techpack and the 
> Kuogee's patches. I see them being used only in the SDE / DPU driver 
> code. Could you please point me to the code path that we are discussing?
> 

DSI code :

https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/dsi/dsi_host.c#L868

DP code:

Refer to dp_panel_dsc_pclk_param_calc in 
https://patchwork.freedesktop.org/patch/519837/?series=113240&rev=1

Timing engine:

refer to https://patchwork.freedesktop.org/patch/519838/?series=113240&rev=1

Probably confusion is due to the naming. bytes_per_line is nothing but 
bytes_per_pkt * pkt_per_line but the concept is common for DP and DSI.

+		if (phys->comp_type == MSM_DISPLAY_COMPRESSION_DSC) {
+			phys->dsc_extra_pclk_cycle_cnt = dsc_info->pclk_per_line;
+			phys->dsc_extra_disp_width = dsc_info->extra_width;
+			phys->dce_bytes_per_line =
+				dsc_info->bytes_per_pkt * dsc_info->pkt_per_line;

> 
>> So either we have a helper in a common location somewhere so that 
>> these 3 modules can call that helper and use it OR each module 
>> duplicates the computation code.
>>
>> What should be the common location is the discussion here.
>>
>> It cannot be dpu_encoder.c as the DSI/DP dont call into the encoder 
>> methods.
>>
>>>>
>>>> Thanks,
>>>>
>>>> Jessica Zhang
>>>>
>>>> [1]
>>>> https://elixir.bootlin.com/linux/v6.3-rc2/source/drivers/gpu/drm/msm/dsi/dsi_host.c#L1756 
>>>>
>>>>
>>>> [2] https://patchwork.freedesktop.org/patch/519845/?series=113240&rev=1
>>>
>>> [3] https://patchwork.freedesktop.org/patch/525441/?series=114472&rev=2
>>>
>>>
>>>
>
Abhinav Kumar March 16, 2023, 11:40 p.m. UTC | #27
On 3/16/2023 9:36 AM, Abhinav Kumar wrote:
> 
> 
> On 3/16/2023 9:23 AM, Dmitry Baryshkov wrote:
>> On 16/03/2023 18:13, Abhinav Kumar wrote:
>>>
>>>
>>> On 3/16/2023 9:03 AM, Dmitry Baryshkov wrote:
>>>> Hi,
>>>>
>>>> [removed previous conversation]
>>>>
>>>>>
>>>>> Hi Dmitry and Abhinav,
>>>>>
>>>>> Just wanted to follow up on this thread. I've gone over the 
>>>>> MSM-specific
>>>>> DSC params for DP and DSI and have found a few shared calculations and
>>>>> variables between both DSI and DP paths:
>>>>>
>>>>> - (as mentioned earlier in the thread) almost all the calculations in
>>>>> dpu_dsc_populate_dsc_config() match dsi_populate_dsc_params() [1]. The
>>>>> only difference in the math I'm seeing is initial_scale_value.
>>>>
>>>> The value in dsi code is valid for initial_offset = 6144. Please use
>>>> the formula from the standard (= sde_dsc_populate_dsc_config) and add
>>>> it to drm_dsc_helper.c
>>>>

Yes, I agree with this part. for rc_model_size we can use 
DSC_RC_MODEL_SIZE_CONST.

initial_offset is already handled in 
https://patchwork.freedesktop.org/patch/525424/?series=114472&rev=2

Then we can use this math:

rc_model_size / (rc_model_size -
initial_offset), keeping in mind that initial_scale_value has three 
fractional bits.

So this would be 8192 / (8192 - 6144) = 4

Then << 3 for 3 fractional bits = 32.

>>>> If I remember correctly the last remaining item in
>>>> dsi_populate_dsc_params() (except mentioned initial_offset) was
>>>> line_buf_depth, see [3]. I'm not sure about setting it to bpc+1.
>>>> According to the standard it should come from a DSC decoder spec,
>>>> which means it should be set by the DSI panel driver or via
>>>> drm_dp_dsc_sink_line_buf_depth() in the case of DP output.
>>>>
>>>>> - dsc_extra_pclk_cycle_cnt and dce_bytes_per_line, which were 
>>>>> introduced
>>>>> in Kuogee's v1 DSC series [2], are used for DSI, DP, and the DPU 
>>>>> timing
>>>>> engine. dsc_extra_pclk_cycle_cnt is calculated based on pclk_per_line
>>>>> (which is calculated differently between DP and DSI), but
>>>>> dce_bytes_per_line is calculated the same way between DP and DSI.
>>>>>
>>>>> To avoid having to duplicate math in 2 different places, I think it
>>>>> would help to have these calculations in some msm_dsc_helper.c 
>>>>> file. Any
>>>>> thoughts on this?
>>>>
>>>> dsc_extra_pclk_cycle_cnt and dce_bytes_per_line are used only in DPU
>>>> code, so they can stay in DPU driver.
>>>>
>>>
>>> They can stay in the dpu driver is fine but where?
>>>
>>> Like Jessica wrote, this is computed and used in 3 places today :
>>>
>>> 1) DSI video engine computation
>>> 2) DP controller computation
>>> 3) timing engine programming
>>
>> Please excuse me if I'm wrong. I checked both vendor techpack and the 
>> Kuogee's patches. I see them being used only in the SDE / DPU driver 
>> code. Could you please point me to the code path that we are discussing?
>>
> 
> DSI code :
> 
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/dsi/dsi_host.c#L868 
> 
> 
> DP code:
> 
> Refer to dp_panel_dsc_pclk_param_calc in 
> https://patchwork.freedesktop.org/patch/519837/?series=113240&rev=1
> 
> Timing engine:
> 
> refer to 
> https://patchwork.freedesktop.org/patch/519838/?series=113240&rev=1
> 
> Probably confusion is due to the naming. bytes_per_line is nothing but 
> bytes_per_pkt * pkt_per_line but the concept is common for DP and DSI.
> 
> +        if (phys->comp_type == MSM_DISPLAY_COMPRESSION_DSC) {
> +            phys->dsc_extra_pclk_cycle_cnt = dsc_info->pclk_per_line;
> +            phys->dsc_extra_disp_width = dsc_info->extra_width;
> +            phys->dce_bytes_per_line =
> +                dsc_info->bytes_per_pkt * dsc_info->pkt_per_line;
> 
>>
>>> So either we have a helper in a common location somewhere so that 
>>> these 3 modules can call that helper and use it OR each module 
>>> duplicates the computation code.
>>>
>>> What should be the common location is the discussion here.
>>>
>>> It cannot be dpu_encoder.c as the DSI/DP dont call into the encoder 
>>> methods.
>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jessica Zhang
>>>>>
>>>>> [1]
>>>>> https://elixir.bootlin.com/linux/v6.3-rc2/source/drivers/gpu/drm/msm/dsi/dsi_host.c#L1756 
>>>>>
>>>>>
>>>>> [2] 
>>>>> https://patchwork.freedesktop.org/patch/519845/?series=113240&rev=1
>>>>
>>>> [3] https://patchwork.freedesktop.org/patch/525441/?series=114472&rev=2
>>>>
>>>>
>>>>
>>
diff mbox series

Patch

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"
+
+
+#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
+};
+
+/*
+ * 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 */
+	{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);
+
+	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)) {
+		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);
+	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];
+
+	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);
+}
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
+
+/**
+ * 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,
+};
+
+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);
+
+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);
+
+#endif /* __DPU_DSC_HELPER_H__ */
+