Re: [PATCH v2 04/16] drm/msm/dpu: move csc matrices to dpu_hw_util

Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> · Fri, 8 Dec 2023 08:35:22 -0800

On 12/8/2023 8:27 AM, Dmitry Baryshkov wrote:
On Fri, 8 Dec 2023 at 18:24, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:



On 12/8/2023 3:12 AM, Dmitry Baryshkov wrote:
On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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 <quic_abhinavk@xxxxxxxxxxx>
---
   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












