diff mbox series

[v2,05/16] drm/msm/dpu: add cdm blocks to sc7280 dpu_hw_catalog

Message ID 20231208050641.32582-6-quic_abhinavk@quicinc.com
State New
Headers
Series [v2,01/16] drm/msm/dpu: add formats check for writeback encoder |

Commit Message

Abhinav Kumar Dec. 8, 2023, 5:06 a.m. UTC 
  Add CDM blocks to the sc7280 dpu_hw_catalog to support
YUV format output from writeback block.

changes in v2:
	- remove explicit zero assignment for features
	- move sc7280_cdm to dpu_hw_catalog from the sc7280
	  catalog file as its definition can be re-used

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h  |  1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c      | 10 ++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h      | 13 +++++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h         |  5 +++++
 4 files changed, 29 insertions(+)

Comments

Dmitry Baryshkov Dec. 8, 2023, 11:13 a.m. UTC | #1 
On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Add CDM blocks to the sc7280 dpu_hw_catalog to support
> YUV format output from writeback block.
>
> changes in v2:
>         - remove explicit zero assignment for features
>         - move sc7280_cdm to dpu_hw_catalog from the sc7280
>           catalog file as its definition can be re-used
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h  |  1 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c      | 10 ++++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h      | 13 +++++++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h         |  5 +++++
>  4 files changed, 29 insertions(+)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Dmitry Baryshkov Dec. 8, 2023, 11:19 a.m. UTC | #2 
On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Add CDM blocks to the sc7280 dpu_hw_catalog to support
> YUV format output from writeback block.
>
> changes in v2:
>         - remove explicit zero assignment for features
>         - move sc7280_cdm to dpu_hw_catalog from the sc7280
>           catalog file as its definition can be re-used
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h  |  1 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c      | 10 ++++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h      | 13 +++++++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h         |  5 +++++
>  4 files changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> index 209675de6742..19c2b7454796 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> @@ -248,6 +248,7 @@ const struct dpu_mdss_cfg dpu_sc7280_cfg = {
>         .mdss_ver = &sc7280_mdss_ver,
>         .caps = &sc7280_dpu_caps,
>         .mdp = &sc7280_mdp,
> +       .cdm = &sc7280_cdm,
>         .ctl_count = ARRAY_SIZE(sc7280_ctl),
>         .ctl = sc7280_ctl,
>         .sspp_count = ARRAY_SIZE(sc7280_sspp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index d52aae54bbd5..1be3156cde05 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -426,6 +426,16 @@ static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
>         .ctl = {.name = "ctl", .base = 0xF80, .len = 0x10},
>  };
>
> +/*************************************************************
> + * CDM sub block config

Nit: it is not a subblock config.

> + *************************************************************/
> +static const struct dpu_cdm_cfg sc7280_cdm = {

I know that I have r-b'ed this patch. But then one thing occurred to
me. If this definition is common to all (or almost all) platforms, can
we just call it dpu_cdm or dpu_common_cdm?

> +       .name = "cdm_0",
> +       .id = CDM_0,
> +       .len = 0x228,
> +       .base = 0x79200,
> +};
> +
>  /*************************************************************
>   * VBIF sub blocks config
>   *************************************************************/
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> index e3c0d007481b..ba82ef4560a6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -682,6 +682,17 @@ struct dpu_vbif_cfg {
>         u32 memtype[MAX_XIN_COUNT];
>  };
>
> +/**
> + * struct dpu_cdm_cfg - information of chroma down blocks
> + * @name               string name for debug purposes
> + * @id                 enum identifying this block
> + * @base               register offset of this block
> + * @features           bit mask identifying sub-blocks/features
> + */
> +struct dpu_cdm_cfg {
> +       DPU_HW_BLK_INFO;
> +};
> +
>  /**
>   * Define CDP use cases
>   * @DPU_PERF_CDP_UDAGE_RT: real-time use cases
> @@ -805,6 +816,8 @@ struct dpu_mdss_cfg {
>         u32 wb_count;
>         const struct dpu_wb_cfg *wb;
>
> +       const struct dpu_cdm_cfg *cdm;
> +
>         u32 ad_count;
>
>         u32 dspp_count;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> index a6702b2bfc68..f319c8232ea5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> @@ -185,6 +185,11 @@ enum dpu_dsc {
>         DSC_MAX
>  };
>
> +enum dpu_cdm {
> +       CDM_0 = 1,
> +       CDM_MAX
> +};
> +
>  enum dpu_pingpong {
>         PINGPONG_NONE,
>         PINGPONG_0,
> --
> 2.40.1
>
Abhinav Kumar Dec. 11, 2023, 9:16 p.m. UTC | #3 
On 12/8/2023 3:19 AM, Dmitry Baryshkov wrote:
> On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Add CDM blocks to the sc7280 dpu_hw_catalog to support
>> YUV format output from writeback block.
>>
>> changes in v2:
>>          - remove explicit zero assignment for features
>>          - move sc7280_cdm to dpu_hw_catalog from the sc7280
>>            catalog file as its definition can be re-used
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h  |  1 +
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c      | 10 ++++++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h      | 13 +++++++++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h         |  5 +++++
>>   4 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> index 209675de6742..19c2b7454796 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> @@ -248,6 +248,7 @@ const struct dpu_mdss_cfg dpu_sc7280_cfg = {
>>          .mdss_ver = &sc7280_mdss_ver,
>>          .caps = &sc7280_dpu_caps,
>>          .mdp = &sc7280_mdp,
>> +       .cdm = &sc7280_cdm,
>>          .ctl_count = ARRAY_SIZE(sc7280_ctl),
>>          .ctl = sc7280_ctl,
>>          .sspp_count = ARRAY_SIZE(sc7280_sspp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> index d52aae54bbd5..1be3156cde05 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -426,6 +426,16 @@ static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
>>          .ctl = {.name = "ctl", .base = 0xF80, .len = 0x10},
>>   };
>>
>> +/*************************************************************
>> + * CDM sub block config
> 
> Nit: it is not a subblock config.
> 

Ack.

>> + *************************************************************/
>> +static const struct dpu_cdm_cfg sc7280_cdm = {
> 
> I know that I have r-b'ed this patch. But then one thing occurred to
> me. If this definition is common to all (or almost all) platforms, can
> we just call it dpu_cdm or dpu_common_cdm?
> 
>> +       .name = "cdm_0",
>> +       .id = CDM_0,
>> +       .len = 0x228,
>> +       .base = 0x79200,
>> +};

hmmm .... almost common but not entirely ... msm8998's CDM has a shorter 
len of 0x224 :(

>> +
>>   /*************************************************************
>>    * VBIF sub blocks config
>>    *************************************************************/
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> index e3c0d007481b..ba82ef4560a6 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -682,6 +682,17 @@ struct dpu_vbif_cfg {
>>          u32 memtype[MAX_XIN_COUNT];
>>   };
>>
>> +/**
>> + * struct dpu_cdm_cfg - information of chroma down blocks
>> + * @name               string name for debug purposes
>> + * @id                 enum identifying this block
>> + * @base               register offset of this block
>> + * @features           bit mask identifying sub-blocks/features
>> + */
>> +struct dpu_cdm_cfg {
>> +       DPU_HW_BLK_INFO;
>> +};
>> +
>>   /**
>>    * Define CDP use cases
>>    * @DPU_PERF_CDP_UDAGE_RT: real-time use cases
>> @@ -805,6 +816,8 @@ struct dpu_mdss_cfg {
>>          u32 wb_count;
>>          const struct dpu_wb_cfg *wb;
>>
>> +       const struct dpu_cdm_cfg *cdm;
>> +
>>          u32 ad_count;
>>
>>          u32 dspp_count;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>> index a6702b2bfc68..f319c8232ea5 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>> @@ -185,6 +185,11 @@ enum dpu_dsc {
>>          DSC_MAX
>>   };
>>
>> +enum dpu_cdm {
>> +       CDM_0 = 1,
>> +       CDM_MAX
>> +};
>> +
>>   enum dpu_pingpong {
>>          PINGPONG_NONE,
>>          PINGPONG_0,
>> --
>> 2.40.1
>>
> 
>
Dmitry Baryshkov Dec. 11, 2023, 9:31 p.m. UTC | #4 
On Mon, 11 Dec 2023 at 23:16, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 12/8/2023 3:19 AM, Dmitry Baryshkov wrote:
> > On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >> Add CDM blocks to the sc7280 dpu_hw_catalog to support
> >> YUV format output from writeback block.
> >>
> >> changes in v2:
> >>          - remove explicit zero assignment for features
> >>          - move sc7280_cdm to dpu_hw_catalog from the sc7280
> >>            catalog file as its definition can be re-used
> >>
> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >> ---
> >>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h  |  1 +
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c      | 10 ++++++++++
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h      | 13 +++++++++++++
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h         |  5 +++++
> >>   4 files changed, 29 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> >> index 209675de6742..19c2b7454796 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> >> @@ -248,6 +248,7 @@ const struct dpu_mdss_cfg dpu_sc7280_cfg = {
> >>          .mdss_ver = &sc7280_mdss_ver,
> >>          .caps = &sc7280_dpu_caps,
> >>          .mdp = &sc7280_mdp,
> >> +       .cdm = &sc7280_cdm,
> >>          .ctl_count = ARRAY_SIZE(sc7280_ctl),
> >>          .ctl = sc7280_ctl,
> >>          .sspp_count = ARRAY_SIZE(sc7280_sspp),
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> index d52aae54bbd5..1be3156cde05 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> @@ -426,6 +426,16 @@ static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
> >>          .ctl = {.name = "ctl", .base = 0xF80, .len = 0x10},
> >>   };
> >>
> >> +/*************************************************************
> >> + * CDM sub block config
> >
> > Nit: it is not a subblock config.
> >
>
> Ack.
>
> >> + *************************************************************/
> >> +static const struct dpu_cdm_cfg sc7280_cdm = {
> >
> > I know that I have r-b'ed this patch. But then one thing occurred to
> > me. If this definition is common to all (or almost all) platforms, can
> > we just call it dpu_cdm or dpu_common_cdm?
> >
> >> +       .name = "cdm_0",
> >> +       .id = CDM_0,
> >> +       .len = 0x228,
> >> +       .base = 0x79200,
> >> +};
>
> hmmm .... almost common but not entirely ... msm8998's CDM has a shorter
> len of 0x224 :(

Then sdm845_cdm?

>
> >> +
> >>   /*************************************************************
> >>    * VBIF sub blocks config
> >>    *************************************************************/
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >> index e3c0d007481b..ba82ef4560a6 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >> @@ -682,6 +682,17 @@ struct dpu_vbif_cfg {
> >>          u32 memtype[MAX_XIN_COUNT];
> >>   };
> >>
> >> +/**
> >> + * struct dpu_cdm_cfg - information of chroma down blocks
> >> + * @name               string name for debug purposes
> >> + * @id                 enum identifying this block
> >> + * @base               register offset of this block
> >> + * @features           bit mask identifying sub-blocks/features
> >> + */
> >> +struct dpu_cdm_cfg {
> >> +       DPU_HW_BLK_INFO;
> >> +};
> >> +
> >>   /**
> >>    * Define CDP use cases
> >>    * @DPU_PERF_CDP_UDAGE_RT: real-time use cases
> >> @@ -805,6 +816,8 @@ struct dpu_mdss_cfg {
> >>          u32 wb_count;
> >>          const struct dpu_wb_cfg *wb;
> >>
> >> +       const struct dpu_cdm_cfg *cdm;
> >> +
> >>          u32 ad_count;
> >>
> >>          u32 dspp_count;
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> >> index a6702b2bfc68..f319c8232ea5 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> >> @@ -185,6 +185,11 @@ enum dpu_dsc {
> >>          DSC_MAX
> >>   };
> >>
> >> +enum dpu_cdm {
> >> +       CDM_0 = 1,
> >> +       CDM_MAX
> >> +};
> >> +
> >>   enum dpu_pingpong {
> >>          PINGPONG_NONE,
> >>          PINGPONG_0,
> >> --
> >> 2.40.1
> >>
> >
> >
Abhinav Kumar Dec. 11, 2023, 9:32 p.m. UTC | #5 
On 12/11/2023 1:31 PM, Dmitry Baryshkov wrote:
> On Mon, 11 Dec 2023 at 23:16, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 12/8/2023 3:19 AM, Dmitry Baryshkov wrote:
>>> On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>> Add CDM blocks to the sc7280 dpu_hw_catalog to support
>>>> YUV format output from writeback block.
>>>>
>>>> changes in v2:
>>>>           - remove explicit zero assignment for features
>>>>           - move sc7280_cdm to dpu_hw_catalog from the sc7280
>>>>             catalog file as its definition can be re-used
>>>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>>    .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h  |  1 +
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c      | 10 ++++++++++
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h      | 13 +++++++++++++
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h         |  5 +++++
>>>>    4 files changed, 29 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>>>> index 209675de6742..19c2b7454796 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>>>> @@ -248,6 +248,7 @@ const struct dpu_mdss_cfg dpu_sc7280_cfg = {
>>>>           .mdss_ver = &sc7280_mdss_ver,
>>>>           .caps = &sc7280_dpu_caps,
>>>>           .mdp = &sc7280_mdp,
>>>> +       .cdm = &sc7280_cdm,
>>>>           .ctl_count = ARRAY_SIZE(sc7280_ctl),
>>>>           .ctl = sc7280_ctl,
>>>>           .sspp_count = ARRAY_SIZE(sc7280_sspp),
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> index d52aae54bbd5..1be3156cde05 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> @@ -426,6 +426,16 @@ static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
>>>>           .ctl = {.name = "ctl", .base = 0xF80, .len = 0x10},
>>>>    };
>>>>
>>>> +/*************************************************************
>>>> + * CDM sub block config
>>>
>>> Nit: it is not a subblock config.
>>>
>>
>> Ack.
>>
>>>> + *************************************************************/
>>>> +static const struct dpu_cdm_cfg sc7280_cdm = {
>>>
>>> I know that I have r-b'ed this patch. But then one thing occurred to
>>> me. If this definition is common to all (or almost all) platforms, can
>>> we just call it dpu_cdm or dpu_common_cdm?
>>>
>>>> +       .name = "cdm_0",
>>>> +       .id = CDM_0,
>>>> +       .len = 0x228,
>>>> +       .base = 0x79200,
>>>> +};
>>
>> hmmm .... almost common but not entirely ... msm8998's CDM has a shorter
>> len of 0x224 :(
> 
> Then sdm845_cdm?
> 

That also has a shorter cdm length.

BTW, sdm845 is not in this series. It will be part of RFT as we discussed.

>>
>>>> +
>>>>    /*************************************************************
>>>>     * VBIF sub blocks config
>>>>     *************************************************************/
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>> index e3c0d007481b..ba82ef4560a6 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>> @@ -682,6 +682,17 @@ struct dpu_vbif_cfg {
>>>>           u32 memtype[MAX_XIN_COUNT];
>>>>    };
>>>>
>>>> +/**
>>>> + * struct dpu_cdm_cfg - information of chroma down blocks
>>>> + * @name               string name for debug purposes
>>>> + * @id                 enum identifying this block
>>>> + * @base               register offset of this block
>>>> + * @features           bit mask identifying sub-blocks/features
>>>> + */
>>>> +struct dpu_cdm_cfg {
>>>> +       DPU_HW_BLK_INFO;
>>>> +};
>>>> +
>>>>    /**
>>>>     * Define CDP use cases
>>>>     * @DPU_PERF_CDP_UDAGE_RT: real-time use cases
>>>> @@ -805,6 +816,8 @@ struct dpu_mdss_cfg {
>>>>           u32 wb_count;
>>>>           const struct dpu_wb_cfg *wb;
>>>>
>>>> +       const struct dpu_cdm_cfg *cdm;
>>>> +
>>>>           u32 ad_count;
>>>>
>>>>           u32 dspp_count;
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>>>> index a6702b2bfc68..f319c8232ea5 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>>>> @@ -185,6 +185,11 @@ enum dpu_dsc {
>>>>           DSC_MAX
>>>>    };
>>>>
>>>> +enum dpu_cdm {
>>>> +       CDM_0 = 1,
>>>> +       CDM_MAX
>>>> +};
>>>> +
>>>>    enum dpu_pingpong {
>>>>           PINGPONG_NONE,
>>>>           PINGPONG_0,
>>>> --
>>>> 2.40.1
>>>>
>>>
>>>
> 
> 
>
Dmitry Baryshkov Dec. 11, 2023, 9:42 p.m. UTC | #6 
On Mon, 11 Dec 2023 at 23:32, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 12/11/2023 1:31 PM, Dmitry Baryshkov wrote:
> > On Mon, 11 Dec 2023 at 23:16, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 12/8/2023 3:19 AM, Dmitry Baryshkov wrote:
> >>> On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>
> >>>> Add CDM blocks to the sc7280 dpu_hw_catalog to support
> >>>> YUV format output from writeback block.
> >>>>
> >>>> changes in v2:
> >>>>           - remove explicit zero assignment for features
> >>>>           - move sc7280_cdm to dpu_hw_catalog from the sc7280
> >>>>             catalog file as its definition can be re-used
> >>>>
> >>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >>>> ---
> >>>>    .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h  |  1 +
> >>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c      | 10 ++++++++++
> >>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h      | 13 +++++++++++++
> >>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h         |  5 +++++
> >>>>    4 files changed, 29 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> >>>> index 209675de6742..19c2b7454796 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> >>>> @@ -248,6 +248,7 @@ const struct dpu_mdss_cfg dpu_sc7280_cfg = {
> >>>>           .mdss_ver = &sc7280_mdss_ver,
> >>>>           .caps = &sc7280_dpu_caps,
> >>>>           .mdp = &sc7280_mdp,
> >>>> +       .cdm = &sc7280_cdm,
> >>>>           .ctl_count = ARRAY_SIZE(sc7280_ctl),
> >>>>           .ctl = sc7280_ctl,
> >>>>           .sspp_count = ARRAY_SIZE(sc7280_sspp),
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>>> index d52aae54bbd5..1be3156cde05 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>>> @@ -426,6 +426,16 @@ static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
> >>>>           .ctl = {.name = "ctl", .base = 0xF80, .len = 0x10},
> >>>>    };
> >>>>
> >>>> +/*************************************************************
> >>>> + * CDM sub block config
> >>>
> >>> Nit: it is not a subblock config.
> >>>
> >>
> >> Ack.
> >>
> >>>> + *************************************************************/
> >>>> +static const struct dpu_cdm_cfg sc7280_cdm = {
> >>>
> >>> I know that I have r-b'ed this patch. But then one thing occurred to
> >>> me. If this definition is common to all (or almost all) platforms, can
> >>> we just call it dpu_cdm or dpu_common_cdm?
> >>>
> >>>> +       .name = "cdm_0",
> >>>> +       .id = CDM_0,
> >>>> +       .len = 0x228,
> >>>> +       .base = 0x79200,
> >>>> +};
> >>
> >> hmmm .... almost common but not entirely ... msm8998's CDM has a shorter
> >> len of 0x224 :(
> >
> > Then sdm845_cdm?
> >
>
> That also has a shorter cdm length.

Could you please clarify. According to the downstream DT files all CDM
blocks up to (but not including) sm8550 had length 0x224. SM8550 and
SM8650 got qcom,sde-cdm-size = <0x220>.  But I don't see any registers
after 0x204.
>
> BTW, sdm845 is not in this series. It will be part of RFT as we discussed.
>
> >>
> >>>> +
> >>>>    /*************************************************************
> >>>>     * VBIF sub blocks config
> >>>>     *************************************************************/
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >>>> index e3c0d007481b..ba82ef4560a6 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >>>> @@ -682,6 +682,17 @@ struct dpu_vbif_cfg {
> >>>>           u32 memtype[MAX_XIN_COUNT];
> >>>>    };
> >>>>
> >>>> +/**
> >>>> + * struct dpu_cdm_cfg - information of chroma down blocks
> >>>> + * @name               string name for debug purposes
> >>>> + * @id                 enum identifying this block
> >>>> + * @base               register offset of this block
> >>>> + * @features           bit mask identifying sub-blocks/features
> >>>> + */
> >>>> +struct dpu_cdm_cfg {
> >>>> +       DPU_HW_BLK_INFO;
> >>>> +};
> >>>> +
> >>>>    /**
> >>>>     * Define CDP use cases
> >>>>     * @DPU_PERF_CDP_UDAGE_RT: real-time use cases
> >>>> @@ -805,6 +816,8 @@ struct dpu_mdss_cfg {
> >>>>           u32 wb_count;
> >>>>           const struct dpu_wb_cfg *wb;
> >>>>
> >>>> +       const struct dpu_cdm_cfg *cdm;
> >>>> +
> >>>>           u32 ad_count;
> >>>>
> >>>>           u32 dspp_count;
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> >>>> index a6702b2bfc68..f319c8232ea5 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> >>>> @@ -185,6 +185,11 @@ enum dpu_dsc {
> >>>>           DSC_MAX
> >>>>    };
> >>>>
> >>>> +enum dpu_cdm {
> >>>> +       CDM_0 = 1,
> >>>> +       CDM_MAX
> >>>> +};
> >>>> +
> >>>>    enum dpu_pingpong {
> >>>>           PINGPONG_NONE,
> >>>>           PINGPONG_0,
> >>>> --
> >>>> 2.40.1
> >>>>
> >>>
> >>>
> >
> >
> >
Abhinav Kumar Dec. 11, 2023, 9:48 p.m. UTC | #7 
On 12/11/2023 1:42 PM, Dmitry Baryshkov wrote:
> On Mon, 11 Dec 2023 at 23:32, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 12/11/2023 1:31 PM, Dmitry Baryshkov wrote:
>>> On Mon, 11 Dec 2023 at 23:16, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 12/8/2023 3:19 AM, Dmitry Baryshkov wrote:
>>>>> On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>>>
>>>>>> Add CDM blocks to the sc7280 dpu_hw_catalog to support
>>>>>> YUV format output from writeback block.
>>>>>>
>>>>>> changes in v2:
>>>>>>            - remove explicit zero assignment for features
>>>>>>            - move sc7280_cdm to dpu_hw_catalog from the sc7280
>>>>>>              catalog file as its definition can be re-used
>>>>>>
>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>> ---
>>>>>>     .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h  |  1 +
>>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c      | 10 ++++++++++
>>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h      | 13 +++++++++++++
>>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h         |  5 +++++
>>>>>>     4 files changed, 29 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>>>>>> index 209675de6742..19c2b7454796 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>>>>>> @@ -248,6 +248,7 @@ const struct dpu_mdss_cfg dpu_sc7280_cfg = {
>>>>>>            .mdss_ver = &sc7280_mdss_ver,
>>>>>>            .caps = &sc7280_dpu_caps,
>>>>>>            .mdp = &sc7280_mdp,
>>>>>> +       .cdm = &sc7280_cdm,
>>>>>>            .ctl_count = ARRAY_SIZE(sc7280_ctl),
>>>>>>            .ctl = sc7280_ctl,
>>>>>>            .sspp_count = ARRAY_SIZE(sc7280_sspp),
>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>> index d52aae54bbd5..1be3156cde05 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>> @@ -426,6 +426,16 @@ static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
>>>>>>            .ctl = {.name = "ctl", .base = 0xF80, .len = 0x10},
>>>>>>     };
>>>>>>
>>>>>> +/*************************************************************
>>>>>> + * CDM sub block config
>>>>>
>>>>> Nit: it is not a subblock config.
>>>>>
>>>>
>>>> Ack.
>>>>
>>>>>> + *************************************************************/
>>>>>> +static const struct dpu_cdm_cfg sc7280_cdm = {
>>>>>
>>>>> I know that I have r-b'ed this patch. But then one thing occurred to
>>>>> me. If this definition is common to all (or almost all) platforms, can
>>>>> we just call it dpu_cdm or dpu_common_cdm?
>>>>>
>>>>>> +       .name = "cdm_0",
>>>>>> +       .id = CDM_0,
>>>>>> +       .len = 0x228,
>>>>>> +       .base = 0x79200,
>>>>>> +};
>>>>
>>>> hmmm .... almost common but not entirely ... msm8998's CDM has a shorter
>>>> len of 0x224 :(
>>>
>>> Then sdm845_cdm?
>>>
>>
>> That also has a shorter cdm length.
> 
> Could you please clarify. According to the downstream DT files all CDM
> blocks up to (but not including) sm8550 had length 0x224. SM8550 and
> SM8650 got qcom,sde-cdm-size = <0x220>.  But I don't see any registers
> after 0x204.
>>

We always list 0x4 more than the last offset.

In chipsets sdm845 and msm8998, I only see the last offset of CDM as 
0x220 but in sm8250 and sc7280, the last offset is 0x224. Hence the 
total length is more in sc7280/sm8250 as compared to sdm845/msm8998.

I didnt follow that you do not see any registers after 0x204.

The CDM_MUX is the last offset which has an offset 0x224 from the base 
address. So thats the last offset.

The newer chipsets have CDM_MUX and the older ones did not. Hence the 
difference in length.

>> BTW, sdm845 is not in this series. It will be part of RFT as we discussed.
>>
>>>>
>>>>>> +
>>>>>>     /*************************************************************
>>>>>>      * VBIF sub blocks config
>>>>>>      *************************************************************/
>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>> index e3c0d007481b..ba82ef4560a6 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>> @@ -682,6 +682,17 @@ struct dpu_vbif_cfg {
>>>>>>            u32 memtype[MAX_XIN_COUNT];
>>>>>>     };
>>>>>>
>>>>>> +/**
>>>>>> + * struct dpu_cdm_cfg - information of chroma down blocks
>>>>>> + * @name               string name for debug purposes
>>>>>> + * @id                 enum identifying this block
>>>>>> + * @base               register offset of this block
>>>>>> + * @features           bit mask identifying sub-blocks/features
>>>>>> + */
>>>>>> +struct dpu_cdm_cfg {
>>>>>> +       DPU_HW_BLK_INFO;
>>>>>> +};
>>>>>> +
>>>>>>     /**
>>>>>>      * Define CDP use cases
>>>>>>      * @DPU_PERF_CDP_UDAGE_RT: real-time use cases
>>>>>> @@ -805,6 +816,8 @@ struct dpu_mdss_cfg {
>>>>>>            u32 wb_count;
>>>>>>            const struct dpu_wb_cfg *wb;
>>>>>>
>>>>>> +       const struct dpu_cdm_cfg *cdm;
>>>>>> +
>>>>>>            u32 ad_count;
>>>>>>
>>>>>>            u32 dspp_count;
>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>>>>>> index a6702b2bfc68..f319c8232ea5 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>>>>>> @@ -185,6 +185,11 @@ enum dpu_dsc {
>>>>>>            DSC_MAX
>>>>>>     };
>>>>>>
>>>>>> +enum dpu_cdm {
>>>>>> +       CDM_0 = 1,
>>>>>> +       CDM_MAX
>>>>>> +};
>>>>>> +
>>>>>>     enum dpu_pingpong {
>>>>>>            PINGPONG_NONE,
>>>>>>            PINGPONG_0,
>>>>>> --
>>>>>> 2.40.1
>>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
> 
> 
>
Dmitry Baryshkov Dec. 12, 2023, 6:49 a.m. UTC | #8 
On Mon, 11 Dec 2023 at 23:48, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 12/11/2023 1:42 PM, Dmitry Baryshkov wrote:
> > On Mon, 11 Dec 2023 at 23:32, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 12/11/2023 1:31 PM, Dmitry Baryshkov wrote:
> >>> On Mon, 11 Dec 2023 at 23:16, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 12/8/2023 3:19 AM, Dmitry Baryshkov wrote:
> >>>>> On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>>>
> >>>>>> Add CDM blocks to the sc7280 dpu_hw_catalog to support
> >>>>>> YUV format output from writeback block.
> >>>>>>
> >>>>>> changes in v2:
> >>>>>>            - remove explicit zero assignment for features
> >>>>>>            - move sc7280_cdm to dpu_hw_catalog from the sc7280
> >>>>>>              catalog file as its definition can be re-used
> >>>>>>
> >>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >>>>>> ---
> >>>>>>     .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h  |  1 +
> >>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c      | 10 ++++++++++
> >>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h      | 13 +++++++++++++
> >>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h         |  5 +++++
> >>>>>>     4 files changed, 29 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> >>>>>> index 209675de6742..19c2b7454796 100644
> >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> >>>>>> @@ -248,6 +248,7 @@ const struct dpu_mdss_cfg dpu_sc7280_cfg = {
> >>>>>>            .mdss_ver = &sc7280_mdss_ver,
> >>>>>>            .caps = &sc7280_dpu_caps,
> >>>>>>            .mdp = &sc7280_mdp,
> >>>>>> +       .cdm = &sc7280_cdm,
> >>>>>>            .ctl_count = ARRAY_SIZE(sc7280_ctl),
> >>>>>>            .ctl = sc7280_ctl,
> >>>>>>            .sspp_count = ARRAY_SIZE(sc7280_sspp),
> >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>>>>> index d52aae54bbd5..1be3156cde05 100644
> >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>>>>> @@ -426,6 +426,16 @@ static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
> >>>>>>            .ctl = {.name = "ctl", .base = 0xF80, .len = 0x10},
> >>>>>>     };
> >>>>>>
> >>>>>> +/*************************************************************
> >>>>>> + * CDM sub block config
> >>>>>
> >>>>> Nit: it is not a subblock config.
> >>>>>
> >>>>
> >>>> Ack.
> >>>>
> >>>>>> + *************************************************************/
> >>>>>> +static const struct dpu_cdm_cfg sc7280_cdm = {
> >>>>>
> >>>>> I know that I have r-b'ed this patch. But then one thing occurred to
> >>>>> me. If this definition is common to all (or almost all) platforms, can
> >>>>> we just call it dpu_cdm or dpu_common_cdm?
> >>>>>
> >>>>>> +       .name = "cdm_0",
> >>>>>> +       .id = CDM_0,
> >>>>>> +       .len = 0x228,
> >>>>>> +       .base = 0x79200,
> >>>>>> +};
> >>>>
> >>>> hmmm .... almost common but not entirely ... msm8998's CDM has a shorter
> >>>> len of 0x224 :(
> >>>
> >>> Then sdm845_cdm?
> >>>
> >>
> >> That also has a shorter cdm length.
> >
> > Could you please clarify. According to the downstream DT files all CDM
> > blocks up to (but not including) sm8550 had length 0x224. SM8550 and
> > SM8650 got qcom,sde-cdm-size = <0x220>.  But I don't see any registers
> > after 0x204.
> >>
>
> We always list 0x4 more than the last offset.

Yes, so this makes it correct for several latest DT files, which have
qcom,sde-cdm-size = <0x220>.
However all the previous DT files (from msm8998 to sm8450) had
qcom,sde-cdm-size = <0x224>;

>
> In chipsets sdm845 and msm8998, I only see the last offset of CDM as
> 0x220 but in sm8250 and sc7280, the last offset is 0x224. Hence the
> total length is more in sc7280/sm8250 as compared to sdm845/msm8998.
>
> I didnt follow that you do not see any registers after 0x204.
>
> The CDM_MUX is the last offset which has an offset 0x224 from the base
> address. So thats the last offset.

Ack

>
> The newer chipsets have CDM_MUX and the older ones did not. Hence the
> difference in length.
>
> >> BTW, sdm845 is not in this series. It will be part of RFT as we discussed.
> >>
> >>>>
> >>>>>> +
> >>>>>>     /*************************************************************
> >>>>>>      * VBIF sub blocks config
> >>>>>>      *************************************************************/
> >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >>>>>> index e3c0d007481b..ba82ef4560a6 100644
> >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >>>>>> @@ -682,6 +682,17 @@ struct dpu_vbif_cfg {
> >>>>>>            u32 memtype[MAX_XIN_COUNT];
> >>>>>>     };
> >>>>>>
> >>>>>> +/**
> >>>>>> + * struct dpu_cdm_cfg - information of chroma down blocks
> >>>>>> + * @name               string name for debug purposes
> >>>>>> + * @id                 enum identifying this block
> >>>>>> + * @base               register offset of this block
> >>>>>> + * @features           bit mask identifying sub-blocks/features
> >>>>>> + */
> >>>>>> +struct dpu_cdm_cfg {
> >>>>>> +       DPU_HW_BLK_INFO;
> >>>>>> +};
> >>>>>> +
> >>>>>>     /**
> >>>>>>      * Define CDP use cases
> >>>>>>      * @DPU_PERF_CDP_UDAGE_RT: real-time use cases
> >>>>>> @@ -805,6 +816,8 @@ struct dpu_mdss_cfg {
> >>>>>>            u32 wb_count;
> >>>>>>            const struct dpu_wb_cfg *wb;
> >>>>>>
> >>>>>> +       const struct dpu_cdm_cfg *cdm;
> >>>>>> +
> >>>>>>            u32 ad_count;
> >>>>>>
> >>>>>>            u32 dspp_count;
> >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> >>>>>> index a6702b2bfc68..f319c8232ea5 100644
> >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> >>>>>> @@ -185,6 +185,11 @@ enum dpu_dsc {
> >>>>>>            DSC_MAX
> >>>>>>     };
> >>>>>>
> >>>>>> +enum dpu_cdm {
> >>>>>> +       CDM_0 = 1,
> >>>>>> +       CDM_MAX
> >>>>>> +};
> >>>>>> +
> >>>>>>     enum dpu_pingpong {
> >>>>>>            PINGPONG_NONE,
> >>>>>>            PINGPONG_0,
> >>>>>> --
> >>>>>> 2.40.1
> >>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >
Dmitry Baryshkov Dec. 12, 2023, 5:12 p.m. UTC | #9 
Hi Abhinav,

On Tue, 12 Dec 2023 at 08:49, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Mon, 11 Dec 2023 at 23:48, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >
> >
> >
> > On 12/11/2023 1:42 PM, Dmitry Baryshkov wrote:
> > > On Mon, 11 Dec 2023 at 23:32, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > >>
> > >>
> > >>
> > >> On 12/11/2023 1:31 PM, Dmitry Baryshkov wrote:
> > >>> On Mon, 11 Dec 2023 at 23:16, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > >>>>
> > >>>>
> > >>>>
> > >>>> On 12/8/2023 3:19 AM, Dmitry Baryshkov wrote:
> > >>>>> On Fri, 8 Dec 2023 at 07:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > >>>>>>
> > >>>>>> Add CDM blocks to the sc7280 dpu_hw_catalog to support
> > >>>>>> YUV format output from writeback block.
> > >>>>>>
> > >>>>>> changes in v2:
> > >>>>>>            - remove explicit zero assignment for features
> > >>>>>>            - move sc7280_cdm to dpu_hw_catalog from the sc7280
> > >>>>>>              catalog file as its definition can be re-used
> > >>>>>>
> > >>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > >>>>>> ---
> > >>>>>>     .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h  |  1 +
> > >>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c      | 10 ++++++++++
> > >>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h      | 13 +++++++++++++
> > >>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h         |  5 +++++
> > >>>>>>     4 files changed, 29 insertions(+)
> > >>>>>>
> > >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> > >>>>>> index 209675de6742..19c2b7454796 100644
> > >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> > >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> > >>>>>> @@ -248,6 +248,7 @@ const struct dpu_mdss_cfg dpu_sc7280_cfg = {
> > >>>>>>            .mdss_ver = &sc7280_mdss_ver,
> > >>>>>>            .caps = &sc7280_dpu_caps,
> > >>>>>>            .mdp = &sc7280_mdp,
> > >>>>>> +       .cdm = &sc7280_cdm,
> > >>>>>>            .ctl_count = ARRAY_SIZE(sc7280_ctl),
> > >>>>>>            .ctl = sc7280_ctl,
> > >>>>>>            .sspp_count = ARRAY_SIZE(sc7280_sspp),
> > >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > >>>>>> index d52aae54bbd5..1be3156cde05 100644
> > >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > >>>>>> @@ -426,6 +426,16 @@ static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
> > >>>>>>            .ctl = {.name = "ctl", .base = 0xF80, .len = 0x10},
> > >>>>>>     };
> > >>>>>>
> > >>>>>> +/*************************************************************
> > >>>>>> + * CDM sub block config
> > >>>>>
> > >>>>> Nit: it is not a subblock config.
> > >>>>>
> > >>>>
> > >>>> Ack.
> > >>>>
> > >>>>>> + *************************************************************/
> > >>>>>> +static const struct dpu_cdm_cfg sc7280_cdm = {
> > >>>>>
> > >>>>> I know that I have r-b'ed this patch. But then one thing occurred to
> > >>>>> me. If this definition is common to all (or almost all) platforms, can
> > >>>>> we just call it dpu_cdm or dpu_common_cdm?
> > >>>>>
> > >>>>>> +       .name = "cdm_0",
> > >>>>>> +       .id = CDM_0,
> > >>>>>> +       .len = 0x228,
> > >>>>>> +       .base = 0x79200,
> > >>>>>> +};
> > >>>>
> > >>>> hmmm .... almost common but not entirely ... msm8998's CDM has a shorter
> > >>>> len of 0x224 :(
> > >>>
> > >>> Then sdm845_cdm?
> > >>>
> > >>
> > >> That also has a shorter cdm length.
> > >
> > > Could you please clarify. According to the downstream DT files all CDM
> > > blocks up to (but not including) sm8550 had length 0x224. SM8550 and
> > > SM8650 got qcom,sde-cdm-size = <0x220>.  But I don't see any registers
> > > after 0x204.
> > >>
> >
> > We always list 0x4 more than the last offset.
>
> Yes, so this makes it correct for several latest DT files, which have
> qcom,sde-cdm-size = <0x220>.
> However all the previous DT files (from msm8998 to sm8450) had
> qcom,sde-cdm-size = <0x224>;

Ok, I think I got it, what you were writing about. And we can ignore
the sde-cdm-size from the DT files.

>
> >
> > In chipsets sdm845 and msm8998, I only see the last offset of CDM as
> > 0x220 but in sm8250 and sc7280, the last offset is 0x224. Hence the
> > total length is more in sc7280/sm8250 as compared to sdm845/msm8998.
> >
> > I didnt follow that you do not see any registers after 0x204.
> >
> > The CDM_MUX is the last offset which has an offset 0x224 from the base
> > address. So thats the last offset.
>
> Ack
>
> >
> > The newer chipsets have CDM_MUX and the older ones did not. Hence the
> > difference in length.
> >
> > >> BTW, sdm845 is not in this series. It will be part of RFT as we discussed.
> > >>
> > >>>>
> > >>>>>> +
> > >>>>>>     /*************************************************************
> > >>>>>>      * VBIF sub blocks config
> > >>>>>>      *************************************************************/
> > >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > >>>>>> index e3c0d007481b..ba82ef4560a6 100644
> > >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > >>>>>> @@ -682,6 +682,17 @@ struct dpu_vbif_cfg {
> > >>>>>>            u32 memtype[MAX_XIN_COUNT];
> > >>>>>>     };
> > >>>>>>
> > >>>>>> +/**
> > >>>>>> + * struct dpu_cdm_cfg - information of chroma down blocks
> > >>>>>> + * @name               string name for debug purposes
> > >>>>>> + * @id                 enum identifying this block
> > >>>>>> + * @base               register offset of this block
> > >>>>>> + * @features           bit mask identifying sub-blocks/features
> > >>>>>> + */
> > >>>>>> +struct dpu_cdm_cfg {
> > >>>>>> +       DPU_HW_BLK_INFO;
> > >>>>>> +};
> > >>>>>> +
> > >>>>>>     /**
> > >>>>>>      * Define CDP use cases
> > >>>>>>      * @DPU_PERF_CDP_UDAGE_RT: real-time use cases
> > >>>>>> @@ -805,6 +816,8 @@ struct dpu_mdss_cfg {
> > >>>>>>            u32 wb_count;
> > >>>>>>            const struct dpu_wb_cfg *wb;
> > >>>>>>
> > >>>>>> +       const struct dpu_cdm_cfg *cdm;
> > >>>>>> +
> > >>>>>>            u32 ad_count;
> > >>>>>>
> > >>>>>>            u32 dspp_count;
> > >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> > >>>>>> index a6702b2bfc68..f319c8232ea5 100644
> > >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> > >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> > >>>>>> @@ -185,6 +185,11 @@ enum dpu_dsc {
> > >>>>>>            DSC_MAX
> > >>>>>>     };
> > >>>>>>
> > >>>>>> +enum dpu_cdm {
> > >>>>>> +       CDM_0 = 1,
> > >>>>>> +       CDM_MAX
> > >>>>>> +};
> > >>>>>> +
> > >>>>>>     enum dpu_pingpong {
> > >>>>>>            PINGPONG_NONE,
> > >>>>>>            PINGPONG_0,
> > >>>>>> --
> > >>>>>> 2.40.1
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>
> > >>>
> > >>>
> > >
> > >
> > >
>
>
>
> --
> With best wishes
> Dmitry
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
index 209675de6742..19c2b7454796 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
@@ -248,6 +248,7 @@  const struct dpu_mdss_cfg dpu_sc7280_cfg = {
 	.mdss_ver = &sc7280_mdss_ver,
 	.caps = &sc7280_dpu_caps,
 	.mdp = &sc7280_mdp,
+	.cdm = &sc7280_cdm,
 	.ctl_count = ARRAY_SIZE(sc7280_ctl),
 	.ctl = sc7280_ctl,
 	.sspp_count = ARRAY_SIZE(sc7280_sspp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index d52aae54bbd5..1be3156cde05 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -426,6 +426,16 @@  static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
 	.ctl = {.name = "ctl", .base = 0xF80, .len = 0x10},
 };
 
+/*************************************************************
+ * CDM sub block config
+ *************************************************************/
+static const struct dpu_cdm_cfg sc7280_cdm = {
+	.name = "cdm_0",
+	.id = CDM_0,
+	.len = 0x228,
+	.base = 0x79200,
+};
+
 /*************************************************************
  * VBIF sub blocks config
  *************************************************************/
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index e3c0d007481b..ba82ef4560a6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -682,6 +682,17 @@  struct dpu_vbif_cfg {
 	u32 memtype[MAX_XIN_COUNT];
 };
 
+/**
+ * struct dpu_cdm_cfg - information of chroma down blocks
+ * @name               string name for debug purposes
+ * @id                 enum identifying this block
+ * @base               register offset of this block
+ * @features           bit mask identifying sub-blocks/features
+ */
+struct dpu_cdm_cfg {
+	DPU_HW_BLK_INFO;
+};
+
 /**
  * Define CDP use cases
  * @DPU_PERF_CDP_UDAGE_RT: real-time use cases
@@ -805,6 +816,8 @@  struct dpu_mdss_cfg {
 	u32 wb_count;
 	const struct dpu_wb_cfg *wb;
 
+	const struct dpu_cdm_cfg *cdm;
+
 	u32 ad_count;
 
 	u32 dspp_count;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
index a6702b2bfc68..f319c8232ea5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
@@ -185,6 +185,11 @@  enum dpu_dsc {
 	DSC_MAX
 };
 
+enum dpu_cdm {
+	CDM_0 = 1,
+	CDM_MAX
+};
+
 enum dpu_pingpong {
 	PINGPONG_NONE,
 	PINGPONG_0,