diff mbox series

drm/msm/dpu: re-introduce dpu core revision to the catalog

Message ID 20230531005358.18090-1-quic_abhinavk@quicinc.com
State New
Headers
Series drm/msm/dpu: re-introduce dpu core revision to the catalog |

Commit Message

Abhinav Kumar May 31, 2023, 12:53 a.m. UTC 
  With [1] dpu core revision was dropped in favor of using the
compatible string from the device tree to select the dpu catalog
being used in the device.

This approach works well however also necessitates adding catalog
entries for small register level details as dpu capabilities and/or
features bloating the catalog unnecessarily. Examples include but
are not limited to data_compress, interrupt register set, widebus etc.

Introduce the dpu core revision back as an entry to the catalog so that
we can just use dpu revision checks and enable those bits which
should be enabled unconditionally and not controlled by a catalog
and also simplify the changes to do something like:

if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
	enable the bit;

Also, add some of the useful macros back to be able to use dpu core
revision effectively.

[1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
 .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
 .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
 .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
 .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
 .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
 .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
 .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
 .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
 .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
 .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
 .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
 .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
 .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31 ++++++++++++++++++-
 14 files changed, 43 insertions(+), 1 deletion(-)

Comments

Bjorn Andersson May 31, 2023, 2:16 a.m. UTC | #1 
On Tue, May 30, 2023 at 05:53:55PM -0700, Abhinav Kumar wrote:
> With [1] dpu core revision was dropped in favor of using the
> compatible string from the device tree to select the dpu catalog
> being used in the device.
> 
> This approach works well however also necessitates adding catalog
> entries for small register level details as dpu capabilities and/or
> features bloating the catalog unnecessarily. Examples include but
> are not limited to data_compress, interrupt register set, widebus etc.
> 
> Introduce the dpu core revision back as an entry to the catalog so that
> we can just use dpu revision checks and enable those bits which
> should be enabled unconditionally and not controlled by a catalog
> and also simplify the changes to do something like:
> 
> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
> 	enable the bit;
> 
> Also, add some of the useful macros back to be able to use dpu core
> revision effectively.
> 
> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
> 

No concerns with the patch, it looks good and the argumentation sounds
reasonable. But it would be preferable to introduce it together with an
actual user (or am I just missing it?).

Regards,
Bjorn

> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>  .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>  .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>  .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>  .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>  .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31 ++++++++++++++++++-
>  14 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> index 3c732a0360c7..16c2861e0359 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> @@ -185,6 +185,7 @@ static const struct dpu_perf_cfg msm8998_perf_data = {
>  };
>  
>  const struct dpu_mdss_cfg dpu_msm8998_cfg = {
> +	.core_rev = DPU_HW_VER_300,
>  	.caps = &msm8998_dpu_caps,
>  	.ubwc = &msm8998_ubwc_cfg,
>  	.mdp_count = ARRAY_SIZE(msm8998_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
> index 36ea1af10894..1c003935c948 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
> @@ -183,6 +183,7 @@ static const struct dpu_perf_cfg sdm845_perf_data = {
>  };
>  
>  const struct dpu_mdss_cfg dpu_sdm845_cfg = {
> +	.core_rev = DPU_HW_VER_400,
>  	.caps = &sdm845_dpu_caps,
>  	.ubwc = &sdm845_ubwc_cfg,
>  	.mdp_count = ARRAY_SIZE(sdm845_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
> index b5f751354267..8c914be62a88 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
> @@ -208,6 +208,7 @@ static const struct dpu_perf_cfg sm8150_perf_data = {
>  };
>  
>  const struct dpu_mdss_cfg dpu_sm8150_cfg = {
> +	.core_rev = DPU_HW_VER_500,
>  	.caps = &sm8150_dpu_caps,
>  	.ubwc = &sm8150_ubwc_cfg,
>  	.mdp_count = ARRAY_SIZE(sm8150_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
> index 8ed2b263c5ea..9465bde128eb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
> @@ -214,6 +214,7 @@ static const struct dpu_perf_cfg sc8180x_perf_data = {
>  };
>  
>  const struct dpu_mdss_cfg dpu_sc8180x_cfg = {
> +	.core_rev = DPU_HW_VER_510,
>  	.caps = &sc8180x_dpu_caps,
>  	.ubwc = &sc8180x_ubwc_cfg,
>  	.mdp_count = ARRAY_SIZE(sc8180x_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
> index daebd2170041..1b04ecfb7cde 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
> @@ -214,6 +214,7 @@ static const struct dpu_perf_cfg sm8250_perf_data = {
>  };
>  
>  const struct dpu_mdss_cfg dpu_sm8250_cfg = {
> +	.core_rev = DPU_HW_VER_600,
>  	.caps = &sm8250_dpu_caps,
>  	.ubwc = &sm8250_ubwc_cfg,
>  	.mdp_count = ARRAY_SIZE(sm8250_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
> index 0b05da2592c0..16e905e35025 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
> @@ -128,6 +128,7 @@ static const struct dpu_perf_cfg sc7180_perf_data = {
>  };
>  
>  const struct dpu_mdss_cfg dpu_sc7180_cfg = {
> +	.core_rev = DPU_HW_VER_620,
>  	.caps = &sc7180_dpu_caps,
>  	.ubwc = &sc7180_ubwc_cfg,
>  	.mdp_count = ARRAY_SIZE(sc7180_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
> index ba9de008519b..87ad7a765e4c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
> @@ -103,6 +103,7 @@ static const struct dpu_perf_cfg sm6115_perf_data = {
>  };
>  
>  const struct dpu_mdss_cfg dpu_sm6115_cfg = {
> +	.core_rev = DPU_HW_VER_630,
>  	.caps = &sm6115_dpu_caps,
>  	.ubwc = &sm6115_ubwc_cfg,
>  	.mdp_count = ARRAY_SIZE(sm6115_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
> index 92ac348eea6b..a61140ab96ed 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
> @@ -93,6 +93,7 @@ static const struct dpu_perf_cfg qcm2290_perf_data = {
>  };
>  
>  const struct dpu_mdss_cfg dpu_qcm2290_cfg = {
> +	.core_rev = DPU_HW_VER_650,
>  	.caps = &qcm2290_dpu_caps,
>  	.ubwc = &qcm2290_ubwc_cfg,
>  	.mdp_count = ARRAY_SIZE(qcm2290_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> index 3c1b2c13398d..01abce7a311c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> @@ -201,6 +201,7 @@ static const struct dpu_perf_cfg sm8350_perf_data = {
>  };
>  
>  const struct dpu_mdss_cfg dpu_sm8350_cfg = {
> +	.core_rev = DPU_HW_VER_700,
>  	.caps = &sm8350_dpu_caps,
>  	.ubwc = &sm8350_ubwc_cfg,
>  	.mdp_count = ARRAY_SIZE(sm8350_mdp),
> 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 5d894cbb0a62..4294f1d35d25 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
> @@ -141,6 +141,7 @@ static const struct dpu_perf_cfg sc7280_perf_data = {
>  };
>  
>  const struct dpu_mdss_cfg dpu_sc7280_cfg = {
> +	.core_rev = DPU_HW_VER_720,
>  	.caps = &sc7280_dpu_caps,
>  	.ubwc = &sc7280_ubwc_cfg,
>  	.mdp_count = ARRAY_SIZE(sc7280_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> index c3f1ae000a21..2108e531f13b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> @@ -203,6 +203,7 @@ static const struct dpu_perf_cfg sc8280xp_perf_data = {
>  };
>  
>  const struct dpu_mdss_cfg dpu_sc8280xp_cfg = {
> +	.core_rev = DPU_HW_VER_800,
>  	.caps = &sc8280xp_dpu_caps,
>  	.ubwc = &sc8280xp_ubwc_cfg,
>  	.mdp_count = ARRAY_SIZE(sc8280xp_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> index 86c2e68ebd2c..b8d5d0ee8c82 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> @@ -209,6 +209,7 @@ static const struct dpu_perf_cfg sm8450_perf_data = {
>  };
>  
>  const struct dpu_mdss_cfg dpu_sm8450_cfg = {
> +	.core_rev = DPU_HW_VER_810,
>  	.caps = &sm8450_dpu_caps,
>  	.ubwc = &sm8450_ubwc_cfg,
>  	.mdp_count = ARRAY_SIZE(sm8450_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> index 85dc34458b88..87a7c06e3024 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> @@ -213,6 +213,7 @@ static const struct dpu_perf_cfg sm8550_perf_data = {
>  };
>  
>  const struct dpu_mdss_cfg dpu_sm8550_cfg = {
> +	.core_rev = DPU_HW_VER_900,
>  	.caps = &sm8550_dpu_caps,
>  	.ubwc = &sm8550_ubwc_cfg,
>  	.mdp_count = ARRAY_SIZE(sm8550_mdp),
> 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 677048cc3b7d..cc4aa75a1219 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -19,6 +19,33 @@
>   */
>  #define MAX_BLOCKS    12
>  
> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
> +		  ((((unsigned int)MAJOR & 0xF) << 28) |\
> +		  ((MINOR & 0xFFF) << 16) |\
> +		  (STEP & 0xFFFF))
> +
> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
> +
> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
> +
> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
> +
>  #define DPU_HW_BLK_NAME_LEN	16
>  
>  #define MAX_IMG_WIDTH 0x3fff
> @@ -769,7 +796,7 @@ struct dpu_perf_cfg {
>  /**
>   * struct dpu_mdss_cfg - information of MDSS HW
>   * This is the main catalog data structure representing
> - * this HW version. Contains number of instances,
> + * this HW version. Contains dpu core revision, number of instances,
>   * register offsets, capabilities of the all MDSS HW sub-blocks.
>   *
>   * @dma_formats        Supported formats for dma pipe
> @@ -778,6 +805,8 @@ struct dpu_perf_cfg {
>   * @mdss_irqs:         Bitmap with the irqs supported by the target
>   */
>  struct dpu_mdss_cfg {
> +	u32 core_rev;
> +
>  	const struct dpu_caps *caps;
>  
>  	const struct dpu_ubwc_cfg *ubwc;
> -- 
> 2.40.1
>
Abhinav Kumar May 31, 2023, 2:32 a.m. UTC | #2 
On 5/30/2023 7:16 PM, Bjorn Andersson wrote:
> On Tue, May 30, 2023 at 05:53:55PM -0700, Abhinav Kumar wrote:
>> With [1] dpu core revision was dropped in favor of using the
>> compatible string from the device tree to select the dpu catalog
>> being used in the device.
>>
>> This approach works well however also necessitates adding catalog
>> entries for small register level details as dpu capabilities and/or
>> features bloating the catalog unnecessarily. Examples include but
>> are not limited to data_compress, interrupt register set, widebus etc.
>>
>> Introduce the dpu core revision back as an entry to the catalog so that
>> we can just use dpu revision checks and enable those bits which
>> should be enabled unconditionally and not controlled by a catalog
>> and also simplify the changes to do something like:
>>
>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>> 	enable the bit;
>>
>> Also, add some of the useful macros back to be able to use dpu core
>> revision effectively.
>>
>> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>
> 
> No concerns with the patch, it looks good and the argumentation sounds
> reasonable. But it would be preferable to introduce it together with an
> actual user (or am I just missing it?).
> 
> Regards,
> Bjorn
> 

The plan is that the DSC over DP (to be posted in a week or week.5), DSC 
over DSI (already on the list) , widebus (already on the list) and 
perhaps even the interrupt cleanup (already on the list) will be rebased 
on top of this.

Its just that the authors for those are different.

If we agree on this change, the rebased versions for those can be posted 
to the list. That was the intention of posting this first as I didnt 
want others to rebase if this wasnt going to be accepted.

>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>>   .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31 ++++++++++++++++++-
>>   14 files changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>> index 3c732a0360c7..16c2861e0359 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>> @@ -185,6 +185,7 @@ static const struct dpu_perf_cfg msm8998_perf_data = {
>>   };
>>   
>>   const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>> +	.core_rev = DPU_HW_VER_300,
>>   	.caps = &msm8998_dpu_caps,
>>   	.ubwc = &msm8998_ubwc_cfg,
>>   	.mdp_count = ARRAY_SIZE(msm8998_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>> index 36ea1af10894..1c003935c948 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>> @@ -183,6 +183,7 @@ static const struct dpu_perf_cfg sdm845_perf_data = {
>>   };
>>   
>>   const struct dpu_mdss_cfg dpu_sdm845_cfg = {
>> +	.core_rev = DPU_HW_VER_400,
>>   	.caps = &sdm845_dpu_caps,
>>   	.ubwc = &sdm845_ubwc_cfg,
>>   	.mdp_count = ARRAY_SIZE(sdm845_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
>> index b5f751354267..8c914be62a88 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
>> @@ -208,6 +208,7 @@ static const struct dpu_perf_cfg sm8150_perf_data = {
>>   };
>>   
>>   const struct dpu_mdss_cfg dpu_sm8150_cfg = {
>> +	.core_rev = DPU_HW_VER_500,
>>   	.caps = &sm8150_dpu_caps,
>>   	.ubwc = &sm8150_ubwc_cfg,
>>   	.mdp_count = ARRAY_SIZE(sm8150_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
>> index 8ed2b263c5ea..9465bde128eb 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
>> @@ -214,6 +214,7 @@ static const struct dpu_perf_cfg sc8180x_perf_data = {
>>   };
>>   
>>   const struct dpu_mdss_cfg dpu_sc8180x_cfg = {
>> +	.core_rev = DPU_HW_VER_510,
>>   	.caps = &sc8180x_dpu_caps,
>>   	.ubwc = &sc8180x_ubwc_cfg,
>>   	.mdp_count = ARRAY_SIZE(sc8180x_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>> index daebd2170041..1b04ecfb7cde 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>> @@ -214,6 +214,7 @@ static const struct dpu_perf_cfg sm8250_perf_data = {
>>   };
>>   
>>   const struct dpu_mdss_cfg dpu_sm8250_cfg = {
>> +	.core_rev = DPU_HW_VER_600,
>>   	.caps = &sm8250_dpu_caps,
>>   	.ubwc = &sm8250_ubwc_cfg,
>>   	.mdp_count = ARRAY_SIZE(sm8250_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>> index 0b05da2592c0..16e905e35025 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>> @@ -128,6 +128,7 @@ static const struct dpu_perf_cfg sc7180_perf_data = {
>>   };
>>   
>>   const struct dpu_mdss_cfg dpu_sc7180_cfg = {
>> +	.core_rev = DPU_HW_VER_620,
>>   	.caps = &sc7180_dpu_caps,
>>   	.ubwc = &sc7180_ubwc_cfg,
>>   	.mdp_count = ARRAY_SIZE(sc7180_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
>> index ba9de008519b..87ad7a765e4c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
>> @@ -103,6 +103,7 @@ static const struct dpu_perf_cfg sm6115_perf_data = {
>>   };
>>   
>>   const struct dpu_mdss_cfg dpu_sm6115_cfg = {
>> +	.core_rev = DPU_HW_VER_630,
>>   	.caps = &sm6115_dpu_caps,
>>   	.ubwc = &sm6115_ubwc_cfg,
>>   	.mdp_count = ARRAY_SIZE(sm6115_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
>> index 92ac348eea6b..a61140ab96ed 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
>> @@ -93,6 +93,7 @@ static const struct dpu_perf_cfg qcm2290_perf_data = {
>>   };
>>   
>>   const struct dpu_mdss_cfg dpu_qcm2290_cfg = {
>> +	.core_rev = DPU_HW_VER_650,
>>   	.caps = &qcm2290_dpu_caps,
>>   	.ubwc = &qcm2290_ubwc_cfg,
>>   	.mdp_count = ARRAY_SIZE(qcm2290_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> index 3c1b2c13398d..01abce7a311c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> @@ -201,6 +201,7 @@ static const struct dpu_perf_cfg sm8350_perf_data = {
>>   };
>>   
>>   const struct dpu_mdss_cfg dpu_sm8350_cfg = {
>> +	.core_rev = DPU_HW_VER_700,
>>   	.caps = &sm8350_dpu_caps,
>>   	.ubwc = &sm8350_ubwc_cfg,
>>   	.mdp_count = ARRAY_SIZE(sm8350_mdp),
>> 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 5d894cbb0a62..4294f1d35d25 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
>> @@ -141,6 +141,7 @@ static const struct dpu_perf_cfg sc7280_perf_data = {
>>   };
>>   
>>   const struct dpu_mdss_cfg dpu_sc7280_cfg = {
>> +	.core_rev = DPU_HW_VER_720,
>>   	.caps = &sc7280_dpu_caps,
>>   	.ubwc = &sc7280_ubwc_cfg,
>>   	.mdp_count = ARRAY_SIZE(sc7280_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> index c3f1ae000a21..2108e531f13b 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> @@ -203,6 +203,7 @@ static const struct dpu_perf_cfg sc8280xp_perf_data = {
>>   };
>>   
>>   const struct dpu_mdss_cfg dpu_sc8280xp_cfg = {
>> +	.core_rev = DPU_HW_VER_800,
>>   	.caps = &sc8280xp_dpu_caps,
>>   	.ubwc = &sc8280xp_ubwc_cfg,
>>   	.mdp_count = ARRAY_SIZE(sc8280xp_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> index 86c2e68ebd2c..b8d5d0ee8c82 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> @@ -209,6 +209,7 @@ static const struct dpu_perf_cfg sm8450_perf_data = {
>>   };
>>   
>>   const struct dpu_mdss_cfg dpu_sm8450_cfg = {
>> +	.core_rev = DPU_HW_VER_810,
>>   	.caps = &sm8450_dpu_caps,
>>   	.ubwc = &sm8450_ubwc_cfg,
>>   	.mdp_count = ARRAY_SIZE(sm8450_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> index 85dc34458b88..87a7c06e3024 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> @@ -213,6 +213,7 @@ static const struct dpu_perf_cfg sm8550_perf_data = {
>>   };
>>   
>>   const struct dpu_mdss_cfg dpu_sm8550_cfg = {
>> +	.core_rev = DPU_HW_VER_900,
>>   	.caps = &sm8550_dpu_caps,
>>   	.ubwc = &sm8550_ubwc_cfg,
>>   	.mdp_count = ARRAY_SIZE(sm8550_mdp),
>> 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 677048cc3b7d..cc4aa75a1219 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -19,6 +19,33 @@
>>    */
>>   #define MAX_BLOCKS    12
>>   
>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>> +		  ((((unsigned int)MAJOR & 0xF) << 28) |\
>> +		  ((MINOR & 0xFFF) << 16) |\
>> +		  (STEP & 0xFFFF))
>> +
>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>> +
>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>> +
>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>> +
>>   #define DPU_HW_BLK_NAME_LEN	16
>>   
>>   #define MAX_IMG_WIDTH 0x3fff
>> @@ -769,7 +796,7 @@ struct dpu_perf_cfg {
>>   /**
>>    * struct dpu_mdss_cfg - information of MDSS HW
>>    * This is the main catalog data structure representing
>> - * this HW version. Contains number of instances,
>> + * this HW version. Contains dpu core revision, number of instances,
>>    * register offsets, capabilities of the all MDSS HW sub-blocks.
>>    *
>>    * @dma_formats        Supported formats for dma pipe
>> @@ -778,6 +805,8 @@ struct dpu_perf_cfg {
>>    * @mdss_irqs:         Bitmap with the irqs supported by the target
>>    */
>>   struct dpu_mdss_cfg {
>> +	u32 core_rev;
>> +
>>   	const struct dpu_caps *caps;
>>   
>>   	const struct dpu_ubwc_cfg *ubwc;
>> -- 
>> 2.40.1
>>
Dmitry Baryshkov May 31, 2023, 2:53 a.m. UTC | #3 
On Wed, 31 May 2023 at 03:54, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> With [1] dpu core revision was dropped in favor of using the
> compatible string from the device tree to select the dpu catalog
> being used in the device.
>
> This approach works well however also necessitates adding catalog
> entries for small register level details as dpu capabilities and/or
> features bloating the catalog unnecessarily. Examples include but
> are not limited to data_compress, interrupt register set, widebus etc.
>
> Introduce the dpu core revision back as an entry to the catalog so that
> we can just use dpu revision checks and enable those bits which
> should be enabled unconditionally and not controlled by a catalog
> and also simplify the changes to do something like:
>
> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>         enable the bit;
>
> Also, add some of the useful macros back to be able to use dpu core
> revision effectively.
>
> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>  .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>  .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>  .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>  .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>  .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>  .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31 ++++++++++++++++++-
>  14 files changed, 43 insertions(+), 1 deletion(-)
>

[skipped catalog changes]

> 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 677048cc3b7d..cc4aa75a1219 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -19,6 +19,33 @@
>   */
>  #define MAX_BLOCKS    12
>
> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
> +                 ((MINOR & 0xFFF) << 16) |\
> +                 (STEP & 0xFFFF))
> +
> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
> +
> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
> +
> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */

Instead of having defines for all SoCs (which can quickly become
unmanageable) and can cause merge conflicts, I'd suggest inlining all
the defines into respective catalog files.

Also, I'm not sure that the "step" should be a part of the catalog. I
know that this follows the hardware revision. However, please correct
me if I'm wrong, different step levels are used for revisions of the
same SoC. The original code that was reading the hw revision from the
hardware register, listed both 5.0.0 and 5.0.1 for sm8150.

> +
>  #define DPU_HW_BLK_NAME_LEN    16
>
>  #define MAX_IMG_WIDTH 0x3fff
> @@ -769,7 +796,7 @@ struct dpu_perf_cfg {
>  /**
>   * struct dpu_mdss_cfg - information of MDSS HW
>   * This is the main catalog data structure representing
> - * this HW version. Contains number of instances,
> + * this HW version. Contains dpu core revision, number of instances,
>   * register offsets, capabilities of the all MDSS HW sub-blocks.
>   *
>   * @dma_formats        Supported formats for dma pipe
> @@ -778,6 +805,8 @@ struct dpu_perf_cfg {
>   * @mdss_irqs:         Bitmap with the irqs supported by the target
>   */
>  struct dpu_mdss_cfg {
> +       u32 core_rev;
> +
>         const struct dpu_caps *caps;
>
>         const struct dpu_ubwc_cfg *ubwc;
> --
> 2.40.1
>
Abhinav Kumar May 31, 2023, 3:05 a.m. UTC | #4 
On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
> On Wed, 31 May 2023 at 03:54, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> With [1] dpu core revision was dropped in favor of using the
>> compatible string from the device tree to select the dpu catalog
>> being used in the device.
>>
>> This approach works well however also necessitates adding catalog
>> entries for small register level details as dpu capabilities and/or
>> features bloating the catalog unnecessarily. Examples include but
>> are not limited to data_compress, interrupt register set, widebus etc.
>>
>> Introduce the dpu core revision back as an entry to the catalog so that
>> we can just use dpu revision checks and enable those bits which
>> should be enabled unconditionally and not controlled by a catalog
>> and also simplify the changes to do something like:
>>
>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>          enable the bit;
>>
>> Also, add some of the useful macros back to be able to use dpu core
>> revision effectively.
>>
>> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>>   .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>>   .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31 ++++++++++++++++++-
>>   14 files changed, 43 insertions(+), 1 deletion(-)
>>
> 
> [skipped catalog changes]
> 
>> 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 677048cc3b7d..cc4aa75a1219 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -19,6 +19,33 @@
>>    */
>>   #define MAX_BLOCKS    12
>>
>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>> +                 ((MINOR & 0xFFF) << 16) |\
>> +                 (STEP & 0xFFFF))
>> +
>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>> +
>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>> +
>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
> 
> Instead of having defines for all SoCs (which can quickly become
> unmanageable) and can cause merge conflicts, I'd suggest inlining all
> the defines into respective catalog files.
> 

Sure, that can be done.

> Also, I'm not sure that the "step" should be a part of the catalog. I
> know that this follows the hardware revision. However, please correct
> me if I'm wrong, different step levels are used for revisions of the
> same SoC. The original code that was reading the hw revision from the
> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
> 

This is one of the things i noticed while making this change.

Before the catalog rework, we used to handle even steps as we used to 
read that from the register and match it with the mdss_cfg handler. But 
after the rework, we dont handle steps anymore. Yes, you are right that 
different step levels are used for the revisions of the same SOC and so 
with that, i dont expect or atleast am not aware of DPU differences 
between steps but I am not able to rule it out.

So are you suggesting we drop step altogether and DPU_HW_VER() macro 
shall only handle major and minor versions? With the current chipsets I 
see, it should not make a difference . Its just that I am not sure if 
that will never happen.

>> +
>>   #define DPU_HW_BLK_NAME_LEN    16
>>
>>   #define MAX_IMG_WIDTH 0x3fff
>> @@ -769,7 +796,7 @@ struct dpu_perf_cfg {
>>   /**
>>    * struct dpu_mdss_cfg - information of MDSS HW
>>    * This is the main catalog data structure representing
>> - * this HW version. Contains number of instances,
>> + * this HW version. Contains dpu core revision, number of instances,
>>    * register offsets, capabilities of the all MDSS HW sub-blocks.
>>    *
>>    * @dma_formats        Supported formats for dma pipe
>> @@ -778,6 +805,8 @@ struct dpu_perf_cfg {
>>    * @mdss_irqs:         Bitmap with the irqs supported by the target
>>    */
>>   struct dpu_mdss_cfg {
>> +       u32 core_rev;
>> +
>>          const struct dpu_caps *caps;
>>
>>          const struct dpu_ubwc_cfg *ubwc;
>> --
>> 2.40.1
>>
> 
>
Dmitry Baryshkov May 31, 2023, 10:07 a.m. UTC | #5 
On 31/05/2023 06:05, Abhinav Kumar wrote:
> 
> 
> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar 
>> <quic_abhinavk@quicinc.com> wrote:
>>>
>>> With [1] dpu core revision was dropped in favor of using the
>>> compatible string from the device tree to select the dpu catalog
>>> being used in the device.
>>>
>>> This approach works well however also necessitates adding catalog
>>> entries for small register level details as dpu capabilities and/or
>>> features bloating the catalog unnecessarily. Examples include but
>>> are not limited to data_compress, interrupt register set, widebus etc.
>>>
>>> Introduce the dpu core revision back as an entry to the catalog so that
>>> we can just use dpu revision checks and enable those bits which
>>> should be enabled unconditionally and not controlled by a catalog
>>> and also simplify the changes to do something like:
>>>
>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>>          enable the bit;
>>>
>>> Also, add some of the useful macros back to be able to use dpu core
>>> revision effectively.
>>>
>>> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>>
>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> ---
>>>   .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>>>   .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>>>   .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>>>   .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>>>   .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>>>   .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>>>   .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>>>   .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>>>   .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>>>   .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>>>   .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>>>   .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>>>   .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>>>   .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31 ++++++++++++++++++-
>>>   14 files changed, 43 insertions(+), 1 deletion(-)
>>>
>>
>> [skipped catalog changes]
>>
>>> 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 677048cc3b7d..cc4aa75a1219 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>> @@ -19,6 +19,33 @@
>>>    */
>>>   #define MAX_BLOCKS    12
>>>
>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>>> +                 ((MINOR & 0xFFF) << 16) |\
>>> +                 (STEP & 0xFFFF))
>>> +
>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>>> +
>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>>> +
>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>>
>> Instead of having defines for all SoCs (which can quickly become
>> unmanageable) and can cause merge conflicts, I'd suggest inlining all
>> the defines into respective catalog files.
>>
> 
> Sure, that can be done.
> 
>> Also, I'm not sure that the "step" should be a part of the catalog. I
>> know that this follows the hardware revision. However, please correct
>> me if I'm wrong, different step levels are used for revisions of the
>> same SoC. The original code that was reading the hw revision from the
>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
>>
> 
> This is one of the things i noticed while making this change.
> 
> Before the catalog rework, we used to handle even steps as we used to 
> read that from the register and match it with the mdss_cfg handler. But 
> after the rework, we dont handle steps anymore. Yes, you are right that 
> different step levels are used for the revisions of the same SOC and so 
> with that, i dont expect or atleast am not aware of DPU differences 
> between steps but I am not able to rule it out.
> 
> So are you suggesting we drop step altogether and DPU_HW_VER() macro 
> shall only handle major and minor versions? With the current chipsets I 
> see, it should not make a difference . Its just that I am not sure if 
> that will never happen.

Yes. The goal of this rework would be to drop generic features and to 
replace those checks with DPU-revision lookups. Correct?
I think that from this perspective having to handle toe step revision is 
a sign of an overkill. Having to handle the step revision is a sign of 
paltform feature (or mis-feature) rather than a generic DPU bit.

In fact I suppose that even handling a minor revision would be an 
overkill. Why don't we start with .dpu_major instead of .core_rev? We 
can add .dpu_minor if/when required.

> 
>>> +
>>>   #define DPU_HW_BLK_NAME_LEN    16
>>>
>>>   #define MAX_IMG_WIDTH 0x3fff
>>> @@ -769,7 +796,7 @@ struct dpu_perf_cfg {
>>>   /**
>>>    * struct dpu_mdss_cfg - information of MDSS HW
>>>    * This is the main catalog data structure representing
>>> - * this HW version. Contains number of instances,
>>> + * this HW version. Contains dpu core revision, number of instances,
>>>    * register offsets, capabilities of the all MDSS HW sub-blocks.
>>>    *
>>>    * @dma_formats        Supported formats for dma pipe
>>> @@ -778,6 +805,8 @@ struct dpu_perf_cfg {
>>>    * @mdss_irqs:         Bitmap with the irqs supported by the target
>>>    */
>>>   struct dpu_mdss_cfg {
>>> +       u32 core_rev;
>>> +
>>>          const struct dpu_caps *caps;
>>>
>>>          const struct dpu_ubwc_cfg *ubwc;
>>> -- 
>>> 2.40.1
>>>
>>
>>
Abhinav Kumar May 31, 2023, 6:25 p.m. UTC | #6 
On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
> On 31/05/2023 06:05, Abhinav Kumar wrote:
>>
>>
>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar 
>>> <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>> With [1] dpu core revision was dropped in favor of using the
>>>> compatible string from the device tree to select the dpu catalog
>>>> being used in the device.
>>>>
>>>> This approach works well however also necessitates adding catalog
>>>> entries for small register level details as dpu capabilities and/or
>>>> features bloating the catalog unnecessarily. Examples include but
>>>> are not limited to data_compress, interrupt register set, widebus etc.
>>>>
>>>> Introduce the dpu core revision back as an entry to the catalog so that
>>>> we can just use dpu revision checks and enable those bits which
>>>> should be enabled unconditionally and not controlled by a catalog
>>>> and also simplify the changes to do something like:
>>>>
>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>>>          enable the bit;
>>>>
>>>> Also, add some of the useful macros back to be able to use dpu core
>>>> revision effectively.
>>>>
>>>> [1]: 
>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>>   .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>>>>   .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>>>>   .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>>>>   .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>>>>   .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>>>>   .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>>>>   .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>>>>   .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>>>>   .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>>>>   .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>>>>   .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>>>>   .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>>>>   .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>>>>   .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31 
>>>> ++++++++++++++++++-
>>>>   14 files changed, 43 insertions(+), 1 deletion(-)
>>>>
>>>
>>> [skipped catalog changes]
>>>
>>>> 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 677048cc3b7d..cc4aa75a1219 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>> @@ -19,6 +19,33 @@
>>>>    */
>>>>   #define MAX_BLOCKS    12
>>>>
>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>>>> +                 ((MINOR & 0xFFF) << 16) |\
>>>> +                 (STEP & 0xFFFF))
>>>> +
>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>>>> +
>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>>>> +
>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>>>
>>> Instead of having defines for all SoCs (which can quickly become
>>> unmanageable) and can cause merge conflicts, I'd suggest inlining all
>>> the defines into respective catalog files.
>>>
>>
>> Sure, that can be done.
>>
>>> Also, I'm not sure that the "step" should be a part of the catalog. I
>>> know that this follows the hardware revision. However, please correct
>>> me if I'm wrong, different step levels are used for revisions of the
>>> same SoC. The original code that was reading the hw revision from the
>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
>>>
>>
>> This is one of the things i noticed while making this change.
>>
>> Before the catalog rework, we used to handle even steps as we used to 
>> read that from the register and match it with the mdss_cfg handler. 
>> But after the rework, we dont handle steps anymore. Yes, you are right 
>> that different step levels are used for the revisions of the same SOC 
>> and so with that, i dont expect or atleast am not aware of DPU 
>> differences between steps but I am not able to rule it out.
>>
>> So are you suggesting we drop step altogether and DPU_HW_VER() macro 
>> shall only handle major and minor versions? With the current chipsets 
>> I see, it should not make a difference . Its just that I am not sure 
>> if that will never happen.
> 
> Yes. The goal of this rework would be to drop generic features and to 
> replace those checks with DPU-revision lookups. Correct?

Yes thats right.

> I think that from this perspective having to handle toe step revision is 
> a sign of an overkill. Having to handle the step revision is a sign of 
> paltform feature (or mis-feature) rather than a generic DPU bit.
> 

Not entirely. Lets not forget that at the moment even dpu_perf_cfg is 
part of the catalog. Even if in terms of major HW blocks steps shouldnt 
change, there is absolutely no guarantee that perf data cannot.

This is what is the sticking point for me which is holding me back 
against dropping step. Thoughts?

> In fact I suppose that even handling a minor revision would be an 
> overkill. Why don't we start with .dpu_major instead of .core_rev? We 
> can add .dpu_minor if/when required.
> 

No, unfortunately we cannot drop minor version for sure. I am seeing 
examples in downstream code where some of the features are available 
after a minor verion as well.

>>
>>>> +
>>>>   #define DPU_HW_BLK_NAME_LEN    16
>>>>
>>>>   #define MAX_IMG_WIDTH 0x3fff
>>>> @@ -769,7 +796,7 @@ struct dpu_perf_cfg {
>>>>   /**
>>>>    * struct dpu_mdss_cfg - information of MDSS HW
>>>>    * This is the main catalog data structure representing
>>>> - * this HW version. Contains number of instances,
>>>> + * this HW version. Contains dpu core revision, number of instances,
>>>>    * register offsets, capabilities of the all MDSS HW sub-blocks.
>>>>    *
>>>>    * @dma_formats        Supported formats for dma pipe
>>>> @@ -778,6 +805,8 @@ struct dpu_perf_cfg {
>>>>    * @mdss_irqs:         Bitmap with the irqs supported by the target
>>>>    */
>>>>   struct dpu_mdss_cfg {
>>>> +       u32 core_rev;
>>>> +
>>>>          const struct dpu_caps *caps;
>>>>
>>>>          const struct dpu_ubwc_cfg *ubwc;
>>>> -- 
>>>> 2.40.1
>>>>
>>>
>>>
>
Dmitry Baryshkov June 4, 2023, 2:21 a.m. UTC | #7 
On 31/05/2023 21:25, Abhinav Kumar wrote:
> 
> 
> On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
>> On 31/05/2023 06:05, Abhinav Kumar wrote:
>>>
>>>
>>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
>>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar 
>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>
>>>>> With [1] dpu core revision was dropped in favor of using the
>>>>> compatible string from the device tree to select the dpu catalog
>>>>> being used in the device.
>>>>>
>>>>> This approach works well however also necessitates adding catalog
>>>>> entries for small register level details as dpu capabilities and/or
>>>>> features bloating the catalog unnecessarily. Examples include but
>>>>> are not limited to data_compress, interrupt register set, widebus etc.
>>>>>
>>>>> Introduce the dpu core revision back as an entry to the catalog so 
>>>>> that
>>>>> we can just use dpu revision checks and enable those bits which
>>>>> should be enabled unconditionally and not controlled by a catalog
>>>>> and also simplify the changes to do something like:
>>>>>
>>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>>>>          enable the bit;
>>>>>
>>>>> Also, add some of the useful macros back to be able to use dpu core
>>>>> revision effectively.
>>>>>
>>>>> [1]: 
>>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>>>>
>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>> ---
>>>>>   .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>>>>>   .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>>>>>   .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>>>>>   .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>>>>>   .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>>>>>   .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>>>>>   .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>>>>>   .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>>>>>   .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>>>>>   .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>>>>>   .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>>>>>   .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>>>>>   .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>>>>>   .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31 
>>>>> ++++++++++++++++++-
>>>>>   14 files changed, 43 insertions(+), 1 deletion(-)
>>>>>
>>>>
>>>> [skipped catalog changes]
>>>>
>>>>> 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 677048cc3b7d..cc4aa75a1219 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>> @@ -19,6 +19,33 @@
>>>>>    */
>>>>>   #define MAX_BLOCKS    12
>>>>>
>>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>>>>> +                 ((MINOR & 0xFFF) << 16) |\
>>>>> +                 (STEP & 0xFFFF))
>>>>> +
>>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>>>>> +
>>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>>>>> +
>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
>>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
>>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>>>>
>>>> Instead of having defines for all SoCs (which can quickly become
>>>> unmanageable) and can cause merge conflicts, I'd suggest inlining all
>>>> the defines into respective catalog files.
>>>>
>>>
>>> Sure, that can be done.
>>>
>>>> Also, I'm not sure that the "step" should be a part of the catalog. I
>>>> know that this follows the hardware revision. However, please correct
>>>> me if I'm wrong, different step levels are used for revisions of the
>>>> same SoC. The original code that was reading the hw revision from the
>>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
>>>>
>>>
>>> This is one of the things i noticed while making this change.
>>>
>>> Before the catalog rework, we used to handle even steps as we used to 
>>> read that from the register and match it with the mdss_cfg handler. 
>>> But after the rework, we dont handle steps anymore. Yes, you are 
>>> right that different step levels are used for the revisions of the 
>>> same SOC and so with that, i dont expect or atleast am not aware of 
>>> DPU differences between steps but I am not able to rule it out.
>>>
>>> So are you suggesting we drop step altogether and DPU_HW_VER() macro 
>>> shall only handle major and minor versions? With the current chipsets 
>>> I see, it should not make a difference . Its just that I am not sure 
>>> if that will never happen.
>>
>> Yes. The goal of this rework would be to drop generic features and to 
>> replace those checks with DPU-revision lookups. Correct?
> 
> Yes thats right.
> 
>> I think that from this perspective having to handle toe step revision 
>> is a sign of an overkill. Having to handle the step revision is a sign 
>> of paltform feature (or mis-feature) rather than a generic DPU bit.
>>
> 
> Not entirely. Lets not forget that at the moment even dpu_perf_cfg is 
> part of the catalog. Even if in terms of major HW blocks steps shouldnt 
> change, there is absolutely no guarantee that perf data cannot.
> 
> This is what is the sticking point for me which is holding me back 
> against dropping step. Thoughts?

We usually do not support ES versions of the chips, only the final 
version. So supporting the perf data for earlier revisions is also not 
required.

> 
>> In fact I suppose that even handling a minor revision would be an 
>> overkill. Why don't we start with .dpu_major instead of .core_rev? We 
>> can add .dpu_minor if/when required.
>>
> 
> No, unfortunately we cannot drop minor version for sure. I am seeing 
> examples in downstream code where some of the features are available 
> after a minor verion as well.

Can you please give an example?

> 
>>>
>>>>> +
>>>>>   #define DPU_HW_BLK_NAME_LEN    16
>>>>>
>>>>>   #define MAX_IMG_WIDTH 0x3fff
>>>>> @@ -769,7 +796,7 @@ struct dpu_perf_cfg {
>>>>>   /**
>>>>>    * struct dpu_mdss_cfg - information of MDSS HW
>>>>>    * This is the main catalog data structure representing
>>>>> - * this HW version. Contains number of instances,
>>>>> + * this HW version. Contains dpu core revision, number of instances,
>>>>>    * register offsets, capabilities of the all MDSS HW sub-blocks.
>>>>>    *
>>>>>    * @dma_formats        Supported formats for dma pipe
>>>>> @@ -778,6 +805,8 @@ struct dpu_perf_cfg {
>>>>>    * @mdss_irqs:         Bitmap with the irqs supported by the target
>>>>>    */
>>>>>   struct dpu_mdss_cfg {
>>>>> +       u32 core_rev;
>>>>> +
>>>>>          const struct dpu_caps *caps;
>>>>>
>>>>>          const struct dpu_ubwc_cfg *ubwc;
>>>>> -- 
>>>>> 2.40.1
>>>>>
>>>>
>>>>
>>
Abhinav Kumar June 6, 2023, 12:55 a.m. UTC | #8 
On 6/3/2023 7:21 PM, Dmitry Baryshkov wrote:
> On 31/05/2023 21:25, Abhinav Kumar wrote:
>>
>>
>> On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
>>> On 31/05/2023 06:05, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
>>>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar 
>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>
>>>>>> With [1] dpu core revision was dropped in favor of using the
>>>>>> compatible string from the device tree to select the dpu catalog
>>>>>> being used in the device.
>>>>>>
>>>>>> This approach works well however also necessitates adding catalog
>>>>>> entries for small register level details as dpu capabilities and/or
>>>>>> features bloating the catalog unnecessarily. Examples include but
>>>>>> are not limited to data_compress, interrupt register set, widebus 
>>>>>> etc.
>>>>>>
>>>>>> Introduce the dpu core revision back as an entry to the catalog so 
>>>>>> that
>>>>>> we can just use dpu revision checks and enable those bits which
>>>>>> should be enabled unconditionally and not controlled by a catalog
>>>>>> and also simplify the changes to do something like:
>>>>>>
>>>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>>>>>          enable the bit;
>>>>>>
>>>>>> Also, add some of the useful macros back to be able to use dpu core
>>>>>> revision effectively.
>>>>>>
>>>>>> [1]: 
>>>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>>>>>
>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>> ---
>>>>>>   .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>>>>>>   .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>>>>>>   .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>>>>>>   .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>>>>>>   .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>>>>>>   .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>>>>>>   .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>>>>>>   .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>>>>>>   .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>>>>>>   .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31 
>>>>>> ++++++++++++++++++-
>>>>>>   14 files changed, 43 insertions(+), 1 deletion(-)
>>>>>>
>>>>>
>>>>> [skipped catalog changes]
>>>>>
>>>>>> 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 677048cc3b7d..cc4aa75a1219 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>> @@ -19,6 +19,33 @@
>>>>>>    */
>>>>>>   #define MAX_BLOCKS    12
>>>>>>
>>>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>>>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>>>>>> +                 ((MINOR & 0xFFF) << 16) |\
>>>>>> +                 (STEP & 0xFFFF))
>>>>>> +
>>>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>>>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>>>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>>>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>>>>>> +
>>>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>>>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>>>>>> +
>>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>>>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>>>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>>>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>>>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>>>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>>>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
>>>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
>>>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>>>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>>>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>>>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>>>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>>>>>
>>>>> Instead of having defines for all SoCs (which can quickly become
>>>>> unmanageable) and can cause merge conflicts, I'd suggest inlining all
>>>>> the defines into respective catalog files.
>>>>>
>>>>
>>>> Sure, that can be done.
>>>>
>>>>> Also, I'm not sure that the "step" should be a part of the catalog. I
>>>>> know that this follows the hardware revision. However, please correct
>>>>> me if I'm wrong, different step levels are used for revisions of the
>>>>> same SoC. The original code that was reading the hw revision from the
>>>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
>>>>>
>>>>
>>>> This is one of the things i noticed while making this change.
>>>>
>>>> Before the catalog rework, we used to handle even steps as we used 
>>>> to read that from the register and match it with the mdss_cfg 
>>>> handler. But after the rework, we dont handle steps anymore. Yes, 
>>>> you are right that different step levels are used for the revisions 
>>>> of the same SOC and so with that, i dont expect or atleast am not 
>>>> aware of DPU differences between steps but I am not able to rule it 
>>>> out.
>>>>
>>>> So are you suggesting we drop step altogether and DPU_HW_VER() macro 
>>>> shall only handle major and minor versions? With the current 
>>>> chipsets I see, it should not make a difference . Its just that I am 
>>>> not sure if that will never happen.
>>>
>>> Yes. The goal of this rework would be to drop generic features and to 
>>> replace those checks with DPU-revision lookups. Correct?
>>
>> Yes thats right.
>>
>>> I think that from this perspective having to handle toe step revision 
>>> is a sign of an overkill. Having to handle the step revision is a 
>>> sign of paltform feature (or mis-feature) rather than a generic DPU bit.
>>>
>>
>> Not entirely. Lets not forget that at the moment even dpu_perf_cfg is 
>> part of the catalog. Even if in terms of major HW blocks steps 
>> shouldnt change, there is absolutely no guarantee that perf data cannot.
>>
>> This is what is the sticking point for me which is holding me back 
>> against dropping step. Thoughts?
> 
> We usually do not support ES versions of the chips, only the final 
> version. So supporting the perf data for earlier revisions is also not 
> required.
> 

ack, we will drop step in that case. and good to know about the ES versions.

>>
>>> In fact I suppose that even handling a minor revision would be an 
>>> overkill. Why don't we start with .dpu_major instead of .core_rev? We 
>>> can add .dpu_minor if/when required.
>>>
>>
>> No, unfortunately we cannot drop minor version for sure. I am seeing 
>> examples in downstream code where some of the features are available 
>> after a minor verion as well.
> 
> Can you please give an example?
> 

Yes, watchdog timer, intf reset counter are available only after DPU HW 
version 8.1 (not major version 8).
Dmitry Baryshkov June 6, 2023, 1:03 a.m. UTC | #9 
On 06/06/2023 03:55, Abhinav Kumar wrote:
> 
> 
> On 6/3/2023 7:21 PM, Dmitry Baryshkov wrote:
>> On 31/05/2023 21:25, Abhinav Kumar wrote:
>>>
>>>
>>> On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>> On 31/05/2023 06:05, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
>>>>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar 
>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>
>>>>>>> With [1] dpu core revision was dropped in favor of using the
>>>>>>> compatible string from the device tree to select the dpu catalog
>>>>>>> being used in the device.
>>>>>>>
>>>>>>> This approach works well however also necessitates adding catalog
>>>>>>> entries for small register level details as dpu capabilities and/or
>>>>>>> features bloating the catalog unnecessarily. Examples include but
>>>>>>> are not limited to data_compress, interrupt register set, widebus 
>>>>>>> etc.
>>>>>>>
>>>>>>> Introduce the dpu core revision back as an entry to the catalog 
>>>>>>> so that
>>>>>>> we can just use dpu revision checks and enable those bits which
>>>>>>> should be enabled unconditionally and not controlled by a catalog
>>>>>>> and also simplify the changes to do something like:
>>>>>>>
>>>>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>>>>>>          enable the bit;
>>>>>>>
>>>>>>> Also, add some of the useful macros back to be able to use dpu core
>>>>>>> revision effectively.
>>>>>>>
>>>>>>> [1]: 
>>>>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>>>>>>
>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>>> ---
>>>>>>>   .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>>>>>>>   .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>>>>>>>   .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>>>>>>>   .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>>>>>>>   .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>>>>>>>   .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>>>>>>>   .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>>>>>>>   .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>>>>>>>   .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>>>>>>>   .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31 
>>>>>>> ++++++++++++++++++-
>>>>>>>   14 files changed, 43 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>
>>>>>> [skipped catalog changes]
>>>>>>
>>>>>>> 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 677048cc3b7d..cc4aa75a1219 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>> @@ -19,6 +19,33 @@
>>>>>>>    */
>>>>>>>   #define MAX_BLOCKS    12
>>>>>>>
>>>>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>>>>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>>>>>>> +                 ((MINOR & 0xFFF) << 16) |\
>>>>>>> +                 (STEP & 0xFFFF))
>>>>>>> +
>>>>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>>>>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>>>>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>>>>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>>>>>>> +
>>>>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>>>>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>>>>>>> +
>>>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>>>>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>>>>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>>>>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>>>>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>>>>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>>>>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
>>>>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
>>>>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>>>>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>>>>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>>>>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>>>>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>>>>>>
>>>>>> Instead of having defines for all SoCs (which can quickly become
>>>>>> unmanageable) and can cause merge conflicts, I'd suggest inlining all
>>>>>> the defines into respective catalog files.
>>>>>>
>>>>>
>>>>> Sure, that can be done.
>>>>>
>>>>>> Also, I'm not sure that the "step" should be a part of the catalog. I
>>>>>> know that this follows the hardware revision. However, please correct
>>>>>> me if I'm wrong, different step levels are used for revisions of the
>>>>>> same SoC. The original code that was reading the hw revision from the
>>>>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
>>>>>>
>>>>>
>>>>> This is one of the things i noticed while making this change.
>>>>>
>>>>> Before the catalog rework, we used to handle even steps as we used 
>>>>> to read that from the register and match it with the mdss_cfg 
>>>>> handler. But after the rework, we dont handle steps anymore. Yes, 
>>>>> you are right that different step levels are used for the revisions 
>>>>> of the same SOC and so with that, i dont expect or atleast am not 
>>>>> aware of DPU differences between steps but I am not able to rule it 
>>>>> out.
>>>>>
>>>>> So are you suggesting we drop step altogether and DPU_HW_VER() 
>>>>> macro shall only handle major and minor versions? With the current 
>>>>> chipsets I see, it should not make a difference . Its just that I 
>>>>> am not sure if that will never happen.
>>>>
>>>> Yes. The goal of this rework would be to drop generic features and 
>>>> to replace those checks with DPU-revision lookups. Correct?
>>>
>>> Yes thats right.
>>>
>>>> I think that from this perspective having to handle toe step 
>>>> revision is a sign of an overkill. Having to handle the step 
>>>> revision is a sign of paltform feature (or mis-feature) rather than 
>>>> a generic DPU bit.
>>>>
>>>
>>> Not entirely. Lets not forget that at the moment even dpu_perf_cfg is 
>>> part of the catalog. Even if in terms of major HW blocks steps 
>>> shouldnt change, there is absolutely no guarantee that perf data cannot.
>>>
>>> This is what is the sticking point for me which is holding me back 
>>> against dropping step. Thoughts?
>>
>> We usually do not support ES versions of the chips, only the final 
>> version. So supporting the perf data for earlier revisions is also not 
>> required.
>>
> 
> ack, we will drop step in that case. and good to know about the ES 
> versions.
> 
>>>
>>>> In fact I suppose that even handling a minor revision would be an 
>>>> overkill. Why don't we start with .dpu_major instead of .core_rev? 
>>>> We can add .dpu_minor if/when required.
>>>>
>>>
>>> No, unfortunately we cannot drop minor version for sure. I am seeing 
>>> examples in downstream code where some of the features are available 
>>> after a minor verion as well.
>>
>> Can you please give an example?
>>
> 
> Yes, watchdog timer, intf reset counter are available only after DPU HW 
> version 8.1 (not major version 8).

Hmm, IIRC, wd timer was available for ages. Was it moved together with 
the introduction of MDSS_PERIPH_0_REMOVED?

But anyway, I see your point. Let's have major and minor. I'd probably 
still ask for the separate major and minor fields, if you don't mind.
Abhinav Kumar June 6, 2023, 2:35 a.m. UTC | #10 
On 6/5/2023 6:03 PM, Dmitry Baryshkov wrote:
> On 06/06/2023 03:55, Abhinav Kumar wrote:
>>
>>
>> On 6/3/2023 7:21 PM, Dmitry Baryshkov wrote:
>>> On 31/05/2023 21:25, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>>> On 31/05/2023 06:05, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
>>>>>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar 
>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>
>>>>>>>> With [1] dpu core revision was dropped in favor of using the
>>>>>>>> compatible string from the device tree to select the dpu catalog
>>>>>>>> being used in the device.
>>>>>>>>
>>>>>>>> This approach works well however also necessitates adding catalog
>>>>>>>> entries for small register level details as dpu capabilities and/or
>>>>>>>> features bloating the catalog unnecessarily. Examples include but
>>>>>>>> are not limited to data_compress, interrupt register set, 
>>>>>>>> widebus etc.
>>>>>>>>
>>>>>>>> Introduce the dpu core revision back as an entry to the catalog 
>>>>>>>> so that
>>>>>>>> we can just use dpu revision checks and enable those bits which
>>>>>>>> should be enabled unconditionally and not controlled by a catalog
>>>>>>>> and also simplify the changes to do something like:
>>>>>>>>
>>>>>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>>>>>>>          enable the bit;
>>>>>>>>
>>>>>>>> Also, add some of the useful macros back to be able to use dpu core
>>>>>>>> revision effectively.
>>>>>>>>
>>>>>>>> [1]: 
>>>>>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>>>>>>>
>>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>>>> ---
>>>>>>>>   .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>>>>>>>>   .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>>>>>>>>   .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>>>>>>>>   .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>>>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>>>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>>>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>>>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>>>>>>>>   .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>>>>>>>>   .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>>>>>>>>   .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>>>>>>>>   .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>>>>>>>>   .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>>>>>>>>   .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31 
>>>>>>>> ++++++++++++++++++-
>>>>>>>>   14 files changed, 43 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>
>>>>>>> [skipped catalog changes]
>>>>>>>
>>>>>>>> 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 677048cc3b7d..cc4aa75a1219 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>> @@ -19,6 +19,33 @@
>>>>>>>>    */
>>>>>>>>   #define MAX_BLOCKS    12
>>>>>>>>
>>>>>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>>>>>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>>>>>>>> +                 ((MINOR & 0xFFF) << 16) |\
>>>>>>>> +                 (STEP & 0xFFFF))
>>>>>>>> +
>>>>>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>>>>>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>>>>>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>>>>>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>>>>>>>> +
>>>>>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>>>>>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>>>>>>>> +
>>>>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>>>>>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>>>>>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>>>>>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>>>>>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>>>>>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>>>>>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
>>>>>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
>>>>>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>>>>>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>>>>>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>>>>>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>>>>>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>>>>>>>
>>>>>>> Instead of having defines for all SoCs (which can quickly become
>>>>>>> unmanageable) and can cause merge conflicts, I'd suggest inlining 
>>>>>>> all
>>>>>>> the defines into respective catalog files.
>>>>>>>
>>>>>>
>>>>>> Sure, that can be done.
>>>>>>
>>>>>>> Also, I'm not sure that the "step" should be a part of the 
>>>>>>> catalog. I
>>>>>>> know that this follows the hardware revision. However, please 
>>>>>>> correct
>>>>>>> me if I'm wrong, different step levels are used for revisions of the
>>>>>>> same SoC. The original code that was reading the hw revision from 
>>>>>>> the
>>>>>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
>>>>>>>
>>>>>>
>>>>>> This is one of the things i noticed while making this change.
>>>>>>
>>>>>> Before the catalog rework, we used to handle even steps as we used 
>>>>>> to read that from the register and match it with the mdss_cfg 
>>>>>> handler. But after the rework, we dont handle steps anymore. Yes, 
>>>>>> you are right that different step levels are used for the 
>>>>>> revisions of the same SOC and so with that, i dont expect or 
>>>>>> atleast am not aware of DPU differences between steps but I am not 
>>>>>> able to rule it out.
>>>>>>
>>>>>> So are you suggesting we drop step altogether and DPU_HW_VER() 
>>>>>> macro shall only handle major and minor versions? With the current 
>>>>>> chipsets I see, it should not make a difference . Its just that I 
>>>>>> am not sure if that will never happen.
>>>>>
>>>>> Yes. The goal of this rework would be to drop generic features and 
>>>>> to replace those checks with DPU-revision lookups. Correct?
>>>>
>>>> Yes thats right.
>>>>
>>>>> I think that from this perspective having to handle toe step 
>>>>> revision is a sign of an overkill. Having to handle the step 
>>>>> revision is a sign of paltform feature (or mis-feature) rather than 
>>>>> a generic DPU bit.
>>>>>
>>>>
>>>> Not entirely. Lets not forget that at the moment even dpu_perf_cfg 
>>>> is part of the catalog. Even if in terms of major HW blocks steps 
>>>> shouldnt change, there is absolutely no guarantee that perf data 
>>>> cannot.
>>>>
>>>> This is what is the sticking point for me which is holding me back 
>>>> against dropping step. Thoughts?
>>>
>>> We usually do not support ES versions of the chips, only the final 
>>> version. So supporting the perf data for earlier revisions is also 
>>> not required.
>>>
>>
>> ack, we will drop step in that case. and good to know about the ES 
>> versions.
>>
>>>>
>>>>> In fact I suppose that even handling a minor revision would be an 
>>>>> overkill. Why don't we start with .dpu_major instead of .core_rev? 
>>>>> We can add .dpu_minor if/when required.
>>>>>
>>>>
>>>> No, unfortunately we cannot drop minor version for sure. I am seeing 
>>>> examples in downstream code where some of the features are available 
>>>> after a minor verion as well.
>>>
>>> Can you please give an example?
>>>
>>
>> Yes, watchdog timer, intf reset counter are available only after DPU 
>> HW version 8.1 (not major version 8).
> 
> Hmm, IIRC, wd timer was available for ages. Was it moved together with 
> the introduction of MDSS_PERIPH_0_REMOVED?
> 

I am not sure of the timeline but its certainly tied to 8.1.

> But anyway, I see your point. Let's have major and minor. I'd probably 
> still ask for the separate major and minor fields, if you don't mind.
> 

hmmm so something like this?

+#define DPU_HW_VER_300 DPU_HW_VER(3, 0) /* 8998 v1.0 */

const struct dpu_mdss_cfg dpu_msm8998_cfg = {
.......
.dpu_major_rev = DPU_HW_MAJOR(DPU_HW_VER_300),
.dpu_minor_rev = DPU_HW_MINOR(DPU_HW_VER_300)
.......
}

But may I ask why? Since the manor/minor version macros handle this 
nicely for us.
Dmitry Baryshkov June 6, 2023, 11:14 a.m. UTC | #11 
On Tue, 6 Jun 2023 at 05:35, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/5/2023 6:03 PM, Dmitry Baryshkov wrote:
> > On 06/06/2023 03:55, Abhinav Kumar wrote:
> >>
> >>
> >> On 6/3/2023 7:21 PM, Dmitry Baryshkov wrote:
> >>> On 31/05/2023 21:25, Abhinav Kumar wrote:
> >>>>
> >>>>
> >>>> On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
> >>>>> On 31/05/2023 06:05, Abhinav Kumar wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
> >>>>>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar
> >>>>>>> <quic_abhinavk@quicinc.com> wrote:
> >>>>>>>>
> >>>>>>>> With [1] dpu core revision was dropped in favor of using the
> >>>>>>>> compatible string from the device tree to select the dpu catalog
> >>>>>>>> being used in the device.
> >>>>>>>>
> >>>>>>>> This approach works well however also necessitates adding catalog
> >>>>>>>> entries for small register level details as dpu capabilities and/or
> >>>>>>>> features bloating the catalog unnecessarily. Examples include but
> >>>>>>>> are not limited to data_compress, interrupt register set,
> >>>>>>>> widebus etc.
> >>>>>>>>
> >>>>>>>> Introduce the dpu core revision back as an entry to the catalog
> >>>>>>>> so that
> >>>>>>>> we can just use dpu revision checks and enable those bits which
> >>>>>>>> should be enabled unconditionally and not controlled by a catalog
> >>>>>>>> and also simplify the changes to do something like:
> >>>>>>>>
> >>>>>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
> >>>>>>>>          enable the bit;
> >>>>>>>>
> >>>>>>>> Also, add some of the useful macros back to be able to use dpu core
> >>>>>>>> revision effectively.
> >>>>>>>>
> >>>>>>>> [1]:
> >>>>>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
> >>>>>>>>
> >>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >>>>>>>> ---
> >>>>>>>>   .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
> >>>>>>>>   .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
> >>>>>>>>   .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
> >>>>>>>>   .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
> >>>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
> >>>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
> >>>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
> >>>>>>>>   .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
> >>>>>>>>   .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
> >>>>>>>>   .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
> >>>>>>>>   .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
> >>>>>>>>   .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
> >>>>>>>>   .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
> >>>>>>>>   .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31
> >>>>>>>> ++++++++++++++++++-
> >>>>>>>>   14 files changed, 43 insertions(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>
> >>>>>>> [skipped catalog changes]
> >>>>>>>
> >>>>>>>> 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 677048cc3b7d..cc4aa75a1219 100644
> >>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >>>>>>>> @@ -19,6 +19,33 @@
> >>>>>>>>    */
> >>>>>>>>   #define MAX_BLOCKS    12
> >>>>>>>>
> >>>>>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
> >>>>>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
> >>>>>>>> +                 ((MINOR & 0xFFF) << 16) |\
> >>>>>>>> +                 (STEP & 0xFFFF))
> >>>>>>>> +
> >>>>>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
> >>>>>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
> >>>>>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
> >>>>>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
> >>>>>>>> +
> >>>>>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
> >>>>>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
> >>>>>>>> +
> >>>>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
> >>>>>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
> >>>>>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
> >>>>>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
> >>>>>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
> >>>>>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
> >>>>>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
> >>>>>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
> >>>>>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
> >>>>>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
> >>>>>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
> >>>>>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
> >>>>>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
> >>>>>>>
> >>>>>>> Instead of having defines for all SoCs (which can quickly become
> >>>>>>> unmanageable) and can cause merge conflicts, I'd suggest inlining
> >>>>>>> all
> >>>>>>> the defines into respective catalog files.
> >>>>>>>
> >>>>>>
> >>>>>> Sure, that can be done.
> >>>>>>
> >>>>>>> Also, I'm not sure that the "step" should be a part of the
> >>>>>>> catalog. I
> >>>>>>> know that this follows the hardware revision. However, please
> >>>>>>> correct
> >>>>>>> me if I'm wrong, different step levels are used for revisions of the
> >>>>>>> same SoC. The original code that was reading the hw revision from
> >>>>>>> the
> >>>>>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
> >>>>>>>
> >>>>>>
> >>>>>> This is one of the things i noticed while making this change.
> >>>>>>
> >>>>>> Before the catalog rework, we used to handle even steps as we used
> >>>>>> to read that from the register and match it with the mdss_cfg
> >>>>>> handler. But after the rework, we dont handle steps anymore. Yes,
> >>>>>> you are right that different step levels are used for the
> >>>>>> revisions of the same SOC and so with that, i dont expect or
> >>>>>> atleast am not aware of DPU differences between steps but I am not
> >>>>>> able to rule it out.
> >>>>>>
> >>>>>> So are you suggesting we drop step altogether and DPU_HW_VER()
> >>>>>> macro shall only handle major and minor versions? With the current
> >>>>>> chipsets I see, it should not make a difference . Its just that I
> >>>>>> am not sure if that will never happen.
> >>>>>
> >>>>> Yes. The goal of this rework would be to drop generic features and
> >>>>> to replace those checks with DPU-revision lookups. Correct?
> >>>>
> >>>> Yes thats right.
> >>>>
> >>>>> I think that from this perspective having to handle toe step
> >>>>> revision is a sign of an overkill. Having to handle the step
> >>>>> revision is a sign of paltform feature (or mis-feature) rather than
> >>>>> a generic DPU bit.
> >>>>>
> >>>>
> >>>> Not entirely. Lets not forget that at the moment even dpu_perf_cfg
> >>>> is part of the catalog. Even if in terms of major HW blocks steps
> >>>> shouldnt change, there is absolutely no guarantee that perf data
> >>>> cannot.
> >>>>
> >>>> This is what is the sticking point for me which is holding me back
> >>>> against dropping step. Thoughts?
> >>>
> >>> We usually do not support ES versions of the chips, only the final
> >>> version. So supporting the perf data for earlier revisions is also
> >>> not required.
> >>>
> >>
> >> ack, we will drop step in that case. and good to know about the ES
> >> versions.
> >>
> >>>>
> >>>>> In fact I suppose that even handling a minor revision would be an
> >>>>> overkill. Why don't we start with .dpu_major instead of .core_rev?
> >>>>> We can add .dpu_minor if/when required.
> >>>>>
> >>>>
> >>>> No, unfortunately we cannot drop minor version for sure. I am seeing
> >>>> examples in downstream code where some of the features are available
> >>>> after a minor verion as well.
> >>>
> >>> Can you please give an example?
> >>>
> >>
> >> Yes, watchdog timer, intf reset counter are available only after DPU
> >> HW version 8.1 (not major version 8).
> >
> > Hmm, IIRC, wd timer was available for ages. Was it moved together with
> > the introduction of MDSS_PERIPH_0_REMOVED?
> >
>
> I am not sure of the timeline but its certainly tied to 8.1.
>
> > But anyway, I see your point. Let's have major and minor. I'd probably
> > still ask for the separate major and minor fields, if you don't mind.
> >
>
> hmmm so something like this?
>
> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0) /* 8998 v1.0 */
>
> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
> .......
> .dpu_major_rev = DPU_HW_MAJOR(DPU_HW_VER_300),
> .dpu_minor_rev = DPU_HW_MINOR(DPU_HW_VER_300)

Just:

const struct dpu_mdss_cfg dpu_msm8998_cfg = {
    .dpu_major_rev = 3,
    .dpu_minor_rev = 0,
    /* .... */
};

We do not need a single enumeration of all the versions. It can easily
become a source of merge conflicts.

> .......
> }
>
> But may I ask why? Since the manor/minor version macros handle this
> nicely for us.
Abhinav Kumar June 6, 2023, 5:51 p.m. UTC | #12 
On 6/6/2023 4:14 AM, Dmitry Baryshkov wrote:
> On Tue, 6 Jun 2023 at 05:35, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/5/2023 6:03 PM, Dmitry Baryshkov wrote:
>>> On 06/06/2023 03:55, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/3/2023 7:21 PM, Dmitry Baryshkov wrote:
>>>>> On 31/05/2023 21:25, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>>>>> On 31/05/2023 06:05, Abhinav Kumar wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar
>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>
>>>>>>>>>> With [1] dpu core revision was dropped in favor of using the
>>>>>>>>>> compatible string from the device tree to select the dpu catalog
>>>>>>>>>> being used in the device.
>>>>>>>>>>
>>>>>>>>>> This approach works well however also necessitates adding catalog
>>>>>>>>>> entries for small register level details as dpu capabilities and/or
>>>>>>>>>> features bloating the catalog unnecessarily. Examples include but
>>>>>>>>>> are not limited to data_compress, interrupt register set,
>>>>>>>>>> widebus etc.
>>>>>>>>>>
>>>>>>>>>> Introduce the dpu core revision back as an entry to the catalog
>>>>>>>>>> so that
>>>>>>>>>> we can just use dpu revision checks and enable those bits which
>>>>>>>>>> should be enabled unconditionally and not controlled by a catalog
>>>>>>>>>> and also simplify the changes to do something like:
>>>>>>>>>>
>>>>>>>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>>>>>>>>>           enable the bit;
>>>>>>>>>>
>>>>>>>>>> Also, add some of the useful macros back to be able to use dpu core
>>>>>>>>>> revision effectively.
>>>>>>>>>>
>>>>>>>>>> [1]:
>>>>>>>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>>>>>> ---
>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>>>>>>>>>>    .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31
>>>>>>>>>> ++++++++++++++++++-
>>>>>>>>>>    14 files changed, 43 insertions(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> [skipped catalog changes]
>>>>>>>>>
>>>>>>>>>> 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 677048cc3b7d..cc4aa75a1219 100644
>>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>> @@ -19,6 +19,33 @@
>>>>>>>>>>     */
>>>>>>>>>>    #define MAX_BLOCKS    12
>>>>>>>>>>
>>>>>>>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>>>>>>>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>>>>>>>>>> +                 ((MINOR & 0xFFF) << 16) |\
>>>>>>>>>> +                 (STEP & 0xFFFF))
>>>>>>>>>> +
>>>>>>>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>>>>>>>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>>>>>>>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>>>>>>>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>>>>>>>>>> +
>>>>>>>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>>>>>>>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>>>>>>>>>> +
>>>>>>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>>>>>>>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>>>>>>>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>>>>>>>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>>>>>>>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>>>>>>>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>>>>>>>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
>>>>>>>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
>>>>>>>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>>>>>>>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>>>>>>>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>>>>>>>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>>>>>>>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>>>>>>>>>
>>>>>>>>> Instead of having defines for all SoCs (which can quickly become
>>>>>>>>> unmanageable) and can cause merge conflicts, I'd suggest inlining
>>>>>>>>> all
>>>>>>>>> the defines into respective catalog files.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Sure, that can be done.
>>>>>>>>
>>>>>>>>> Also, I'm not sure that the "step" should be a part of the
>>>>>>>>> catalog. I
>>>>>>>>> know that this follows the hardware revision. However, please
>>>>>>>>> correct
>>>>>>>>> me if I'm wrong, different step levels are used for revisions of the
>>>>>>>>> same SoC. The original code that was reading the hw revision from
>>>>>>>>> the
>>>>>>>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
>>>>>>>>>
>>>>>>>>
>>>>>>>> This is one of the things i noticed while making this change.
>>>>>>>>
>>>>>>>> Before the catalog rework, we used to handle even steps as we used
>>>>>>>> to read that from the register and match it with the mdss_cfg
>>>>>>>> handler. But after the rework, we dont handle steps anymore. Yes,
>>>>>>>> you are right that different step levels are used for the
>>>>>>>> revisions of the same SOC and so with that, i dont expect or
>>>>>>>> atleast am not aware of DPU differences between steps but I am not
>>>>>>>> able to rule it out.
>>>>>>>>
>>>>>>>> So are you suggesting we drop step altogether and DPU_HW_VER()
>>>>>>>> macro shall only handle major and minor versions? With the current
>>>>>>>> chipsets I see, it should not make a difference . Its just that I
>>>>>>>> am not sure if that will never happen.
>>>>>>>
>>>>>>> Yes. The goal of this rework would be to drop generic features and
>>>>>>> to replace those checks with DPU-revision lookups. Correct?
>>>>>>
>>>>>> Yes thats right.
>>>>>>
>>>>>>> I think that from this perspective having to handle toe step
>>>>>>> revision is a sign of an overkill. Having to handle the step
>>>>>>> revision is a sign of paltform feature (or mis-feature) rather than
>>>>>>> a generic DPU bit.
>>>>>>>
>>>>>>
>>>>>> Not entirely. Lets not forget that at the moment even dpu_perf_cfg
>>>>>> is part of the catalog. Even if in terms of major HW blocks steps
>>>>>> shouldnt change, there is absolutely no guarantee that perf data
>>>>>> cannot.
>>>>>>
>>>>>> This is what is the sticking point for me which is holding me back
>>>>>> against dropping step. Thoughts?
>>>>>
>>>>> We usually do not support ES versions of the chips, only the final
>>>>> version. So supporting the perf data for earlier revisions is also
>>>>> not required.
>>>>>
>>>>
>>>> ack, we will drop step in that case. and good to know about the ES
>>>> versions.
>>>>
>>>>>>
>>>>>>> In fact I suppose that even handling a minor revision would be an
>>>>>>> overkill. Why don't we start with .dpu_major instead of .core_rev?
>>>>>>> We can add .dpu_minor if/when required.
>>>>>>>
>>>>>>
>>>>>> No, unfortunately we cannot drop minor version for sure. I am seeing
>>>>>> examples in downstream code where some of the features are available
>>>>>> after a minor verion as well.
>>>>>
>>>>> Can you please give an example?
>>>>>
>>>>
>>>> Yes, watchdog timer, intf reset counter are available only after DPU
>>>> HW version 8.1 (not major version 8).
>>>
>>> Hmm, IIRC, wd timer was available for ages. Was it moved together with
>>> the introduction of MDSS_PERIPH_0_REMOVED?
>>>
>>
>> I am not sure of the timeline but its certainly tied to 8.1.
>>
>>> But anyway, I see your point. Let's have major and minor. I'd probably
>>> still ask for the separate major and minor fields, if you don't mind.
>>>
>>
>> hmmm so something like this?
>>
>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0) /* 8998 v1.0 */
>>
>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>> .......
>> .dpu_major_rev = DPU_HW_MAJOR(DPU_HW_VER_300),
>> .dpu_minor_rev = DPU_HW_MINOR(DPU_HW_VER_300)
> 
> Just:
> 
> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>      .dpu_major_rev = 3,
>      .dpu_minor_rev = 0,
>      /* .... */
> };
> 
> We do not need a single enumeration of all the versions. It can easily
> become a source of merge conflicts.
> 

The issue with this approch is then the DPU_HW_VER_xxx macros become 
redundant but we should keep them. Because in the code, its much more 
readable to have

if (core_rev > DPU_HW_MAJOR(DPU_HW_VER_xxx))
	then enable feature;

But now we will have to do

if (dpu_major_rev > xxx && dpu_minor_ver > yyy)
	then enable feature;

/// probably folks will question this xxx and yyy as to what it means.

Can you please explain the single enumeration a bit more?

I thought we are going to move each revision to its separate file to 
avoid the conflicts as you suggested earlier in the comments. Isnt that 
enough?

>> .......
>> }
>>
>> But may I ask why? Since the manor/minor version macros handle this
>> nicely for us.
> 
> 
>
Dmitry Baryshkov June 6, 2023, 7:09 p.m. UTC | #13 
On 06/06/2023 20:51, Abhinav Kumar wrote:
> 
> 
> On 6/6/2023 4:14 AM, Dmitry Baryshkov wrote:
>> On Tue, 6 Jun 2023 at 05:35, Abhinav Kumar <quic_abhinavk@quicinc.com> 
>> wrote:
>>>
>>>
>>>
>>> On 6/5/2023 6:03 PM, Dmitry Baryshkov wrote:
>>>> On 06/06/2023 03:55, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 6/3/2023 7:21 PM, Dmitry Baryshkov wrote:
>>>>>> On 31/05/2023 21:25, Abhinav Kumar wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>>>>>> On 31/05/2023 06:05, Abhinav Kumar wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar
>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> With [1] dpu core revision was dropped in favor of using the
>>>>>>>>>>> compatible string from the device tree to select the dpu catalog
>>>>>>>>>>> being used in the device.
>>>>>>>>>>>
>>>>>>>>>>> This approach works well however also necessitates adding 
>>>>>>>>>>> catalog
>>>>>>>>>>> entries for small register level details as dpu capabilities 
>>>>>>>>>>> and/or
>>>>>>>>>>> features bloating the catalog unnecessarily. Examples include 
>>>>>>>>>>> but
>>>>>>>>>>> are not limited to data_compress, interrupt register set,
>>>>>>>>>>> widebus etc.
>>>>>>>>>>>
>>>>>>>>>>> Introduce the dpu core revision back as an entry to the catalog
>>>>>>>>>>> so that
>>>>>>>>>>> we can just use dpu revision checks and enable those bits which
>>>>>>>>>>> should be enabled unconditionally and not controlled by a 
>>>>>>>>>>> catalog
>>>>>>>>>>> and also simplify the changes to do something like:
>>>>>>>>>>>
>>>>>>>>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>>>>>>>>>>           enable the bit;
>>>>>>>>>>>
>>>>>>>>>>> Also, add some of the useful macros back to be able to use 
>>>>>>>>>>> dpu core
>>>>>>>>>>> revision effectively.
>>>>>>>>>>>
>>>>>>>>>>> [1]:
>>>>>>>>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>>>>>>> ---
>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>>>>>>>>>>>    .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31
>>>>>>>>>>> ++++++++++++++++++-
>>>>>>>>>>>    14 files changed, 43 insertions(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> [skipped catalog changes]
>>>>>>>>>>
>>>>>>>>>>> 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 677048cc3b7d..cc4aa75a1219 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>>> @@ -19,6 +19,33 @@
>>>>>>>>>>>     */
>>>>>>>>>>>    #define MAX_BLOCKS    12
>>>>>>>>>>>
>>>>>>>>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>>>>>>>>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>>>>>>>>>>> +                 ((MINOR & 0xFFF) << 16) |\
>>>>>>>>>>> +                 (STEP & 0xFFFF))
>>>>>>>>>>> +
>>>>>>>>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>>>>>>>>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>>>>>>>>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>>>>>>>>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>>>>>>>>>>> +
>>>>>>>>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>>>>>>>>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>>>>>>>>>>> +
>>>>>>>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>>>>>>>>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>>>>>>>>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>>>>>>>>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>>>>>>>>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>>>>>>>>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>>>>>>>>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
>>>>>>>>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
>>>>>>>>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>>>>>>>>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>>>>>>>>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>>>>>>>>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>>>>>>>>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>>>>>>>>>>
>>>>>>>>>> Instead of having defines for all SoCs (which can quickly become
>>>>>>>>>> unmanageable) and can cause merge conflicts, I'd suggest inlining
>>>>>>>>>> all
>>>>>>>>>> the defines into respective catalog files.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Sure, that can be done.
>>>>>>>>>
>>>>>>>>>> Also, I'm not sure that the "step" should be a part of the
>>>>>>>>>> catalog. I
>>>>>>>>>> know that this follows the hardware revision. However, please
>>>>>>>>>> correct
>>>>>>>>>> me if I'm wrong, different step levels are used for revisions 
>>>>>>>>>> of the
>>>>>>>>>> same SoC. The original code that was reading the hw revision from
>>>>>>>>>> the
>>>>>>>>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This is one of the things i noticed while making this change.
>>>>>>>>>
>>>>>>>>> Before the catalog rework, we used to handle even steps as we used
>>>>>>>>> to read that from the register and match it with the mdss_cfg
>>>>>>>>> handler. But after the rework, we dont handle steps anymore. Yes,
>>>>>>>>> you are right that different step levels are used for the
>>>>>>>>> revisions of the same SOC and so with that, i dont expect or
>>>>>>>>> atleast am not aware of DPU differences between steps but I am not
>>>>>>>>> able to rule it out.
>>>>>>>>>
>>>>>>>>> So are you suggesting we drop step altogether and DPU_HW_VER()
>>>>>>>>> macro shall only handle major and minor versions? With the current
>>>>>>>>> chipsets I see, it should not make a difference . Its just that I
>>>>>>>>> am not sure if that will never happen.
>>>>>>>>
>>>>>>>> Yes. The goal of this rework would be to drop generic features and
>>>>>>>> to replace those checks with DPU-revision lookups. Correct?
>>>>>>>
>>>>>>> Yes thats right.
>>>>>>>
>>>>>>>> I think that from this perspective having to handle toe step
>>>>>>>> revision is a sign of an overkill. Having to handle the step
>>>>>>>> revision is a sign of paltform feature (or mis-feature) rather than
>>>>>>>> a generic DPU bit.
>>>>>>>>
>>>>>>>
>>>>>>> Not entirely. Lets not forget that at the moment even dpu_perf_cfg
>>>>>>> is part of the catalog. Even if in terms of major HW blocks steps
>>>>>>> shouldnt change, there is absolutely no guarantee that perf data
>>>>>>> cannot.
>>>>>>>
>>>>>>> This is what is the sticking point for me which is holding me back
>>>>>>> against dropping step. Thoughts?
>>>>>>
>>>>>> We usually do not support ES versions of the chips, only the final
>>>>>> version. So supporting the perf data for earlier revisions is also
>>>>>> not required.
>>>>>>
>>>>>
>>>>> ack, we will drop step in that case. and good to know about the ES
>>>>> versions.
>>>>>
>>>>>>>
>>>>>>>> In fact I suppose that even handling a minor revision would be an
>>>>>>>> overkill. Why don't we start with .dpu_major instead of .core_rev?
>>>>>>>> We can add .dpu_minor if/when required.
>>>>>>>>
>>>>>>>
>>>>>>> No, unfortunately we cannot drop minor version for sure. I am seeing
>>>>>>> examples in downstream code where some of the features are available
>>>>>>> after a minor verion as well.
>>>>>>
>>>>>> Can you please give an example?
>>>>>>
>>>>>
>>>>> Yes, watchdog timer, intf reset counter are available only after DPU
>>>>> HW version 8.1 (not major version 8).
>>>>
>>>> Hmm, IIRC, wd timer was available for ages. Was it moved together with
>>>> the introduction of MDSS_PERIPH_0_REMOVED?
>>>>
>>>
>>> I am not sure of the timeline but its certainly tied to 8.1.
>>>
>>>> But anyway, I see your point. Let's have major and minor. I'd probably
>>>> still ask for the separate major and minor fields, if you don't mind.
>>>>
>>>
>>> hmmm so something like this?
>>>
>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0) /* 8998 v1.0 */
>>>
>>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>>> .......
>>> .dpu_major_rev = DPU_HW_MAJOR(DPU_HW_VER_300),
>>> .dpu_minor_rev = DPU_HW_MINOR(DPU_HW_VER_300)
>>
>> Just:
>>
>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>>      .dpu_major_rev = 3,
>>      .dpu_minor_rev = 0,
>>      /* .... */
>> };
>>
>> We do not need a single enumeration of all the versions. It can easily
>> become a source of merge conflicts.
>>
> 
> The issue with this approch is then the DPU_HW_VER_xxx macros become 
> redundant but we should keep them. Because in the code, its much more 
> readable to have
> 
> if (core_rev > DPU_HW_MAJOR(DPU_HW_VER_xxx))
>      then enable feature;
> 
> But now we will have to do
> 
> if (dpu_major_rev > xxx && dpu_minor_ver > yyy)
>      then enable feature;
> 
> /// probably folks will question this xxx and yyy as to what it means.

The first approach is less readable. It will require anybody to check, 
what is the major/minor of the mentioned XXX platform. In the second 
case we know exactly what we are looking for. E.g. we have new INTF 
interrupt addresses since 7.0. We have MDP_TOP_PERIPH0_REMOVED since 8.0 
(or 8.1?) We have PP_TE until 5.0 (not included) and INTF_TE since 5.0.

Having DPU_HW_VER_foo, I'd have to look up each time, what is the HW 
revision of sm8350, sc8280xp, sm8150, etc.

> 
> Can you please explain the single enumeration a bit more?
> 
> I thought we are going to move each revision to its separate file to 
> avoid the conflicts as you suggested earlier in the comments. Isnt that 
> enough?

This way you wouldn't be able to check against them in other modules.

> 
>>> .......
>>> }
>>>
>>> But may I ask why? Since the manor/minor version macros handle this
>>> nicely for us.
>>
>>
>>
Abhinav Kumar June 6, 2023, 7:28 p.m. UTC | #14 
On 6/6/2023 12:09 PM, Dmitry Baryshkov wrote:
> On 06/06/2023 20:51, Abhinav Kumar wrote:
>>
>>
>> On 6/6/2023 4:14 AM, Dmitry Baryshkov wrote:
>>> On Tue, 6 Jun 2023 at 05:35, Abhinav Kumar 
>>> <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/5/2023 6:03 PM, Dmitry Baryshkov wrote:
>>>>> On 06/06/2023 03:55, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 6/3/2023 7:21 PM, Dmitry Baryshkov wrote:
>>>>>>> On 31/05/2023 21:25, Abhinav Kumar wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>>>>>>> On 31/05/2023 06:05, Abhinav Kumar wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar
>>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> With [1] dpu core revision was dropped in favor of using the
>>>>>>>>>>>> compatible string from the device tree to select the dpu 
>>>>>>>>>>>> catalog
>>>>>>>>>>>> being used in the device.
>>>>>>>>>>>>
>>>>>>>>>>>> This approach works well however also necessitates adding 
>>>>>>>>>>>> catalog
>>>>>>>>>>>> entries for small register level details as dpu capabilities 
>>>>>>>>>>>> and/or
>>>>>>>>>>>> features bloating the catalog unnecessarily. Examples 
>>>>>>>>>>>> include but
>>>>>>>>>>>> are not limited to data_compress, interrupt register set,
>>>>>>>>>>>> widebus etc.
>>>>>>>>>>>>
>>>>>>>>>>>> Introduce the dpu core revision back as an entry to the catalog
>>>>>>>>>>>> so that
>>>>>>>>>>>> we can just use dpu revision checks and enable those bits which
>>>>>>>>>>>> should be enabled unconditionally and not controlled by a 
>>>>>>>>>>>> catalog
>>>>>>>>>>>> and also simplify the changes to do something like:
>>>>>>>>>>>>
>>>>>>>>>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>>>>>>>>>>>           enable the bit;
>>>>>>>>>>>>
>>>>>>>>>>>> Also, add some of the useful macros back to be able to use 
>>>>>>>>>>>> dpu core
>>>>>>>>>>>> revision effectively.
>>>>>>>>>>>>
>>>>>>>>>>>> [1]:
>>>>>>>>>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>>>>>>>>>>>>    .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31
>>>>>>>>>>>> ++++++++++++++++++-
>>>>>>>>>>>>    14 files changed, 43 insertions(+), 1 deletion(-)
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> [skipped catalog changes]
>>>>>>>>>>>
>>>>>>>>>>>> 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 677048cc3b7d..cc4aa75a1219 100644
>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>>>> @@ -19,6 +19,33 @@
>>>>>>>>>>>>     */
>>>>>>>>>>>>    #define MAX_BLOCKS    12
>>>>>>>>>>>>
>>>>>>>>>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>>>>>>>>>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>>>>>>>>>>>> +                 ((MINOR & 0xFFF) << 16) |\
>>>>>>>>>>>> +                 (STEP & 0xFFFF))
>>>>>>>>>>>> +
>>>>>>>>>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>>>>>>>>>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>>>>>>>>>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>>>>>>>>>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>>>>>>>>>>>> +
>>>>>>>>>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>>>>>>>>>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>>>>>>>>>>>> +
>>>>>>>>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>>>>>>>>>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>>>>>>>>>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>>>>>>>>>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>>>>>>>>>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>>>>>>>>>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>>>>>>>>>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
>>>>>>>>>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* 
>>>>>>>>>>>> qcm2290|sm4125 */
>>>>>>>>>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>>>>>>>>>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>>>>>>>>>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>>>>>>>>>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>>>>>>>>>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>>>>>>>>>>>
>>>>>>>>>>> Instead of having defines for all SoCs (which can quickly become
>>>>>>>>>>> unmanageable) and can cause merge conflicts, I'd suggest 
>>>>>>>>>>> inlining
>>>>>>>>>>> all
>>>>>>>>>>> the defines into respective catalog files.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Sure, that can be done.
>>>>>>>>>>
>>>>>>>>>>> Also, I'm not sure that the "step" should be a part of the
>>>>>>>>>>> catalog. I
>>>>>>>>>>> know that this follows the hardware revision. However, please
>>>>>>>>>>> correct
>>>>>>>>>>> me if I'm wrong, different step levels are used for revisions 
>>>>>>>>>>> of the
>>>>>>>>>>> same SoC. The original code that was reading the hw revision 
>>>>>>>>>>> from
>>>>>>>>>>> the
>>>>>>>>>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This is one of the things i noticed while making this change.
>>>>>>>>>>
>>>>>>>>>> Before the catalog rework, we used to handle even steps as we 
>>>>>>>>>> used
>>>>>>>>>> to read that from the register and match it with the mdss_cfg
>>>>>>>>>> handler. But after the rework, we dont handle steps anymore. Yes,
>>>>>>>>>> you are right that different step levels are used for the
>>>>>>>>>> revisions of the same SOC and so with that, i dont expect or
>>>>>>>>>> atleast am not aware of DPU differences between steps but I am 
>>>>>>>>>> not
>>>>>>>>>> able to rule it out.
>>>>>>>>>>
>>>>>>>>>> So are you suggesting we drop step altogether and DPU_HW_VER()
>>>>>>>>>> macro shall only handle major and minor versions? With the 
>>>>>>>>>> current
>>>>>>>>>> chipsets I see, it should not make a difference . Its just that I
>>>>>>>>>> am not sure if that will never happen.
>>>>>>>>>
>>>>>>>>> Yes. The goal of this rework would be to drop generic features and
>>>>>>>>> to replace those checks with DPU-revision lookups. Correct?
>>>>>>>>
>>>>>>>> Yes thats right.
>>>>>>>>
>>>>>>>>> I think that from this perspective having to handle toe step
>>>>>>>>> revision is a sign of an overkill. Having to handle the step
>>>>>>>>> revision is a sign of paltform feature (or mis-feature) rather 
>>>>>>>>> than
>>>>>>>>> a generic DPU bit.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Not entirely. Lets not forget that at the moment even dpu_perf_cfg
>>>>>>>> is part of the catalog. Even if in terms of major HW blocks steps
>>>>>>>> shouldnt change, there is absolutely no guarantee that perf data
>>>>>>>> cannot.
>>>>>>>>
>>>>>>>> This is what is the sticking point for me which is holding me back
>>>>>>>> against dropping step. Thoughts?
>>>>>>>
>>>>>>> We usually do not support ES versions of the chips, only the final
>>>>>>> version. So supporting the perf data for earlier revisions is also
>>>>>>> not required.
>>>>>>>
>>>>>>
>>>>>> ack, we will drop step in that case. and good to know about the ES
>>>>>> versions.
>>>>>>
>>>>>>>>
>>>>>>>>> In fact I suppose that even handling a minor revision would be an
>>>>>>>>> overkill. Why don't we start with .dpu_major instead of .core_rev?
>>>>>>>>> We can add .dpu_minor if/when required.
>>>>>>>>>
>>>>>>>>
>>>>>>>> No, unfortunately we cannot drop minor version for sure. I am 
>>>>>>>> seeing
>>>>>>>> examples in downstream code where some of the features are 
>>>>>>>> available
>>>>>>>> after a minor verion as well.
>>>>>>>
>>>>>>> Can you please give an example?
>>>>>>>
>>>>>>
>>>>>> Yes, watchdog timer, intf reset counter are available only after DPU
>>>>>> HW version 8.1 (not major version 8).
>>>>>
>>>>> Hmm, IIRC, wd timer was available for ages. Was it moved together with
>>>>> the introduction of MDSS_PERIPH_0_REMOVED?
>>>>>
>>>>
>>>> I am not sure of the timeline but its certainly tied to 8.1.
>>>>
>>>>> But anyway, I see your point. Let's have major and minor. I'd probably
>>>>> still ask for the separate major and minor fields, if you don't mind.
>>>>>
>>>>
>>>> hmmm so something like this?
>>>>
>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0) /* 8998 v1.0 */
>>>>
>>>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>>>> .......
>>>> .dpu_major_rev = DPU_HW_MAJOR(DPU_HW_VER_300),
>>>> .dpu_minor_rev = DPU_HW_MINOR(DPU_HW_VER_300)
>>>
>>> Just:
>>>
>>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>>>      .dpu_major_rev = 3,
>>>      .dpu_minor_rev = 0,
>>>      /* .... */
>>> };
>>>
>>> We do not need a single enumeration of all the versions. It can easily
>>> become a source of merge conflicts.
>>>
>>
>> The issue with this approch is then the DPU_HW_VER_xxx macros become 
>> redundant but we should keep them. Because in the code, its much more 
>> readable to have
>>
>> if (core_rev > DPU_HW_MAJOR(DPU_HW_VER_xxx))
>>      then enable feature;
>>
>> But now we will have to do
>>
>> if (dpu_major_rev > xxx && dpu_minor_ver > yyy)
>>      then enable feature;
>>
>> /// probably folks will question this xxx and yyy as to what it means.
> 
> The first approach is less readable. It will require anybody to check, 
> what is the major/minor of the mentioned XXX platform. In the second 
> case we know exactly what we are looking for. E.g. we have new INTF 
> interrupt addresses since 7.0. We have MDP_TOP_PERIPH0_REMOVED since 8.0 
> (or 8.1?) We have PP_TE until 5.0 (not included) and INTF_TE since 5.0.
> 
> Having DPU_HW_VER_foo, I'd have to look up each time, what is the HW 
> revision of sm8350, sc8280xp, sm8150, etc.
> 

Agreed, it will avoid one extra lookup but are you comfortable with a 
hard-coded number in the code? So for example.

if (dpu_major_rev > 0x3 && dpu_minor_ver > 0x5)
	enable feature;

Is this acceptable more than having to lookup?

Thats my only concern.
Dmitry Baryshkov June 6, 2023, 9:01 p.m. UTC | #15 
On 06/06/2023 22:28, Abhinav Kumar wrote:
> 
> 
> On 6/6/2023 12:09 PM, Dmitry Baryshkov wrote:
>> On 06/06/2023 20:51, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/6/2023 4:14 AM, Dmitry Baryshkov wrote:
>>>> On Tue, 6 Jun 2023 at 05:35, Abhinav Kumar 
>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 6/5/2023 6:03 PM, Dmitry Baryshkov wrote:
>>>>>> On 06/06/2023 03:55, Abhinav Kumar wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 6/3/2023 7:21 PM, Dmitry Baryshkov wrote:
>>>>>>>> On 31/05/2023 21:25, Abhinav Kumar wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>>>>>>>> On 31/05/2023 06:05, Abhinav Kumar wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar
>>>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> With [1] dpu core revision was dropped in favor of using the
>>>>>>>>>>>>> compatible string from the device tree to select the dpu 
>>>>>>>>>>>>> catalog
>>>>>>>>>>>>> being used in the device.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This approach works well however also necessitates adding 
>>>>>>>>>>>>> catalog
>>>>>>>>>>>>> entries for small register level details as dpu 
>>>>>>>>>>>>> capabilities and/or
>>>>>>>>>>>>> features bloating the catalog unnecessarily. Examples 
>>>>>>>>>>>>> include but
>>>>>>>>>>>>> are not limited to data_compress, interrupt register set,
>>>>>>>>>>>>> widebus etc.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Introduce the dpu core revision back as an entry to the 
>>>>>>>>>>>>> catalog
>>>>>>>>>>>>> so that
>>>>>>>>>>>>> we can just use dpu revision checks and enable those bits 
>>>>>>>>>>>>> which
>>>>>>>>>>>>> should be enabled unconditionally and not controlled by a 
>>>>>>>>>>>>> catalog
>>>>>>>>>>>>> and also simplify the changes to do something like:
>>>>>>>>>>>>>
>>>>>>>>>>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>>>>>>>>>>>>           enable the bit;
>>>>>>>>>>>>>
>>>>>>>>>>>>> Also, add some of the useful macros back to be able to use 
>>>>>>>>>>>>> dpu core
>>>>>>>>>>>>> revision effectively.
>>>>>>>>>>>>>
>>>>>>>>>>>>> [1]:
>>>>>>>>>>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>>>>>>>>>>>>>    .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31
>>>>>>>>>>>>> ++++++++++++++++++-
>>>>>>>>>>>>>    14 files changed, 43 insertions(+), 1 deletion(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> [skipped catalog changes]
>>>>>>>>>>>>
>>>>>>>>>>>>> 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 677048cc3b7d..cc4aa75a1219 100644
>>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>>>>> @@ -19,6 +19,33 @@
>>>>>>>>>>>>>     */
>>>>>>>>>>>>>    #define MAX_BLOCKS    12
>>>>>>>>>>>>>
>>>>>>>>>>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>>>>>>>>>>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>>>>>>>>>>>>> +                 ((MINOR & 0xFFF) << 16) |\
>>>>>>>>>>>>> +                 (STEP & 0xFFFF))
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>>>>>>>>>>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>>>>>>>>>>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>>>>>>>>>>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>>>>>>>>>>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>>>>>>>>>>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>>>>>>>>>>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>>>>>>>>>>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>>>>>>>>>>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>>>>>>>>>>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>>>>>>>>>>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* 
>>>>>>>>>>>>> sm6115|sm4250 */
>>>>>>>>>>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* 
>>>>>>>>>>>>> qcm2290|sm4125 */
>>>>>>>>>>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>>>>>>>>>>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>>>>>>>>>>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>>>>>>>>>>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>>>>>>>>>>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>>>>>>>>>>>>
>>>>>>>>>>>> Instead of having defines for all SoCs (which can quickly 
>>>>>>>>>>>> become
>>>>>>>>>>>> unmanageable) and can cause merge conflicts, I'd suggest 
>>>>>>>>>>>> inlining
>>>>>>>>>>>> all
>>>>>>>>>>>> the defines into respective catalog files.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Sure, that can be done.
>>>>>>>>>>>
>>>>>>>>>>>> Also, I'm not sure that the "step" should be a part of the
>>>>>>>>>>>> catalog. I
>>>>>>>>>>>> know that this follows the hardware revision. However, please
>>>>>>>>>>>> correct
>>>>>>>>>>>> me if I'm wrong, different step levels are used for 
>>>>>>>>>>>> revisions of the
>>>>>>>>>>>> same SoC. The original code that was reading the hw revision 
>>>>>>>>>>>> from
>>>>>>>>>>>> the
>>>>>>>>>>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> This is one of the things i noticed while making this change.
>>>>>>>>>>>
>>>>>>>>>>> Before the catalog rework, we used to handle even steps as we 
>>>>>>>>>>> used
>>>>>>>>>>> to read that from the register and match it with the mdss_cfg
>>>>>>>>>>> handler. But after the rework, we dont handle steps anymore. 
>>>>>>>>>>> Yes,
>>>>>>>>>>> you are right that different step levels are used for the
>>>>>>>>>>> revisions of the same SOC and so with that, i dont expect or
>>>>>>>>>>> atleast am not aware of DPU differences between steps but I 
>>>>>>>>>>> am not
>>>>>>>>>>> able to rule it out.
>>>>>>>>>>>
>>>>>>>>>>> So are you suggesting we drop step altogether and DPU_HW_VER()
>>>>>>>>>>> macro shall only handle major and minor versions? With the 
>>>>>>>>>>> current
>>>>>>>>>>> chipsets I see, it should not make a difference . Its just 
>>>>>>>>>>> that I
>>>>>>>>>>> am not sure if that will never happen.
>>>>>>>>>>
>>>>>>>>>> Yes. The goal of this rework would be to drop generic features 
>>>>>>>>>> and
>>>>>>>>>> to replace those checks with DPU-revision lookups. Correct?
>>>>>>>>>
>>>>>>>>> Yes thats right.
>>>>>>>>>
>>>>>>>>>> I think that from this perspective having to handle toe step
>>>>>>>>>> revision is a sign of an overkill. Having to handle the step
>>>>>>>>>> revision is a sign of paltform feature (or mis-feature) rather 
>>>>>>>>>> than
>>>>>>>>>> a generic DPU bit.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Not entirely. Lets not forget that at the moment even dpu_perf_cfg
>>>>>>>>> is part of the catalog. Even if in terms of major HW blocks steps
>>>>>>>>> shouldnt change, there is absolutely no guarantee that perf data
>>>>>>>>> cannot.
>>>>>>>>>
>>>>>>>>> This is what is the sticking point for me which is holding me back
>>>>>>>>> against dropping step. Thoughts?
>>>>>>>>
>>>>>>>> We usually do not support ES versions of the chips, only the final
>>>>>>>> version. So supporting the perf data for earlier revisions is also
>>>>>>>> not required.
>>>>>>>>
>>>>>>>
>>>>>>> ack, we will drop step in that case. and good to know about the ES
>>>>>>> versions.
>>>>>>>
>>>>>>>>>
>>>>>>>>>> In fact I suppose that even handling a minor revision would be an
>>>>>>>>>> overkill. Why don't we start with .dpu_major instead of 
>>>>>>>>>> .core_rev?
>>>>>>>>>> We can add .dpu_minor if/when required.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> No, unfortunately we cannot drop minor version for sure. I am 
>>>>>>>>> seeing
>>>>>>>>> examples in downstream code where some of the features are 
>>>>>>>>> available
>>>>>>>>> after a minor verion as well.
>>>>>>>>
>>>>>>>> Can you please give an example?
>>>>>>>>
>>>>>>>
>>>>>>> Yes, watchdog timer, intf reset counter are available only after DPU
>>>>>>> HW version 8.1 (not major version 8).
>>>>>>
>>>>>> Hmm, IIRC, wd timer was available for ages. Was it moved together 
>>>>>> with
>>>>>> the introduction of MDSS_PERIPH_0_REMOVED?
>>>>>>
>>>>>
>>>>> I am not sure of the timeline but its certainly tied to 8.1.
>>>>>
>>>>>> But anyway, I see your point. Let's have major and minor. I'd 
>>>>>> probably
>>>>>> still ask for the separate major and minor fields, if you don't mind.
>>>>>>
>>>>>
>>>>> hmmm so something like this?
>>>>>
>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0) /* 8998 v1.0 */
>>>>>
>>>>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>>>>> .......
>>>>> .dpu_major_rev = DPU_HW_MAJOR(DPU_HW_VER_300),
>>>>> .dpu_minor_rev = DPU_HW_MINOR(DPU_HW_VER_300)
>>>>
>>>> Just:
>>>>
>>>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>>>>      .dpu_major_rev = 3,
>>>>      .dpu_minor_rev = 0,
>>>>      /* .... */
>>>> };
>>>>
>>>> We do not need a single enumeration of all the versions. It can easily
>>>> become a source of merge conflicts.
>>>>
>>>
>>> The issue with this approch is then the DPU_HW_VER_xxx macros become 
>>> redundant but we should keep them. Because in the code, its much more 
>>> readable to have
>>>
>>> if (core_rev > DPU_HW_MAJOR(DPU_HW_VER_xxx))
>>>      then enable feature;
>>>
>>> But now we will have to do
>>>
>>> if (dpu_major_rev > xxx && dpu_minor_ver > yyy)
>>>      then enable feature;
>>>
>>> /// probably folks will question this xxx and yyy as to what it means.
>>
>> The first approach is less readable. It will require anybody to check, 
>> what is the major/minor of the mentioned XXX platform. In the second 
>> case we know exactly what we are looking for. E.g. we have new INTF 
>> interrupt addresses since 7.0. We have MDP_TOP_PERIPH0_REMOVED since 
>> 8.0 (or 8.1?) We have PP_TE until 5.0 (not included) and INTF_TE since 
>> 5.0.
>>
>> Having DPU_HW_VER_foo, I'd have to look up each time, what is the HW 
>> revision of sm8350, sc8280xp, sm8150, etc.
>>
> 
> Agreed, it will avoid one extra lookup but are you comfortable with a 
> hard-coded number in the code? So for example.
> 
> if (dpu_major_rev > 0x3 && dpu_minor_ver > 0x5)
>      enable feature;
> 
> Is this acceptable more than having to lookup?

Yes, definitely.

Note that this is not a correct condition for what you have meant.

> 
> Thats my only concern.
Dmitry Baryshkov June 6, 2023, 9:08 p.m. UTC | #16 
On 07/06/2023 00:01, Dmitry Baryshkov wrote:
> On 06/06/2023 22:28, Abhinav Kumar wrote:
>>
>>
>> On 6/6/2023 12:09 PM, Dmitry Baryshkov wrote:
>>> On 06/06/2023 20:51, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/6/2023 4:14 AM, Dmitry Baryshkov wrote:
>>>>> On Tue, 6 Jun 2023 at 05:35, Abhinav Kumar 
>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 6/5/2023 6:03 PM, Dmitry Baryshkov wrote:
>>>>>>> On 06/06/2023 03:55, Abhinav Kumar wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/3/2023 7:21 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On 31/05/2023 21:25, Abhinav Kumar wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On 31/05/2023 06:05, Abhinav Kumar wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar
>>>>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> With [1] dpu core revision was dropped in favor of using the
>>>>>>>>>>>>>> compatible string from the device tree to select the dpu 
>>>>>>>>>>>>>> catalog
>>>>>>>>>>>>>> being used in the device.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This approach works well however also necessitates adding 
>>>>>>>>>>>>>> catalog
>>>>>>>>>>>>>> entries for small register level details as dpu 
>>>>>>>>>>>>>> capabilities and/or
>>>>>>>>>>>>>> features bloating the catalog unnecessarily. Examples 
>>>>>>>>>>>>>> include but
>>>>>>>>>>>>>> are not limited to data_compress, interrupt register set,
>>>>>>>>>>>>>> widebus etc.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Introduce the dpu core revision back as an entry to the 
>>>>>>>>>>>>>> catalog
>>>>>>>>>>>>>> so that
>>>>>>>>>>>>>> we can just use dpu revision checks and enable those bits 
>>>>>>>>>>>>>> which
>>>>>>>>>>>>>> should be enabled unconditionally and not controlled by a 
>>>>>>>>>>>>>> catalog
>>>>>>>>>>>>>> and also simplify the changes to do something like:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>>>>>>>>>>>>>           enable the bit;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Also, add some of the useful macros back to be able to use 
>>>>>>>>>>>>>> dpu core
>>>>>>>>>>>>>> revision effectively.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [1]:
>>>>>>>>>>>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>>>>>>>>>>>>>>    .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31
>>>>>>>>>>>>>> ++++++++++++++++++-
>>>>>>>>>>>>>>    14 files changed, 43 insertions(+), 1 deletion(-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> [skipped catalog changes]
>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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 677048cc3b7d..cc4aa75a1219 100644
>>>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>>>>>> @@ -19,6 +19,33 @@
>>>>>>>>>>>>>>     */
>>>>>>>>>>>>>>    #define MAX_BLOCKS    12
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>>>>>>>>>>>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>>>>>>>>>>>>>> +                 ((MINOR & 0xFFF) << 16) |\
>>>>>>>>>>>>>> +                 (STEP & 0xFFFF))
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>>>>>>>>>>>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>>>>>>>>>>>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>>>>>>>>>>>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>>>>>>>>>>>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>>>>>>>>>>>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>>>>>>>>>>>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>>>>>>>>>>>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>>>>>>>>>>>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>>>>>>>>>>>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>>>>>>>>>>>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* 
>>>>>>>>>>>>>> sm6115|sm4250 */
>>>>>>>>>>>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* 
>>>>>>>>>>>>>> qcm2290|sm4125 */
>>>>>>>>>>>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>>>>>>>>>>>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>>>>>>>>>>>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>>>>>>>>>>>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>>>>>>>>>>>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>>>>>>>>>>>>>
>>>>>>>>>>>>> Instead of having defines for all SoCs (which can quickly 
>>>>>>>>>>>>> become
>>>>>>>>>>>>> unmanageable) and can cause merge conflicts, I'd suggest 
>>>>>>>>>>>>> inlining
>>>>>>>>>>>>> all
>>>>>>>>>>>>> the defines into respective catalog files.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Sure, that can be done.
>>>>>>>>>>>>
>>>>>>>>>>>>> Also, I'm not sure that the "step" should be a part of the
>>>>>>>>>>>>> catalog. I
>>>>>>>>>>>>> know that this follows the hardware revision. However, please
>>>>>>>>>>>>> correct
>>>>>>>>>>>>> me if I'm wrong, different step levels are used for 
>>>>>>>>>>>>> revisions of the
>>>>>>>>>>>>> same SoC. The original code that was reading the hw 
>>>>>>>>>>>>> revision from
>>>>>>>>>>>>> the
>>>>>>>>>>>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> This is one of the things i noticed while making this change.
>>>>>>>>>>>>
>>>>>>>>>>>> Before the catalog rework, we used to handle even steps as 
>>>>>>>>>>>> we used
>>>>>>>>>>>> to read that from the register and match it with the mdss_cfg
>>>>>>>>>>>> handler. But after the rework, we dont handle steps anymore. 
>>>>>>>>>>>> Yes,
>>>>>>>>>>>> you are right that different step levels are used for the
>>>>>>>>>>>> revisions of the same SOC and so with that, i dont expect or
>>>>>>>>>>>> atleast am not aware of DPU differences between steps but I 
>>>>>>>>>>>> am not
>>>>>>>>>>>> able to rule it out.
>>>>>>>>>>>>
>>>>>>>>>>>> So are you suggesting we drop step altogether and DPU_HW_VER()
>>>>>>>>>>>> macro shall only handle major and minor versions? With the 
>>>>>>>>>>>> current
>>>>>>>>>>>> chipsets I see, it should not make a difference . Its just 
>>>>>>>>>>>> that I
>>>>>>>>>>>> am not sure if that will never happen.
>>>>>>>>>>>
>>>>>>>>>>> Yes. The goal of this rework would be to drop generic 
>>>>>>>>>>> features and
>>>>>>>>>>> to replace those checks with DPU-revision lookups. Correct?
>>>>>>>>>>
>>>>>>>>>> Yes thats right.
>>>>>>>>>>
>>>>>>>>>>> I think that from this perspective having to handle toe step
>>>>>>>>>>> revision is a sign of an overkill. Having to handle the step
>>>>>>>>>>> revision is a sign of paltform feature (or mis-feature) 
>>>>>>>>>>> rather than
>>>>>>>>>>> a generic DPU bit.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Not entirely. Lets not forget that at the moment even 
>>>>>>>>>> dpu_perf_cfg
>>>>>>>>>> is part of the catalog. Even if in terms of major HW blocks steps
>>>>>>>>>> shouldnt change, there is absolutely no guarantee that perf data
>>>>>>>>>> cannot.
>>>>>>>>>>
>>>>>>>>>> This is what is the sticking point for me which is holding me 
>>>>>>>>>> back
>>>>>>>>>> against dropping step. Thoughts?
>>>>>>>>>
>>>>>>>>> We usually do not support ES versions of the chips, only the final
>>>>>>>>> version. So supporting the perf data for earlier revisions is also
>>>>>>>>> not required.
>>>>>>>>>
>>>>>>>>
>>>>>>>> ack, we will drop step in that case. and good to know about the ES
>>>>>>>> versions.
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> In fact I suppose that even handling a minor revision would 
>>>>>>>>>>> be an
>>>>>>>>>>> overkill. Why don't we start with .dpu_major instead of 
>>>>>>>>>>> .core_rev?
>>>>>>>>>>> We can add .dpu_minor if/when required.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> No, unfortunately we cannot drop minor version for sure. I am 
>>>>>>>>>> seeing
>>>>>>>>>> examples in downstream code where some of the features are 
>>>>>>>>>> available
>>>>>>>>>> after a minor verion as well.
>>>>>>>>>
>>>>>>>>> Can you please give an example?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yes, watchdog timer, intf reset counter are available only after 
>>>>>>>> DPU
>>>>>>>> HW version 8.1 (not major version 8).
>>>>>>>
>>>>>>> Hmm, IIRC, wd timer was available for ages. Was it moved together 
>>>>>>> with
>>>>>>> the introduction of MDSS_PERIPH_0_REMOVED?
>>>>>>>
>>>>>>
>>>>>> I am not sure of the timeline but its certainly tied to 8.1.
>>>>>>
>>>>>>> But anyway, I see your point. Let's have major and minor. I'd 
>>>>>>> probably
>>>>>>> still ask for the separate major and minor fields, if you don't 
>>>>>>> mind.
>>>>>>>
>>>>>>
>>>>>> hmmm so something like this?
>>>>>>
>>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0) /* 8998 v1.0 */
>>>>>>
>>>>>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>>>>>> .......
>>>>>> .dpu_major_rev = DPU_HW_MAJOR(DPU_HW_VER_300),
>>>>>> .dpu_minor_rev = DPU_HW_MINOR(DPU_HW_VER_300)
>>>>>
>>>>> Just:
>>>>>
>>>>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>>>>>      .dpu_major_rev = 3,
>>>>>      .dpu_minor_rev = 0,
>>>>>      /* .... */
>>>>> };
>>>>>
>>>>> We do not need a single enumeration of all the versions. It can easily
>>>>> become a source of merge conflicts.
>>>>>
>>>>
>>>> The issue with this approch is then the DPU_HW_VER_xxx macros become 
>>>> redundant but we should keep them. Because in the code, its much 
>>>> more readable to have
>>>>
>>>> if (core_rev > DPU_HW_MAJOR(DPU_HW_VER_xxx))
>>>>      then enable feature;
>>>>
>>>> But now we will have to do
>>>>
>>>> if (dpu_major_rev > xxx && dpu_minor_ver > yyy)
>>>>      then enable feature;
>>>>
>>>> /// probably folks will question this xxx and yyy as to what it means.
>>>
>>> The first approach is less readable. It will require anybody to 
>>> check, what is the major/minor of the mentioned XXX platform. In the 
>>> second case we know exactly what we are looking for. E.g. we have new 
>>> INTF interrupt addresses since 7.0. We have MDP_TOP_PERIPH0_REMOVED 
>>> since 8.0 (or 8.1?) We have PP_TE until 5.0 (not included) and 
>>> INTF_TE since 5.0.
>>>
>>> Having DPU_HW_VER_foo, I'd have to look up each time, what is the HW 
>>> revision of sm8350, sc8280xp, sm8150, etc.
>>>
>>
>> Agreed, it will avoid one extra lookup but are you comfortable with a 
>> hard-coded number in the code? So for example.
>>
>> if (dpu_major_rev > 0x3 && dpu_minor_ver > 0x5)
>>      enable feature;
>>
>> Is this acceptable more than having to lookup?
> 
> Yes, definitely.
> 
> Note that this is not a correct condition for what you have meant.

Please excuse me for not being explicit. I assumed we have to enable 
feature since 4.6 (major > 3, minor > 5)

Proper condition would be:

if (dpu_major_rev > 4 ||
     (dpu_major_rev == 4 && dpu_minor_rev >= 6))
Abhinav Kumar June 6, 2023, 9:16 p.m. UTC | #17 
On 6/6/2023 2:08 PM, Dmitry Baryshkov wrote:
> On 07/06/2023 00:01, Dmitry Baryshkov wrote:
>> On 06/06/2023 22:28, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/6/2023 12:09 PM, Dmitry Baryshkov wrote:
>>>> On 06/06/2023 20:51, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 6/6/2023 4:14 AM, Dmitry Baryshkov wrote:
>>>>>> On Tue, 6 Jun 2023 at 05:35, Abhinav Kumar 
>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 6/5/2023 6:03 PM, Dmitry Baryshkov wrote:
>>>>>>>> On 06/06/2023 03:55, Abhinav Kumar wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 6/3/2023 7:21 PM, Dmitry Baryshkov wrote:
>>>>>>>>>> On 31/05/2023 21:25, Abhinav Kumar wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>> On 31/05/2023 06:05, Abhinav Kumar wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>>>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar
>>>>>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> With [1] dpu core revision was dropped in favor of using the
>>>>>>>>>>>>>>> compatible string from the device tree to select the dpu 
>>>>>>>>>>>>>>> catalog
>>>>>>>>>>>>>>> being used in the device.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This approach works well however also necessitates adding 
>>>>>>>>>>>>>>> catalog
>>>>>>>>>>>>>>> entries for small register level details as dpu 
>>>>>>>>>>>>>>> capabilities and/or
>>>>>>>>>>>>>>> features bloating the catalog unnecessarily. Examples 
>>>>>>>>>>>>>>> include but
>>>>>>>>>>>>>>> are not limited to data_compress, interrupt register set,
>>>>>>>>>>>>>>> widebus etc.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Introduce the dpu core revision back as an entry to the 
>>>>>>>>>>>>>>> catalog
>>>>>>>>>>>>>>> so that
>>>>>>>>>>>>>>> we can just use dpu revision checks and enable those bits 
>>>>>>>>>>>>>>> which
>>>>>>>>>>>>>>> should be enabled unconditionally and not controlled by a 
>>>>>>>>>>>>>>> catalog
>>>>>>>>>>>>>>> and also simplify the changes to do something like:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>>>>>>>>>>>>>>           enable the bit;
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Also, add some of the useful macros back to be able to 
>>>>>>>>>>>>>>> use dpu core
>>>>>>>>>>>>>>> revision effectively.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> [1]:
>>>>>>>>>>>>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 +
>>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 +
>>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 +
>>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 +
>>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 +
>>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 +
>>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 +
>>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 +
>>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 +
>>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 +
>>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 +
>>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 +
>>>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 +
>>>>>>>>>>>>>>>    .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31
>>>>>>>>>>>>>>> ++++++++++++++++++-
>>>>>>>>>>>>>>>    14 files changed, 43 insertions(+), 1 deletion(-)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [skipped catalog changes]
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 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 677048cc3b7d..cc4aa75a1219 100644
>>>>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>>>>>>> @@ -19,6 +19,33 @@
>>>>>>>>>>>>>>>     */
>>>>>>>>>>>>>>>    #define MAX_BLOCKS    12
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>>>>>>>>>>>>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>>>>>>>>>>>>>>> +                 ((MINOR & 0xFFF) << 16) |\
>>>>>>>>>>>>>>> +                 (STEP & 0xFFFF))
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>>>>>>>>>>>>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>>>>>>>>>>>>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>>>>>>>>>>>>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>>>>>>>>>>>>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>>>>>>>>>>>>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 
>>>>>>>>>>>>>>> v1.0 */
>>>>>>>>>>>>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 
>>>>>>>>>>>>>>> v1.0 */
>>>>>>>>>>>>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>>>>>>>>>>>>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>>>>>>>>>>>>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 
>>>>>>>>>>>>>>> v1.0 */
>>>>>>>>>>>>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* 
>>>>>>>>>>>>>>> sm6115|sm4250 */
>>>>>>>>>>>>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* 
>>>>>>>>>>>>>>> qcm2290|sm4125 */
>>>>>>>>>>>>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>>>>>>>>>>>>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>>>>>>>>>>>>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>>>>>>>>>>>>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>>>>>>>>>>>>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Instead of having defines for all SoCs (which can quickly 
>>>>>>>>>>>>>> become
>>>>>>>>>>>>>> unmanageable) and can cause merge conflicts, I'd suggest 
>>>>>>>>>>>>>> inlining
>>>>>>>>>>>>>> all
>>>>>>>>>>>>>> the defines into respective catalog files.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Sure, that can be done.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Also, I'm not sure that the "step" should be a part of the
>>>>>>>>>>>>>> catalog. I
>>>>>>>>>>>>>> know that this follows the hardware revision. However, please
>>>>>>>>>>>>>> correct
>>>>>>>>>>>>>> me if I'm wrong, different step levels are used for 
>>>>>>>>>>>>>> revisions of the
>>>>>>>>>>>>>> same SoC. The original code that was reading the hw 
>>>>>>>>>>>>>> revision from
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> This is one of the things i noticed while making this change.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Before the catalog rework, we used to handle even steps as 
>>>>>>>>>>>>> we used
>>>>>>>>>>>>> to read that from the register and match it with the mdss_cfg
>>>>>>>>>>>>> handler. But after the rework, we dont handle steps 
>>>>>>>>>>>>> anymore. Yes,
>>>>>>>>>>>>> you are right that different step levels are used for the
>>>>>>>>>>>>> revisions of the same SOC and so with that, i dont expect or
>>>>>>>>>>>>> atleast am not aware of DPU differences between steps but I 
>>>>>>>>>>>>> am not
>>>>>>>>>>>>> able to rule it out.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So are you suggesting we drop step altogether and DPU_HW_VER()
>>>>>>>>>>>>> macro shall only handle major and minor versions? With the 
>>>>>>>>>>>>> current
>>>>>>>>>>>>> chipsets I see, it should not make a difference . Its just 
>>>>>>>>>>>>> that I
>>>>>>>>>>>>> am not sure if that will never happen.
>>>>>>>>>>>>
>>>>>>>>>>>> Yes. The goal of this rework would be to drop generic 
>>>>>>>>>>>> features and
>>>>>>>>>>>> to replace those checks with DPU-revision lookups. Correct?
>>>>>>>>>>>
>>>>>>>>>>> Yes thats right.
>>>>>>>>>>>
>>>>>>>>>>>> I think that from this perspective having to handle toe step
>>>>>>>>>>>> revision is a sign of an overkill. Having to handle the step
>>>>>>>>>>>> revision is a sign of paltform feature (or mis-feature) 
>>>>>>>>>>>> rather than
>>>>>>>>>>>> a generic DPU bit.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Not entirely. Lets not forget that at the moment even 
>>>>>>>>>>> dpu_perf_cfg
>>>>>>>>>>> is part of the catalog. Even if in terms of major HW blocks 
>>>>>>>>>>> steps
>>>>>>>>>>> shouldnt change, there is absolutely no guarantee that perf data
>>>>>>>>>>> cannot.
>>>>>>>>>>>
>>>>>>>>>>> This is what is the sticking point for me which is holding me 
>>>>>>>>>>> back
>>>>>>>>>>> against dropping step. Thoughts?
>>>>>>>>>>
>>>>>>>>>> We usually do not support ES versions of the chips, only the 
>>>>>>>>>> final
>>>>>>>>>> version. So supporting the perf data for earlier revisions is 
>>>>>>>>>> also
>>>>>>>>>> not required.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ack, we will drop step in that case. and good to know about the ES
>>>>>>>>> versions.
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> In fact I suppose that even handling a minor revision would 
>>>>>>>>>>>> be an
>>>>>>>>>>>> overkill. Why don't we start with .dpu_major instead of 
>>>>>>>>>>>> .core_rev?
>>>>>>>>>>>> We can add .dpu_minor if/when required.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> No, unfortunately we cannot drop minor version for sure. I am 
>>>>>>>>>>> seeing
>>>>>>>>>>> examples in downstream code where some of the features are 
>>>>>>>>>>> available
>>>>>>>>>>> after a minor verion as well.
>>>>>>>>>>
>>>>>>>>>> Can you please give an example?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yes, watchdog timer, intf reset counter are available only 
>>>>>>>>> after DPU
>>>>>>>>> HW version 8.1 (not major version 8).
>>>>>>>>
>>>>>>>> Hmm, IIRC, wd timer was available for ages. Was it moved 
>>>>>>>> together with
>>>>>>>> the introduction of MDSS_PERIPH_0_REMOVED?
>>>>>>>>
>>>>>>>
>>>>>>> I am not sure of the timeline but its certainly tied to 8.1.
>>>>>>>
>>>>>>>> But anyway, I see your point. Let's have major and minor. I'd 
>>>>>>>> probably
>>>>>>>> still ask for the separate major and minor fields, if you don't 
>>>>>>>> mind.
>>>>>>>>
>>>>>>>
>>>>>>> hmmm so something like this?
>>>>>>>
>>>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0) /* 8998 v1.0 */
>>>>>>>
>>>>>>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>>>>>>> .......
>>>>>>> .dpu_major_rev = DPU_HW_MAJOR(DPU_HW_VER_300),
>>>>>>> .dpu_minor_rev = DPU_HW_MINOR(DPU_HW_VER_300)
>>>>>>
>>>>>> Just:
>>>>>>
>>>>>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>>>>>>      .dpu_major_rev = 3,
>>>>>>      .dpu_minor_rev = 0,
>>>>>>      /* .... */
>>>>>> };
>>>>>>
>>>>>> We do not need a single enumeration of all the versions. It can 
>>>>>> easily
>>>>>> become a source of merge conflicts.
>>>>>>
>>>>>
>>>>> The issue with this approch is then the DPU_HW_VER_xxx macros 
>>>>> become redundant but we should keep them. Because in the code, its 
>>>>> much more readable to have
>>>>>
>>>>> if (core_rev > DPU_HW_MAJOR(DPU_HW_VER_xxx))
>>>>>      then enable feature;
>>>>>
>>>>> But now we will have to do
>>>>>
>>>>> if (dpu_major_rev > xxx && dpu_minor_ver > yyy)
>>>>>      then enable feature;
>>>>>
>>>>> /// probably folks will question this xxx and yyy as to what it means.
>>>>
>>>> The first approach is less readable. It will require anybody to 
>>>> check, what is the major/minor of the mentioned XXX platform. In the 
>>>> second case we know exactly what we are looking for. E.g. we have 
>>>> new INTF interrupt addresses since 7.0. We have 
>>>> MDP_TOP_PERIPH0_REMOVED since 8.0 (or 8.1?) We have PP_TE until 5.0 
>>>> (not included) and INTF_TE since 5.0.
>>>>
>>>> Having DPU_HW_VER_foo, I'd have to look up each time, what is the HW 
>>>> revision of sm8350, sc8280xp, sm8150, etc.
>>>>
>>>
>>> Agreed, it will avoid one extra lookup but are you comfortable with a 
>>> hard-coded number in the code? So for example.
>>>
>>> if (dpu_major_rev > 0x3 && dpu_minor_ver > 0x5)
>>>      enable feature;
>>>
>>> Is this acceptable more than having to lookup?
>>
>> Yes, definitely.
>>
>> Note that this is not a correct condition for what you have meant.
> 
> Please excuse me for not being explicit. I assumed we have to enable 
> feature since 4.6 (major > 3, minor > 5)
> 
> Proper condition would be:
> 
> if (dpu_major_rev > 4 ||
>      (dpu_major_rev == 4 && dpu_minor_rev >= 6))
> 

Ah okay, I really didnt focus too much on the condition itself but more 
on the fact that we will now use hard-coded revision numbers in the code.

Will spin a v2 with the things that were agreed on. Thanks.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
index 3c732a0360c7..16c2861e0359 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
@@ -185,6 +185,7 @@  static const struct dpu_perf_cfg msm8998_perf_data = {
 };
 
 const struct dpu_mdss_cfg dpu_msm8998_cfg = {
+	.core_rev = DPU_HW_VER_300,
 	.caps = &msm8998_dpu_caps,
 	.ubwc = &msm8998_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(msm8998_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
index 36ea1af10894..1c003935c948 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
@@ -183,6 +183,7 @@  static const struct dpu_perf_cfg sdm845_perf_data = {
 };
 
 const struct dpu_mdss_cfg dpu_sdm845_cfg = {
+	.core_rev = DPU_HW_VER_400,
 	.caps = &sdm845_dpu_caps,
 	.ubwc = &sdm845_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sdm845_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
index b5f751354267..8c914be62a88 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
@@ -208,6 +208,7 @@  static const struct dpu_perf_cfg sm8150_perf_data = {
 };
 
 const struct dpu_mdss_cfg dpu_sm8150_cfg = {
+	.core_rev = DPU_HW_VER_500,
 	.caps = &sm8150_dpu_caps,
 	.ubwc = &sm8150_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sm8150_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
index 8ed2b263c5ea..9465bde128eb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
@@ -214,6 +214,7 @@  static const struct dpu_perf_cfg sc8180x_perf_data = {
 };
 
 const struct dpu_mdss_cfg dpu_sc8180x_cfg = {
+	.core_rev = DPU_HW_VER_510,
 	.caps = &sc8180x_dpu_caps,
 	.ubwc = &sc8180x_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sc8180x_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
index daebd2170041..1b04ecfb7cde 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
@@ -214,6 +214,7 @@  static const struct dpu_perf_cfg sm8250_perf_data = {
 };
 
 const struct dpu_mdss_cfg dpu_sm8250_cfg = {
+	.core_rev = DPU_HW_VER_600,
 	.caps = &sm8250_dpu_caps,
 	.ubwc = &sm8250_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sm8250_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
index 0b05da2592c0..16e905e35025 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
@@ -128,6 +128,7 @@  static const struct dpu_perf_cfg sc7180_perf_data = {
 };
 
 const struct dpu_mdss_cfg dpu_sc7180_cfg = {
+	.core_rev = DPU_HW_VER_620,
 	.caps = &sc7180_dpu_caps,
 	.ubwc = &sc7180_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sc7180_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
index ba9de008519b..87ad7a765e4c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
@@ -103,6 +103,7 @@  static const struct dpu_perf_cfg sm6115_perf_data = {
 };
 
 const struct dpu_mdss_cfg dpu_sm6115_cfg = {
+	.core_rev = DPU_HW_VER_630,
 	.caps = &sm6115_dpu_caps,
 	.ubwc = &sm6115_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sm6115_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
index 92ac348eea6b..a61140ab96ed 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
@@ -93,6 +93,7 @@  static const struct dpu_perf_cfg qcm2290_perf_data = {
 };
 
 const struct dpu_mdss_cfg dpu_qcm2290_cfg = {
+	.core_rev = DPU_HW_VER_650,
 	.caps = &qcm2290_dpu_caps,
 	.ubwc = &qcm2290_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(qcm2290_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
index 3c1b2c13398d..01abce7a311c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
@@ -201,6 +201,7 @@  static const struct dpu_perf_cfg sm8350_perf_data = {
 };
 
 const struct dpu_mdss_cfg dpu_sm8350_cfg = {
+	.core_rev = DPU_HW_VER_700,
 	.caps = &sm8350_dpu_caps,
 	.ubwc = &sm8350_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sm8350_mdp),
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 5d894cbb0a62..4294f1d35d25 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
@@ -141,6 +141,7 @@  static const struct dpu_perf_cfg sc7280_perf_data = {
 };
 
 const struct dpu_mdss_cfg dpu_sc7280_cfg = {
+	.core_rev = DPU_HW_VER_720,
 	.caps = &sc7280_dpu_caps,
 	.ubwc = &sc7280_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sc7280_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
index c3f1ae000a21..2108e531f13b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
@@ -203,6 +203,7 @@  static const struct dpu_perf_cfg sc8280xp_perf_data = {
 };
 
 const struct dpu_mdss_cfg dpu_sc8280xp_cfg = {
+	.core_rev = DPU_HW_VER_800,
 	.caps = &sc8280xp_dpu_caps,
 	.ubwc = &sc8280xp_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sc8280xp_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
index 86c2e68ebd2c..b8d5d0ee8c82 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
@@ -209,6 +209,7 @@  static const struct dpu_perf_cfg sm8450_perf_data = {
 };
 
 const struct dpu_mdss_cfg dpu_sm8450_cfg = {
+	.core_rev = DPU_HW_VER_810,
 	.caps = &sm8450_dpu_caps,
 	.ubwc = &sm8450_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sm8450_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
index 85dc34458b88..87a7c06e3024 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
@@ -213,6 +213,7 @@  static const struct dpu_perf_cfg sm8550_perf_data = {
 };
 
 const struct dpu_mdss_cfg dpu_sm8550_cfg = {
+	.core_rev = DPU_HW_VER_900,
 	.caps = &sm8550_dpu_caps,
 	.ubwc = &sm8550_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sm8550_mdp),
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 677048cc3b7d..cc4aa75a1219 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -19,6 +19,33 @@ 
  */
 #define MAX_BLOCKS    12
 
+#define DPU_HW_VER(MAJOR, MINOR, STEP)\
+		  ((((unsigned int)MAJOR & 0xF) << 28) |\
+		  ((MINOR & 0xFFF) << 16) |\
+		  (STEP & 0xFFFF))
+
+#define DPU_HW_MAJOR(rev)((rev) >> 28)
+#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
+#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
+#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
+
+#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
+(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
+
+#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
+#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
+#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
+#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
+#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
+#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
+#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
+#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
+#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
+#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
+#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
+#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
+#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
+
 #define DPU_HW_BLK_NAME_LEN	16
 
 #define MAX_IMG_WIDTH 0x3fff
@@ -769,7 +796,7 @@  struct dpu_perf_cfg {
 /**
  * struct dpu_mdss_cfg - information of MDSS HW
  * This is the main catalog data structure representing
- * this HW version. Contains number of instances,
+ * this HW version. Contains dpu core revision, number of instances,
  * register offsets, capabilities of the all MDSS HW sub-blocks.
  *
  * @dma_formats        Supported formats for dma pipe
@@ -778,6 +805,8 @@  struct dpu_perf_cfg {
  * @mdss_irqs:         Bitmap with the irqs supported by the target
  */
 struct dpu_mdss_cfg {
+	u32 core_rev;
+
 	const struct dpu_caps *caps;
 
 	const struct dpu_ubwc_cfg *ubwc;