Since the type and usage of CSC matrices is spanning across DPU
lets introduce a helper to the dpu_hw_util to return the CSC
corresponding to the request type. This will help to add more
supported CSC types such as the RGB to YUV one which is used in
the case of CDM.
Signed-off-by: Abhinav Kumar <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 54 +++++++++++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 7 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 39 ++-------------
3 files changed, 64 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
index 0b05061e3e62..59a153331194 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
@@ -87,6 +87,60 @@ static u32 dpu_hw_util_log_mask = DPU_DBG_MASK_NONE;
#define QOS_QOS_CTRL_VBLANK_EN BIT(16)
#define QOS_QOS_CTRL_CREQ_VBLANK_MASK GENMASK(21, 20)
+static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
+ {
+ /* S15.16 format */
+ 0x00012A00, 0x00000000, 0x00019880,
+ 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
+ 0x00012A00, 0x00020480, 0x00000000,
+ },
+ /* signed bias */
+ { 0xfff0, 0xff80, 0xff80,},
+ { 0x0, 0x0, 0x0,},
+ /* unsigned clamp */
+ { 0x10, 0xeb, 0x10, 0xf0, 0x10, 0xf0,},
+ { 0x00, 0xff, 0x00, 0xff, 0x00, 0xff,},
+};
+
+static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
+ {
+ /* S15.16 format */
+ 0x00012A00, 0x00000000, 0x00019880,
+ 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
+ 0x00012A00, 0x00020480, 0x00000000,
+ },
+ /* signed bias */
+ { 0xffc0, 0xfe00, 0xfe00,},
+ { 0x0, 0x0, 0x0,},
+ /* unsigned clamp */
+ { 0x40, 0x3ac, 0x40, 0x3c0, 0x40, 0x3c0,},
+ { 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
+};
+
+/**
+ * dpu_hw_get_csc_cfg - get the CSC matrix based on the request type
+ * @type: type of the requested CSC matrix from caller
+ * Return: CSC matrix corresponding to the request type in DPU format
+ */
+const struct dpu_csc_cfg *dpu_hw_get_csc_cfg(enum dpu_hw_csc_cfg_type type)
+{
+ const struct dpu_csc_cfg *csc_cfg = NULL;
+
+ switch (type) {
+ case DPU_HW_YUV2RGB_601L:
+ csc_cfg = &dpu_csc_YUV2RGB_601L;
+ break;
+ case DPU_HW_YUV2RGB_601L_10BIT:
+ csc_cfg = &dpu_csc10_YUV2RGB_601L;
+ break;
+ default:
+ DPU_ERROR("unknown csc_cfg type\n");
+ break;
+ }
+
+ return csc_cfg;
+}
+
void dpu_reg_write(struct dpu_hw_blk_reg_map *c,
u32 reg_off,
u32 val,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
index fe083b2e5696..49f2bcf6de15 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
@@ -19,6 +19,11 @@
#define MISR_CTRL_STATUS_CLEAR BIT(10)
#define MISR_CTRL_FREE_RUN_MASK BIT(31)
+enum dpu_hw_csc_cfg_type {
+ DPU_HW_YUV2RGB_601L,
+ DPU_HW_YUV2RGB_601L_10BIT,
+};
+
/*
* This is the common struct maintained by each sub block
* for mapping the register offsets in this block to the
@@ -368,4 +373,6 @@ bool dpu_hw_clk_force_ctrl(struct dpu_hw_blk_reg_map *c,
const struct dpu_clk_ctrl_reg *clk_ctrl_reg,
bool enable);
+const struct dpu_csc_cfg *dpu_hw_get_csc_cfg(enum dpu_hw_csc_cfg_type type);
+
#endif /* _DPU_HW_UTIL_H */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 3235ab132540..31641889b9f0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -21,6 +21,7 @@
#include "dpu_kms.h"
#include "dpu_formats.h"
#include "dpu_hw_sspp.h"
+#include "dpu_hw_util.h"
#include "dpu_trace.h"
#include "dpu_crtc.h"
#include "dpu_vbif.h"
@@ -508,50 +509,16 @@ static void _dpu_plane_setup_pixel_ext(struct dpu_hw_scaler3_cfg *scale_cfg,
}
}
-static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
- {
- /* S15.16 format */
- 0x00012A00, 0x00000000, 0x00019880,
- 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
- 0x00012A00, 0x00020480, 0x00000000,
- },
- /* signed bias */
- { 0xfff0, 0xff80, 0xff80,},
- { 0x0, 0x0, 0x0,},
- /* unsigned clamp */
- { 0x10, 0xeb, 0x10, 0xf0, 0x10, 0xf0,},
- { 0x00, 0xff, 0x00, 0xff, 0x00, 0xff,},
-};
-
-static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
- {
- /* S15.16 format */
- 0x00012A00, 0x00000000, 0x00019880,
- 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
- 0x00012A00, 0x00020480, 0x00000000,
- },
- /* signed bias */
- { 0xffc0, 0xfe00, 0xfe00,},
- { 0x0, 0x0, 0x0,},
- /* unsigned clamp */
- { 0x40, 0x3ac, 0x40, 0x3c0, 0x40, 0x3c0,},
- { 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
-};
-
static const struct dpu_csc_cfg *_dpu_plane_get_csc(struct dpu_sw_pipe *pipe,
const struct dpu_format *fmt)
{
- const struct dpu_csc_cfg *csc_ptr;
-
if (!DPU_FORMAT_IS_YUV(fmt))
return NULL;
if (BIT(DPU_SSPP_CSC_10BIT) & pipe->sspp->cap->features)
- csc_ptr = &dpu_csc10_YUV2RGB_601L;
+ return dpu_hw_get_csc_cfg(DPU_HW_YUV2RGB_601L_10BIT);
else
- csc_ptr = &dpu_csc_YUV2RGB_601L;
-
- return csc_ptr;
+ return dpu_hw_get_csc_cfg(DPU_HW_YUV2RGB_601L);
}
static void _dpu_plane_setup_scaler(struct dpu_sw_pipe *pipe,
--
2.40.1
On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <[email protected]> wrote:
>
> Since the type and usage of CSC matrices is spanning across DPU
> lets introduce a helper to the dpu_hw_util to return the CSC
> corresponding to the request type. This will help to add more
> supported CSC types such as the RGB to YUV one which is used in
> the case of CDM.
>
> Signed-off-by: Abhinav Kumar <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 54 +++++++++++++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 7 +++
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 39 ++-------------
> 3 files changed, 64 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> index 0b05061e3e62..59a153331194 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> @@ -87,6 +87,60 @@ static u32 dpu_hw_util_log_mask = DPU_DBG_MASK_NONE;
> #define QOS_QOS_CTRL_VBLANK_EN BIT(16)
> #define QOS_QOS_CTRL_CREQ_VBLANK_MASK GENMASK(21, 20)
>
> +static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
> + {
> + /* S15.16 format */
> + 0x00012A00, 0x00000000, 0x00019880,
> + 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
> + 0x00012A00, 0x00020480, 0x00000000,
> + },
> + /* signed bias */
> + { 0xfff0, 0xff80, 0xff80,},
> + { 0x0, 0x0, 0x0,},
> + /* unsigned clamp */
> + { 0x10, 0xeb, 0x10, 0xf0, 0x10, 0xf0,},
> + { 0x00, 0xff, 0x00, 0xff, 0x00, 0xff,},
> +};
> +
> +static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
> + {
> + /* S15.16 format */
> + 0x00012A00, 0x00000000, 0x00019880,
> + 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
> + 0x00012A00, 0x00020480, 0x00000000,
> + },
> + /* signed bias */
> + { 0xffc0, 0xfe00, 0xfe00,},
> + { 0x0, 0x0, 0x0,},
> + /* unsigned clamp */
> + { 0x40, 0x3ac, 0x40, 0x3c0, 0x40, 0x3c0,},
> + { 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
> +};
> +
> +/**
> + * dpu_hw_get_csc_cfg - get the CSC matrix based on the request type
> + * @type: type of the requested CSC matrix from caller
> + * Return: CSC matrix corresponding to the request type in DPU format
> + */
> +const struct dpu_csc_cfg *dpu_hw_get_csc_cfg(enum dpu_hw_csc_cfg_type type)
> +{
> + const struct dpu_csc_cfg *csc_cfg = NULL;
> +
> + switch (type) {
> + case DPU_HW_YUV2RGB_601L:
> + csc_cfg = &dpu_csc_YUV2RGB_601L;
> + break;
> + case DPU_HW_YUV2RGB_601L_10BIT:
> + csc_cfg = &dpu_csc10_YUV2RGB_601L;
> + break;
> + default:
> + DPU_ERROR("unknown csc_cfg type\n");
> + break;
> + }
> +
> + return csc_cfg;
> +}
> +
> void dpu_reg_write(struct dpu_hw_blk_reg_map *c,
> u32 reg_off,
> u32 val,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> index fe083b2e5696..49f2bcf6de15 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> @@ -19,6 +19,11 @@
> #define MISR_CTRL_STATUS_CLEAR BIT(10)
> #define MISR_CTRL_FREE_RUN_MASK BIT(31)
>
> +enum dpu_hw_csc_cfg_type {
> + DPU_HW_YUV2RGB_601L,
> + DPU_HW_YUV2RGB_601L_10BIT,
> +};
> +
> /*
> * This is the common struct maintained by each sub block
> * for mapping the register offsets in this block to the
> @@ -368,4 +373,6 @@ bool dpu_hw_clk_force_ctrl(struct dpu_hw_blk_reg_map *c,
> const struct dpu_clk_ctrl_reg *clk_ctrl_reg,
> bool enable);
>
> +const struct dpu_csc_cfg *dpu_hw_get_csc_cfg(enum dpu_hw_csc_cfg_type type);
I don't think we need extra enum and wrapper. Just export const data
structures directly.
> +
> #endif /* _DPU_HW_UTIL_H */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 3235ab132540..31641889b9f0 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -21,6 +21,7 @@
> #include "dpu_kms.h"
> #include "dpu_formats.h"
> #include "dpu_hw_sspp.h"
> +#include "dpu_hw_util.h"
> #include "dpu_trace.h"
> #include "dpu_crtc.h"
> #include "dpu_vbif.h"
> @@ -508,50 +509,16 @@ static void _dpu_plane_setup_pixel_ext(struct dpu_hw_scaler3_cfg *scale_cfg,
> }
> }
>
> -static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
> - {
> - /* S15.16 format */
> - 0x00012A00, 0x00000000, 0x00019880,
> - 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
> - 0x00012A00, 0x00020480, 0x00000000,
> - },
> - /* signed bias */
> - { 0xfff0, 0xff80, 0xff80,},
> - { 0x0, 0x0, 0x0,},
> - /* unsigned clamp */
> - { 0x10, 0xeb, 0x10, 0xf0, 0x10, 0xf0,},
> - { 0x00, 0xff, 0x00, 0xff, 0x00, 0xff,},
> -};
> -
> -static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
> - {
> - /* S15.16 format */
> - 0x00012A00, 0x00000000, 0x00019880,
> - 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
> - 0x00012A00, 0x00020480, 0x00000000,
> - },
> - /* signed bias */
> - { 0xffc0, 0xfe00, 0xfe00,},
> - { 0x0, 0x0, 0x0,},
> - /* unsigned clamp */
> - { 0x40, 0x3ac, 0x40, 0x3c0, 0x40, 0x3c0,},
> - { 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
> -};
> -
> static const struct dpu_csc_cfg *_dpu_plane_get_csc(struct dpu_sw_pipe *pipe,
> const struct dpu_format *fmt)
> {
> - const struct dpu_csc_cfg *csc_ptr;
> -
> if (!DPU_FORMAT_IS_YUV(fmt))
> return NULL;
>
> if (BIT(DPU_SSPP_CSC_10BIT) & pipe->sspp->cap->features)
> - csc_ptr = &dpu_csc10_YUV2RGB_601L;
> + return dpu_hw_get_csc_cfg(DPU_HW_YUV2RGB_601L_10BIT);
> else
> - csc_ptr = &dpu_csc_YUV2RGB_601L;
> -
> - return csc_ptr;
> + return dpu_hw_get_csc_cfg(DPU_HW_YUV2RGB_601L);
> }
>
> static void _dpu_plane_setup_scaler(struct dpu_sw_pipe *pipe,
> --
> 2.40.1
>
--
With best wishes
Dmitry
On 12/8/2023 3:12 AM, Dmitry Baryshkov wrote:
> On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <[email protected]> wrote:
>>
>> Since the type and usage of CSC matrices is spanning across DPU
>> lets introduce a helper to the dpu_hw_util to return the CSC
>> corresponding to the request type. This will help to add more
>> supported CSC types such as the RGB to YUV one which is used in
>> the case of CDM.
>>
>> Signed-off-by: Abhinav Kumar <[email protected]>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 54 +++++++++++++++++++++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 7 +++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 39 ++-------------
>> 3 files changed, 64 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>> index 0b05061e3e62..59a153331194 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>> @@ -87,6 +87,60 @@ static u32 dpu_hw_util_log_mask = DPU_DBG_MASK_NONE;
>> #define QOS_QOS_CTRL_VBLANK_EN BIT(16)
>> #define QOS_QOS_CTRL_CREQ_VBLANK_MASK GENMASK(21, 20)
>>
>> +static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
>> + {
>> + /* S15.16 format */
>> + 0x00012A00, 0x00000000, 0x00019880,
>> + 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
>> + 0x00012A00, 0x00020480, 0x00000000,
>> + },
>> + /* signed bias */
>> + { 0xfff0, 0xff80, 0xff80,},
>> + { 0x0, 0x0, 0x0,},
>> + /* unsigned clamp */
>> + { 0x10, 0xeb, 0x10, 0xf0, 0x10, 0xf0,},
>> + { 0x00, 0xff, 0x00, 0xff, 0x00, 0xff,},
>> +};
>> +
>> +static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
>> + {
>> + /* S15.16 format */
>> + 0x00012A00, 0x00000000, 0x00019880,
>> + 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
>> + 0x00012A00, 0x00020480, 0x00000000,
>> + },
>> + /* signed bias */
>> + { 0xffc0, 0xfe00, 0xfe00,},
>> + { 0x0, 0x0, 0x0,},
>> + /* unsigned clamp */
>> + { 0x40, 0x3ac, 0x40, 0x3c0, 0x40, 0x3c0,},
>> + { 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
>> +};
>> +
>> +/**
>> + * dpu_hw_get_csc_cfg - get the CSC matrix based on the request type
>> + * @type: type of the requested CSC matrix from caller
>> + * Return: CSC matrix corresponding to the request type in DPU format
>> + */
>> +const struct dpu_csc_cfg *dpu_hw_get_csc_cfg(enum dpu_hw_csc_cfg_type type)
>> +{
>> + const struct dpu_csc_cfg *csc_cfg = NULL;
>> +
>> + switch (type) {
>> + case DPU_HW_YUV2RGB_601L:
>> + csc_cfg = &dpu_csc_YUV2RGB_601L;
>> + break;
>> + case DPU_HW_YUV2RGB_601L_10BIT:
>> + csc_cfg = &dpu_csc10_YUV2RGB_601L;
>> + break;
>> + default:
>> + DPU_ERROR("unknown csc_cfg type\n");
>> + break;
>> + }
>> +
>> + return csc_cfg;
>> +}
>> +
>> void dpu_reg_write(struct dpu_hw_blk_reg_map *c,
>> u32 reg_off,
>> u32 val,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>> index fe083b2e5696..49f2bcf6de15 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>> @@ -19,6 +19,11 @@
>> #define MISR_CTRL_STATUS_CLEAR BIT(10)
>> #define MISR_CTRL_FREE_RUN_MASK BIT(31)
>>
>> +enum dpu_hw_csc_cfg_type {
>> + DPU_HW_YUV2RGB_601L,
>> + DPU_HW_YUV2RGB_601L_10BIT,
>> +};
>> +
>> /*
>> * This is the common struct maintained by each sub block
>> * for mapping the register offsets in this block to the
>> @@ -368,4 +373,6 @@ bool dpu_hw_clk_force_ctrl(struct dpu_hw_blk_reg_map *c,
>> const struct dpu_clk_ctrl_reg *clk_ctrl_reg,
>> bool enable);
>>
>> +const struct dpu_csc_cfg *dpu_hw_get_csc_cfg(enum dpu_hw_csc_cfg_type type);
>
> I don't think we need extra enum and wrapper. Just export const data
> structures directly.
>
I liked this approach because the blocks of DPU such as plane and CDM
are clients to the dpu_hw_util and just request the type and the util
handles their request of returning the correct csc matrix.
Do you see any issue with this?
>> +
>> #endif /* _DPU_HW_UTIL_H */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> index 3235ab132540..31641889b9f0 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> @@ -21,6 +21,7 @@
>> #include "dpu_kms.h"
>> #include "dpu_formats.h"
>> #include "dpu_hw_sspp.h"
>> +#include "dpu_hw_util.h"
>> #include "dpu_trace.h"
>> #include "dpu_crtc.h"
>> #include "dpu_vbif.h"
>> @@ -508,50 +509,16 @@ static void _dpu_plane_setup_pixel_ext(struct dpu_hw_scaler3_cfg *scale_cfg,
>> }
>> }
>>
>> -static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
>> - {
>> - /* S15.16 format */
>> - 0x00012A00, 0x00000000, 0x00019880,
>> - 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
>> - 0x00012A00, 0x00020480, 0x00000000,
>> - },
>> - /* signed bias */
>> - { 0xfff0, 0xff80, 0xff80,},
>> - { 0x0, 0x0, 0x0,},
>> - /* unsigned clamp */
>> - { 0x10, 0xeb, 0x10, 0xf0, 0x10, 0xf0,},
>> - { 0x00, 0xff, 0x00, 0xff, 0x00, 0xff,},
>> -};
>> -
>> -static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
>> - {
>> - /* S15.16 format */
>> - 0x00012A00, 0x00000000, 0x00019880,
>> - 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
>> - 0x00012A00, 0x00020480, 0x00000000,
>> - },
>> - /* signed bias */
>> - { 0xffc0, 0xfe00, 0xfe00,},
>> - { 0x0, 0x0, 0x0,},
>> - /* unsigned clamp */
>> - { 0x40, 0x3ac, 0x40, 0x3c0, 0x40, 0x3c0,},
>> - { 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
>> -};
>> -
>> static const struct dpu_csc_cfg *_dpu_plane_get_csc(struct dpu_sw_pipe *pipe,
>> const struct dpu_format *fmt)
>> {
>> - const struct dpu_csc_cfg *csc_ptr;
>> -
>> if (!DPU_FORMAT_IS_YUV(fmt))
>> return NULL;
>>
>> if (BIT(DPU_SSPP_CSC_10BIT) & pipe->sspp->cap->features)
>> - csc_ptr = &dpu_csc10_YUV2RGB_601L;
>> + return dpu_hw_get_csc_cfg(DPU_HW_YUV2RGB_601L_10BIT);
>> else
>> - csc_ptr = &dpu_csc_YUV2RGB_601L;
>> -
>> - return csc_ptr;
>> + return dpu_hw_get_csc_cfg(DPU_HW_YUV2RGB_601L);
>> }
>>
>> static void _dpu_plane_setup_scaler(struct dpu_sw_pipe *pipe,
>> --
>> 2.40.1
>>
>
>
On Fri, 8 Dec 2023 at 18:24, Abhinav Kumar <[email protected]> wrote:
>
>
>
> On 12/8/2023 3:12 AM, Dmitry Baryshkov wrote:
> > On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <[email protected]> wrote:
> >>
> >> Since the type and usage of CSC matrices is spanning across DPU
> >> lets introduce a helper to the dpu_hw_util to return the CSC
> >> corresponding to the request type. This will help to add more
> >> supported CSC types such as the RGB to YUV one which is used in
> >> the case of CDM.
> >>
> >> Signed-off-by: Abhinav Kumar <[email protected]>
> >> ---
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 54 +++++++++++++++++++++
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 7 +++
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 39 ++-------------
> >> 3 files changed, 64 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> >> index 0b05061e3e62..59a153331194 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> >> @@ -87,6 +87,60 @@ static u32 dpu_hw_util_log_mask = DPU_DBG_MASK_NONE;
> >> #define QOS_QOS_CTRL_VBLANK_EN BIT(16)
> >> #define QOS_QOS_CTRL_CREQ_VBLANK_MASK GENMASK(21, 20)
> >>
> >> +static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
> >> + {
> >> + /* S15.16 format */
> >> + 0x00012A00, 0x00000000, 0x00019880,
> >> + 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
> >> + 0x00012A00, 0x00020480, 0x00000000,
> >> + },
> >> + /* signed bias */
> >> + { 0xfff0, 0xff80, 0xff80,},
> >> + { 0x0, 0x0, 0x0,},
> >> + /* unsigned clamp */
> >> + { 0x10, 0xeb, 0x10, 0xf0, 0x10, 0xf0,},
> >> + { 0x00, 0xff, 0x00, 0xff, 0x00, 0xff,},
> >> +};
> >> +
> >> +static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
> >> + {
> >> + /* S15.16 format */
> >> + 0x00012A00, 0x00000000, 0x00019880,
> >> + 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
> >> + 0x00012A00, 0x00020480, 0x00000000,
> >> + },
> >> + /* signed bias */
> >> + { 0xffc0, 0xfe00, 0xfe00,},
> >> + { 0x0, 0x0, 0x0,},
> >> + /* unsigned clamp */
> >> + { 0x40, 0x3ac, 0x40, 0x3c0, 0x40, 0x3c0,},
> >> + { 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
> >> +};
> >> +
> >> +/**
> >> + * dpu_hw_get_csc_cfg - get the CSC matrix based on the request type
> >> + * @type: type of the requested CSC matrix from caller
> >> + * Return: CSC matrix corresponding to the request type in DPU format
> >> + */
> >> +const struct dpu_csc_cfg *dpu_hw_get_csc_cfg(enum dpu_hw_csc_cfg_type type)
> >> +{
> >> + const struct dpu_csc_cfg *csc_cfg = NULL;
> >> +
> >> + switch (type) {
> >> + case DPU_HW_YUV2RGB_601L:
> >> + csc_cfg = &dpu_csc_YUV2RGB_601L;
> >> + break;
> >> + case DPU_HW_YUV2RGB_601L_10BIT:
> >> + csc_cfg = &dpu_csc10_YUV2RGB_601L;
> >> + break;
> >> + default:
> >> + DPU_ERROR("unknown csc_cfg type\n");
> >> + break;
> >> + }
> >> +
> >> + return csc_cfg;
> >> +}
> >> +
> >> void dpu_reg_write(struct dpu_hw_blk_reg_map *c,
> >> u32 reg_off,
> >> u32 val,
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> >> index fe083b2e5696..49f2bcf6de15 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> >> @@ -19,6 +19,11 @@
> >> #define MISR_CTRL_STATUS_CLEAR BIT(10)
> >> #define MISR_CTRL_FREE_RUN_MASK BIT(31)
> >>
> >> +enum dpu_hw_csc_cfg_type {
> >> + DPU_HW_YUV2RGB_601L,
> >> + DPU_HW_YUV2RGB_601L_10BIT,
> >> +};
> >> +
> >> /*
> >> * This is the common struct maintained by each sub block
> >> * for mapping the register offsets in this block to the
> >> @@ -368,4 +373,6 @@ bool dpu_hw_clk_force_ctrl(struct dpu_hw_blk_reg_map *c,
> >> const struct dpu_clk_ctrl_reg *clk_ctrl_reg,
> >> bool enable);
> >>
> >> +const struct dpu_csc_cfg *dpu_hw_get_csc_cfg(enum dpu_hw_csc_cfg_type type);
> >
> > I don't think we need extra enum and wrapper. Just export const data
> > structures directly.
> >
>
> I liked this approach because the blocks of DPU such as plane and CDM
> are clients to the dpu_hw_util and just request the type and the util
> handles their request of returning the correct csc matrix.
>
> Do you see any issue with this?
Not an issue, but I don't see anything that requires an extra
abstraction. We perfectly know which CSC config we would like to get.
>
> >> +
> >> #endif /* _DPU_HW_UTIL_H */
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> index 3235ab132540..31641889b9f0 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> @@ -21,6 +21,7 @@
> >> #include "dpu_kms.h"
> >> #include "dpu_formats.h"
> >> #include "dpu_hw_sspp.h"
> >> +#include "dpu_hw_util.h"
> >> #include "dpu_trace.h"
> >> #include "dpu_crtc.h"
> >> #include "dpu_vbif.h"
> >> @@ -508,50 +509,16 @@ static void _dpu_plane_setup_pixel_ext(struct dpu_hw_scaler3_cfg *scale_cfg,
> >> }
> >> }
> >>
> >> -static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
> >> - {
> >> - /* S15.16 format */
> >> - 0x00012A00, 0x00000000, 0x00019880,
> >> - 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
> >> - 0x00012A00, 0x00020480, 0x00000000,
> >> - },
> >> - /* signed bias */
> >> - { 0xfff0, 0xff80, 0xff80,},
> >> - { 0x0, 0x0, 0x0,},
> >> - /* unsigned clamp */
> >> - { 0x10, 0xeb, 0x10, 0xf0, 0x10, 0xf0,},
> >> - { 0x00, 0xff, 0x00, 0xff, 0x00, 0xff,},
> >> -};
> >> -
> >> -static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
> >> - {
> >> - /* S15.16 format */
> >> - 0x00012A00, 0x00000000, 0x00019880,
> >> - 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
> >> - 0x00012A00, 0x00020480, 0x00000000,
> >> - },
> >> - /* signed bias */
> >> - { 0xffc0, 0xfe00, 0xfe00,},
> >> - { 0x0, 0x0, 0x0,},
> >> - /* unsigned clamp */
> >> - { 0x40, 0x3ac, 0x40, 0x3c0, 0x40, 0x3c0,},
> >> - { 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
> >> -};
> >> -
> >> static const struct dpu_csc_cfg *_dpu_plane_get_csc(struct dpu_sw_pipe *pipe,
> >> const struct dpu_format *fmt)
> >> {
> >> - const struct dpu_csc_cfg *csc_ptr;
> >> -
> >> if (!DPU_FORMAT_IS_YUV(fmt))
> >> return NULL;
> >>
> >> if (BIT(DPU_SSPP_CSC_10BIT) & pipe->sspp->cap->features)
> >> - csc_ptr = &dpu_csc10_YUV2RGB_601L;
> >> + return dpu_hw_get_csc_cfg(DPU_HW_YUV2RGB_601L_10BIT);
> >> else
> >> - csc_ptr = &dpu_csc_YUV2RGB_601L;
> >> -
> >> - return csc_ptr;
> >> + return dpu_hw_get_csc_cfg(DPU_HW_YUV2RGB_601L);
> >> }
> >>
> >> static void _dpu_plane_setup_scaler(struct dpu_sw_pipe *pipe,
> >> --
> >> 2.40.1
> >>
> >
> >
--
With best wishes
Dmitry
On 12/8/2023 8:27 AM, Dmitry Baryshkov wrote:
> On Fri, 8 Dec 2023 at 18:24, Abhinav Kumar <[email protected]> wrote:
>>
>>
>>
>> On 12/8/2023 3:12 AM, Dmitry Baryshkov wrote:
>>> On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <[email protected]> wrote:
>>>>
>>>> Since the type and usage of CSC matrices is spanning across DPU
>>>> lets introduce a helper to the dpu_hw_util to return the CSC
>>>> corresponding to the request type. This will help to add more
>>>> supported CSC types such as the RGB to YUV one which is used in
>>>> the case of CDM.
>>>>
>>>> Signed-off-by: Abhinav Kumar <[email protected]>
>>>> ---
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 54 +++++++++++++++++++++
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 7 +++
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 39 ++-------------
>>>> 3 files changed, 64 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>>>> index 0b05061e3e62..59a153331194 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>>>> @@ -87,6 +87,60 @@ static u32 dpu_hw_util_log_mask = DPU_DBG_MASK_NONE;
>>>> #define QOS_QOS_CTRL_VBLANK_EN BIT(16)
>>>> #define QOS_QOS_CTRL_CREQ_VBLANK_MASK GENMASK(21, 20)
>>>>
>>>> +static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
>>>> + {
>>>> + /* S15.16 format */
>>>> + 0x00012A00, 0x00000000, 0x00019880,
>>>> + 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
>>>> + 0x00012A00, 0x00020480, 0x00000000,
>>>> + },
>>>> + /* signed bias */
>>>> + { 0xfff0, 0xff80, 0xff80,},
>>>> + { 0x0, 0x0, 0x0,},
>>>> + /* unsigned clamp */
>>>> + { 0x10, 0xeb, 0x10, 0xf0, 0x10, 0xf0,},
>>>> + { 0x00, 0xff, 0x00, 0xff, 0x00, 0xff,},
>>>> +};
>>>> +
>>>> +static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
>>>> + {
>>>> + /* S15.16 format */
>>>> + 0x00012A00, 0x00000000, 0x00019880,
>>>> + 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
>>>> + 0x00012A00, 0x00020480, 0x00000000,
>>>> + },
>>>> + /* signed bias */
>>>> + { 0xffc0, 0xfe00, 0xfe00,},
>>>> + { 0x0, 0x0, 0x0,},
>>>> + /* unsigned clamp */
>>>> + { 0x40, 0x3ac, 0x40, 0x3c0, 0x40, 0x3c0,},
>>>> + { 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
>>>> +};
>>>> +
>>>> +/**
>>>> + * dpu_hw_get_csc_cfg - get the CSC matrix based on the request type
>>>> + * @type: type of the requested CSC matrix from caller
>>>> + * Return: CSC matrix corresponding to the request type in DPU format
>>>> + */
>>>> +const struct dpu_csc_cfg *dpu_hw_get_csc_cfg(enum dpu_hw_csc_cfg_type type)
>>>> +{
>>>> + const struct dpu_csc_cfg *csc_cfg = NULL;
>>>> +
>>>> + switch (type) {
>>>> + case DPU_HW_YUV2RGB_601L:
>>>> + csc_cfg = &dpu_csc_YUV2RGB_601L;
>>>> + break;
>>>> + case DPU_HW_YUV2RGB_601L_10BIT:
>>>> + csc_cfg = &dpu_csc10_YUV2RGB_601L;
>>>> + break;
>>>> + default:
>>>> + DPU_ERROR("unknown csc_cfg type\n");
>>>> + break;
>>>> + }
>>>> +
>>>> + return csc_cfg;
>>>> +}
>>>> +
>>>> void dpu_reg_write(struct dpu_hw_blk_reg_map *c,
>>>> u32 reg_off,
>>>> u32 val,
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>>>> index fe083b2e5696..49f2bcf6de15 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>>>> @@ -19,6 +19,11 @@
>>>> #define MISR_CTRL_STATUS_CLEAR BIT(10)
>>>> #define MISR_CTRL_FREE_RUN_MASK BIT(31)
>>>>
>>>> +enum dpu_hw_csc_cfg_type {
>>>> + DPU_HW_YUV2RGB_601L,
>>>> + DPU_HW_YUV2RGB_601L_10BIT,
>>>> +};
>>>> +
>>>> /*
>>>> * This is the common struct maintained by each sub block
>>>> * for mapping the register offsets in this block to the
>>>> @@ -368,4 +373,6 @@ bool dpu_hw_clk_force_ctrl(struct dpu_hw_blk_reg_map *c,
>>>> const struct dpu_clk_ctrl_reg *clk_ctrl_reg,
>>>> bool enable);
>>>>
>>>> +const struct dpu_csc_cfg *dpu_hw_get_csc_cfg(enum dpu_hw_csc_cfg_type type);
>>>
>>> I don't think we need extra enum and wrapper. Just export const data
>>> structures directly.
>>>
>>
>> I liked this approach because the blocks of DPU such as plane and CDM
>> are clients to the dpu_hw_util and just request the type and the util
>> handles their request of returning the correct csc matrix.
>>
>> Do you see any issue with this?
>
> Not an issue, but I don't see anything that requires an extra
> abstraction. We perfectly know which CSC config we would like to get.
>
Correct, so the clients know which "type" of matrix they need and not
the matrix itself. That was the idea behind this.
>>
>>>> +
>>>> #endif /* _DPU_HW_UTIL_H */
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>> index 3235ab132540..31641889b9f0 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>> @@ -21,6 +21,7 @@
>>>> #include "dpu_kms.h"
>>>> #include "dpu_formats.h"
>>>> #include "dpu_hw_sspp.h"
>>>> +#include "dpu_hw_util.h"
>>>> #include "dpu_trace.h"
>>>> #include "dpu_crtc.h"
>>>> #include "dpu_vbif.h"
>>>> @@ -508,50 +509,16 @@ static void _dpu_plane_setup_pixel_ext(struct dpu_hw_scaler3_cfg *scale_cfg,
>>>> }
>>>> }
>>>>
>>>> -static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
>>>> - {
>>>> - /* S15.16 format */
>>>> - 0x00012A00, 0x00000000, 0x00019880,
>>>> - 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
>>>> - 0x00012A00, 0x00020480, 0x00000000,
>>>> - },
>>>> - /* signed bias */
>>>> - { 0xfff0, 0xff80, 0xff80,},
>>>> - { 0x0, 0x0, 0x0,},
>>>> - /* unsigned clamp */
>>>> - { 0x10, 0xeb, 0x10, 0xf0, 0x10, 0xf0,},
>>>> - { 0x00, 0xff, 0x00, 0xff, 0x00, 0xff,},
>>>> -};
>>>> -
>>>> -static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
>>>> - {
>>>> - /* S15.16 format */
>>>> - 0x00012A00, 0x00000000, 0x00019880,
>>>> - 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
>>>> - 0x00012A00, 0x00020480, 0x00000000,
>>>> - },
>>>> - /* signed bias */
>>>> - { 0xffc0, 0xfe00, 0xfe00,},
>>>> - { 0x0, 0x0, 0x0,},
>>>> - /* unsigned clamp */
>>>> - { 0x40, 0x3ac, 0x40, 0x3c0, 0x40, 0x3c0,},
>>>> - { 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
>>>> -};
>>>> -
>>>> static const struct dpu_csc_cfg *_dpu_plane_get_csc(struct dpu_sw_pipe *pipe,
>>>> const struct dpu_format *fmt)
>>>> {
>>>> - const struct dpu_csc_cfg *csc_ptr;
>>>> -
>>>> if (!DPU_FORMAT_IS_YUV(fmt))
>>>> return NULL;
>>>>
>>>> if (BIT(DPU_SSPP_CSC_10BIT) & pipe->sspp->cap->features)
>>>> - csc_ptr = &dpu_csc10_YUV2RGB_601L;
>>>> + return dpu_hw_get_csc_cfg(DPU_HW_YUV2RGB_601L_10BIT);
>>>> else
>>>> - csc_ptr = &dpu_csc_YUV2RGB_601L;
>>>> -
>>>> - return csc_ptr;
>>>> + return dpu_hw_get_csc_cfg(DPU_HW_YUV2RGB_601L);
>>>> }
>>>>
>>>> static void _dpu_plane_setup_scaler(struct dpu_sw_pipe *pipe,
>>>> --
>>>> 2.40.1
>>>>
>>>
>>>
>
>
>
On Fri, 8 Dec 2023 at 18:35, Abhinav Kumar <[email protected]> wrote:
>
>
>
> On 12/8/2023 8:27 AM, Dmitry Baryshkov wrote:
> > On Fri, 8 Dec 2023 at 18:24, Abhinav Kumar <[email protected]> wrote:
> >>
> >>
> >>
> >> On 12/8/2023 3:12 AM, Dmitry Baryshkov wrote:
> >>> On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <[email protected]> wrote:
> >>>>
> >>>> Since the type and usage of CSC matrices is spanning across DPU
> >>>> lets introduce a helper to the dpu_hw_util to return the CSC
> >>>> corresponding to the request type. This will help to add more
> >>>> supported CSC types such as the RGB to YUV one which is used in
> >>>> the case of CDM.
> >>>>
> >>>> Signed-off-by: Abhinav Kumar <[email protected]>
> >>>> ---
> >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 54 +++++++++++++++++++++
> >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 7 +++
> >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 39 ++-------------
> >>>> 3 files changed, 64 insertions(+), 36 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> >>>> index 0b05061e3e62..59a153331194 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> >>>> @@ -87,6 +87,60 @@ static u32 dpu_hw_util_log_mask = DPU_DBG_MASK_NONE;
> >>>> #define QOS_QOS_CTRL_VBLANK_EN BIT(16)
> >>>> #define QOS_QOS_CTRL_CREQ_VBLANK_MASK GENMASK(21, 20)
> >>>>
> >>>> +static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
> >>>> + {
> >>>> + /* S15.16 format */
> >>>> + 0x00012A00, 0x00000000, 0x00019880,
> >>>> + 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
> >>>> + 0x00012A00, 0x00020480, 0x00000000,
> >>>> + },
> >>>> + /* signed bias */
> >>>> + { 0xfff0, 0xff80, 0xff80,},
> >>>> + { 0x0, 0x0, 0x0,},
> >>>> + /* unsigned clamp */
> >>>> + { 0x10, 0xeb, 0x10, 0xf0, 0x10, 0xf0,},
> >>>> + { 0x00, 0xff, 0x00, 0xff, 0x00, 0xff,},
> >>>> +};
> >>>> +
> >>>> +static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
> >>>> + {
> >>>> + /* S15.16 format */
> >>>> + 0x00012A00, 0x00000000, 0x00019880,
> >>>> + 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
> >>>> + 0x00012A00, 0x00020480, 0x00000000,
> >>>> + },
> >>>> + /* signed bias */
> >>>> + { 0xffc0, 0xfe00, 0xfe00,},
> >>>> + { 0x0, 0x0, 0x0,},
> >>>> + /* unsigned clamp */
> >>>> + { 0x40, 0x3ac, 0x40, 0x3c0, 0x40, 0x3c0,},
> >>>> + { 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
> >>>> +};
> >>>> +
> >>>> +/**
> >>>> + * dpu_hw_get_csc_cfg - get the CSC matrix based on the request type
> >>>> + * @type: type of the requested CSC matrix from caller
> >>>> + * Return: CSC matrix corresponding to the request type in DPU format
> >>>> + */
> >>>> +const struct dpu_csc_cfg *dpu_hw_get_csc_cfg(enum dpu_hw_csc_cfg_type type)
> >>>> +{
> >>>> + const struct dpu_csc_cfg *csc_cfg = NULL;
> >>>> +
> >>>> + switch (type) {
> >>>> + case DPU_HW_YUV2RGB_601L:
> >>>> + csc_cfg = &dpu_csc_YUV2RGB_601L;
> >>>> + break;
> >>>> + case DPU_HW_YUV2RGB_601L_10BIT:
> >>>> + csc_cfg = &dpu_csc10_YUV2RGB_601L;
> >>>> + break;
> >>>> + default:
> >>>> + DPU_ERROR("unknown csc_cfg type\n");
> >>>> + break;
> >>>> + }
> >>>> +
> >>>> + return csc_cfg;
> >>>> +}
> >>>> +
> >>>> void dpu_reg_write(struct dpu_hw_blk_reg_map *c,
> >>>> u32 reg_off,
> >>>> u32 val,
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> >>>> index fe083b2e5696..49f2bcf6de15 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> >>>> @@ -19,6 +19,11 @@
> >>>> #define MISR_CTRL_STATUS_CLEAR BIT(10)
> >>>> #define MISR_CTRL_FREE_RUN_MASK BIT(31)
> >>>>
> >>>> +enum dpu_hw_csc_cfg_type {
> >>>> + DPU_HW_YUV2RGB_601L,
> >>>> + DPU_HW_YUV2RGB_601L_10BIT,
> >>>> +};
> >>>> +
> >>>> /*
> >>>> * This is the common struct maintained by each sub block
> >>>> * for mapping the register offsets in this block to the
> >>>> @@ -368,4 +373,6 @@ bool dpu_hw_clk_force_ctrl(struct dpu_hw_blk_reg_map *c,
> >>>> const struct dpu_clk_ctrl_reg *clk_ctrl_reg,
> >>>> bool enable);
> >>>>
> >>>> +const struct dpu_csc_cfg *dpu_hw_get_csc_cfg(enum dpu_hw_csc_cfg_type type);
> >>>
> >>> I don't think we need extra enum and wrapper. Just export const data
> >>> structures directly.
> >>>
> >>
> >> I liked this approach because the blocks of DPU such as plane and CDM
> >> are clients to the dpu_hw_util and just request the type and the util
> >> handles their request of returning the correct csc matrix.
> >>
> >> Do you see any issue with this?
> >
> > Not an issue, but I don't see anything that requires an extra
> > abstraction. We perfectly know which CSC config we would like to get.
> >
>
> Correct, so the clients know which "type" of matrix they need and not
> the matrix itself. That was the idea behind this.
I consider this to be an unnecessary abstraction. In our case, knowing
the type = knowing the address of the matrix. I don't foresee any
additional logic there.
>
> >>
> >>>> +
> >>>> #endif /* _DPU_HW_UTIL_H */
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >>>> index 3235ab132540..31641889b9f0 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >>>> @@ -21,6 +21,7 @@
> >>>> #include "dpu_kms.h"
> >>>> #include "dpu_formats.h"
> >>>> #include "dpu_hw_sspp.h"
> >>>> +#include "dpu_hw_util.h"
> >>>> #include "dpu_trace.h"
> >>>> #include "dpu_crtc.h"
> >>>> #include "dpu_vbif.h"
> >>>> @@ -508,50 +509,16 @@ static void _dpu_plane_setup_pixel_ext(struct dpu_hw_scaler3_cfg *scale_cfg,
> >>>> }
> >>>> }
> >>>>
> >>>> -static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
> >>>> - {
> >>>> - /* S15.16 format */
> >>>> - 0x00012A00, 0x00000000, 0x00019880,
> >>>> - 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
> >>>> - 0x00012A00, 0x00020480, 0x00000000,
> >>>> - },
> >>>> - /* signed bias */
> >>>> - { 0xfff0, 0xff80, 0xff80,},
> >>>> - { 0x0, 0x0, 0x0,},
> >>>> - /* unsigned clamp */
> >>>> - { 0x10, 0xeb, 0x10, 0xf0, 0x10, 0xf0,},
> >>>> - { 0x00, 0xff, 0x00, 0xff, 0x00, 0xff,},
> >>>> -};
> >>>> -
> >>>> -static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
> >>>> - {
> >>>> - /* S15.16 format */
> >>>> - 0x00012A00, 0x00000000, 0x00019880,
> >>>> - 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
> >>>> - 0x00012A00, 0x00020480, 0x00000000,
> >>>> - },
> >>>> - /* signed bias */
> >>>> - { 0xffc0, 0xfe00, 0xfe00,},
> >>>> - { 0x0, 0x0, 0x0,},
> >>>> - /* unsigned clamp */
> >>>> - { 0x40, 0x3ac, 0x40, 0x3c0, 0x40, 0x3c0,},
> >>>> - { 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
> >>>> -};
> >>>> -
> >>>> static const struct dpu_csc_cfg *_dpu_plane_get_csc(struct dpu_sw_pipe *pipe,
> >>>> const struct dpu_format *fmt)
> >>>> {
> >>>> - const struct dpu_csc_cfg *csc_ptr;
> >>>> -
> >>>> if (!DPU_FORMAT_IS_YUV(fmt))
> >>>> return NULL;
> >>>>
> >>>> if (BIT(DPU_SSPP_CSC_10BIT) & pipe->sspp->cap->features)
> >>>> - csc_ptr = &dpu_csc10_YUV2RGB_601L;
> >>>> + return dpu_hw_get_csc_cfg(DPU_HW_YUV2RGB_601L_10BIT);
> >>>> else
> >>>> - csc_ptr = &dpu_csc_YUV2RGB_601L;
> >>>> -
> >>>> - return csc_ptr;
> >>>> + return dpu_hw_get_csc_cfg(DPU_HW_YUV2RGB_601L);
> >>>> }
> >>>>
> >>>> static void _dpu_plane_setup_scaler(struct dpu_sw_pipe *pipe,
> >>>> --
> >>>> 2.40.1
> >>>>
> >>>
> >>>
> >
> >
> >
--
With best wishes
Dmitry