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

From: Dmitry Baryshkov
Date: Fri Dec 08 2023 - 11:40:35 EST

On Fri, 8 Dec 2023 at 18:35, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> 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.

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