Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
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 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 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 > x && dpu_core_revision < x) 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=4 Signed-off-by: Abhinav Kumar --- .../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 & 0x)) + +#define DPU_HW_MAJOR(rev)((rev) >> 28) +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF) +#define DPU_HW_STEP(rev)((rev) & 0x) +#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
Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
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 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 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 > x && dpu_core_revision < x) 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=4 Signed-off-by: Abhinav Kumar --- .../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 & 0x)) + +#define DPU_HW_MAJOR(rev)((rev) >> 28) +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF) +#define DPU_HW_STEP(rev)((rev) & 0x) +#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
Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
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 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 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 > x && dpu_core_revision < x) 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=4 Signed-off-by: Abhinav Kumar --- .../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 & 0x)) + +#define DPU_HW_MAJOR(rev)((rev) >> 28) +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF) +#define DPU_HW_STEP(rev)((rev) & 0x) +#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
Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
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 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 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 > x && dpu_core_revision < x) 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=4 Signed-off-by: Abhinav Kumar --- .../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 & 0x)) + +#define DPU_HW_MAJOR(rev)((rev) >> 28) +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF) +#define DPU_HW_STEP(rev)((rev) & 0x) +#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
Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
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 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 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 > x && dpu_core_revision < x) 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=4 Signed-off-by: Abhinav Kumar --- .../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 & 0x)) + +#define DPU_HW_MAJOR(rev)((rev) >> 28) +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF) +#define DPU_HW_STEP(rev)((rev) & 0x) +#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
Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
On 6/6/2023 4:14 AM, Dmitry Baryshkov wrote: On Tue, 6 Jun 2023 at 05:35, Abhinav Kumar 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 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 > x && dpu_core_revision < x) 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=4 Signed-off-by: Abhinav Kumar --- .../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_BLOCKS12 +#define DPU_HW_VER(MAJOR, MINOR, STEP)\ + unsigned int)MAJOR & 0xF) << 28) |\ + ((MINOR & 0xFFF) << 16) |\ + (STEP & 0x)) + +#define DPU_HW_MAJOR(rev)((rev) >> 28) +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF) +#define DPU_HW_STEP(rev)((rev) & 0x) +#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
Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
On Tue, 6 Jun 2023 at 05:35, Abhinav Kumar 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 > >>> 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 > x && dpu_core_revision < x) > 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=4 > > Signed-off-by: Abhinav Kumar > --- > .../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_BLOCKS12 > > +#define DPU_HW_VER(MAJOR, MINOR, STEP)\ > + unsigned int)MAJOR & 0xF) << 28) |\ > + ((MINOR & 0xFFF) << 16) |\ > + (STEP & 0x)) > + > +#define DPU_HW_MAJOR(rev)((rev) >> 28) > +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF) > +#define DPU_HW_STEP(rev)((rev) & 0x) > +#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
Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
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 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 > x && dpu_core_revision < x) 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=4 Signed-off-by: Abhinav Kumar --- .../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 & 0x)) + +#define DPU_HW_MAJOR(rev)((rev) >> 28) +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF) +#define DPU_HW_STEP(rev)((rev) & 0x) +#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
Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
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 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 > x && dpu_core_revision < x) 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=4 Signed-off-by: Abhinav Kumar --- .../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 & 0x)) + +#define DPU_HW_MAJOR(rev)((rev) >> 28) +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF) +#define DPU_HW_STEP(rev)((rev) & 0x) +#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
Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
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 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 > x && dpu_core_revision < x) 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=4 Signed-off-by: Abhinav Kumar --- .../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 & 0x)) + +#define DPU_HW_MAJOR(rev)((rev) >> 28) +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF) +#define DPU_HW_STEP(rev)((rev) & 0x) +#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
Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
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 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 > x && dpu_core_revision < x) 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=4 Signed-off-by: Abhinav Kumar --- .../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 & 0x)) + +#define DPU_HW_MAJOR(rev)((rev) >> 28) +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF) +#define DPU_HW_STEP(rev)((rev) & 0x) +#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
Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
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 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 > x && dpu_core_revision < x) 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=4 Signed-off-by: Abhinav Kumar --- .../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 & 0x)) + +#define DPU_HW_MAJOR(rev)((rev) >> 28) +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF) +#define DPU_HW_STEP(rev)((rev) & 0x) +#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.
Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
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 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 > x && dpu_core_revision < x) 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=4 Signed-off-by: Abhinav Kumar --- .../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 & 0x)) + +#define DPU_HW_MAJOR(rev)((rev) >> 28) +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF) +#define DPU_HW_STEP(rev)((rev) & 0x) +#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
Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
On Wed, 31 May 2023 at 03:54, 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 > x && dpu_core_revision < x)
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=4
Signed-off-by: Abhinav Kumar
---
.../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_BLOCKS12
+#define DPU_HW_VER(MAJOR, MINOR, STEP)\
+ unsigned int)MAJOR & 0xF) << 28) |\
+ ((MINOR & 0xFFF) << 16) |\
+ (STEP & 0x))
+
+#define DPU_HW_MAJOR(rev)((rev) >> 28)
+#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
+#define DPU_HW_STEP(rev)((rev) & 0x)
+#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_LEN16
#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
