Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <ceb7e736-22f3-0cf3-3d65-7ec33e7c9d95@quicinc.com>
Date:   Fri, 8 Dec 2023 08:24:15 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v2 04/16] drm/msm/dpu: move csc matrices to dpu_hw_util
Content-Language: en-US
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC:     <freedreno@lists.freedesktop.org>, Rob Clark <robdclark@gmail.com>,
        "Sean Paul" <sean@poorly.run>,
        Marijn Suijten <marijn.suijten@somainline.org>,
        "David Airlie" <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        <dri-devel@lists.freedesktop.org>, <quic_jesszhan@quicinc.com>,
        <quic_parellan@quicinc.com>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20231208050641.32582-1-quic_abhinavk@quicinc.com>
 <20231208050641.32582-5-quic_abhinavk@quicinc.com>
 <CAA8EJpro4ehwZ8Gs+ASUmrMJS0HbnDXd5pXwijs1jkDpR-kAOQ@mail.gmail.com>
From:   Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <CAA8EJpro4ehwZ8Gs+ASUmrMJS0HbnDXd5pXwijs1jkDpR-kAOQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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
>>
> 
>