Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <5d4104ae-8737-6576-79ab-e68126bd826a@quicinc.com>
Date:   Fri, 8 Dec 2023 08:35:22 -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>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <dri-devel@lists.freedesktop.org>,
        <quic_parellan@quicinc.com>, <quic_jesszhan@quicinc.com>,
        Marijn Suijten <marijn.suijten@somainline.org>,
        Sean Paul <sean@poorly.run>
References: <20231208050641.32582-1-quic_abhinavk@quicinc.com>
 <20231208050641.32582-5-quic_abhinavk@quicinc.com>
 <CAA8EJpro4ehwZ8Gs+ASUmrMJS0HbnDXd5pXwijs1jkDpR-kAOQ@mail.gmail.com>
 <ceb7e736-22f3-0cf3-3d65-7ec33e7c9d95@quicinc.com>
 <CAA8EJprr=T1cNst_dTNSToW_fVOM3mo-yRmWuK=8BU5rDNd54Q@mail.gmail.com>
From:   Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <CAA8EJprr=T1cNst_dTNSToW_fVOM3mo-yRmWuK=8BU5rDNd54Q@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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