diff mbox series

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

Message ID	20231208050641.32582-5-quic_abhinavk@quicinc.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34; From: Abhinav Kumar <quic_abhinavk@quicinc.com> To: <freedreno@lists.freedesktop.org>, Rob Clark <robdclark@gmail.com>, Abhinav Kumar <quic_abhinavk@quicinc.com>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Sean Paul <sean@poorly.run>, Marijn Suijten <marijn.suijten@somainline.org>, David Airlie <airlied@gmail.com>, "Daniel Vetter" <daniel@ffwll.ch> CC: <dri-devel@lists.freedesktop.org>, <quic_jesszhan@quicinc.com>, <quic_parellan@quicinc.com>, <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org> Subject: [PATCH v2 04/16] drm/msm/dpu: move csc matrices to dpu_hw_util Date: Thu, 7 Dec 2023 21:06:29 -0800 Message-ID: <20231208050641.32582-5-quic_abhinavk@quicinc.com> In-Reply-To: <20231208050641.32582-1-quic_abhinavk@quicinc.com> References: <20231208050641.32582-1-quic_abhinavk@quicinc.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain Precedence: bulk
Series	[v2,01/16] drm/msm/dpu: add formats check for writeback encoder \| [v2,01/16] drm/msm/dpu: add formats check for writeback encoder [v2,02/16] drm/msm/dpu: rename dpu_encoder_phys_wb_setup_cdp to match its functionality [v2,03/16] drm/msm/dpu: fix writeback programming for YUV cases [v2,04/16] drm/msm/dpu: move csc matrices to dpu_hw_util [v2,05/16] drm/msm/dpu: add cdm blocks to sc7280 dpu_hw_catalog [v2,06/16] drm/msm/dpu: add cdm blocks to sm8250 dpu_hw_catalog [v2,07/16] drm/msm/dpu: add dpu_hw_cdm abstraction for CDM block [v2,08/16] drm/msm/dpu: add cdm blocks to RM [v2,09/16] drm/msm/dpu: add support to allocate CDM from RM [v2,10/16] drm/msm/dpu: add CDM related logic to dpu_hw_ctl layer [v2,11/16] drm/msm/dpu: add support to disable CDM block during encoder cleanup [v2,12/16] drm/msm/dpu: add an API to setup the CDM block for writeback [v2,13/16] drm/msm/dpu: plug-in the cdm related bits to writeback setup [v2,14/16] drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output [v2,15/16] drm/msm/dpu: introduce separate wb2_format arrays for rgb and yuv [v2,16/16] drm/msm/dpu: add cdm blocks to dpu snapshot

Commit Message

Abhinav Kumar Dec. 8, 2023, 5:06 a.m. UTC

  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@quicinc.com>
---
 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(-)

Comments

Dmitry Baryshkov Dec. 8, 2023, 11:12 a.m. UTC | #1

On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> 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@quicinc.com>
> ---
>  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
>

Abhinav Kumar Dec. 8, 2023, 4:24 p.m. UTC | #2

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

Dmitry Baryshkov Dec. 8, 2023, 4:27 p.m. UTC | #3

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

Abhinav Kumar Dec. 8, 2023, 4:35 p.m. UTC | #4

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

Dmitry Baryshkov Dec. 8, 2023, 4:40 p.m. UTC | #5

On Fri, 8 Dec 2023 at 18:35, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 12/8/2023 8:27 AM, Dmitry Baryshkov wrote:
> > On Fri, 8 Dec 2023 at 18:24, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 12/8/2023 3:12 AM, Dmitry Baryshkov wrote:
> >>> On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> 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@quicinc.com>
> >>>> ---
> >>>>    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

diff mbox series

Patch

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,