| Message ID | 20230521171026.4159495-1-dmitry.baryshkov@linaro.org |
|---|---|
| Headers | show |
| Series | drm/msm/dpu: use UBWC data from MDSS driver | expand |
On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote: > DPU programming requires knowledge of some of UBWC parameters. This > results in duplication of UBWC data between MDSS and DPU drivers. Export > the required data from MDSS driver. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/msm_mdss.c | 30 +++++++++++++----------------- > drivers/gpu/drm/msm/msm_mdss.h | 27 +++++++++++++++++++++++++++ > 2 files changed, 40 insertions(+), 17 deletions(-) > create mode 100644 drivers/gpu/drm/msm/msm_mdss.h > > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c > index d1e57099b517..ed836c659688 100644 > --- a/drivers/gpu/drm/msm/msm_mdss.c > +++ b/drivers/gpu/drm/msm/msm_mdss.c > @@ -13,7 +13,7 @@ > #include <linux/pm_runtime.h> > #include <linux/reset.h> > > -#include "msm_drv.h" > +#include "msm_mdss.h" > #include "msm_kms.h" > > #define HW_REV 0x0 > @@ -26,16 +26,6 @@ > > #define MIN_IB_BW 400000000UL /* Min ib vote 400MB */ > > -struct msm_mdss_data { > - u32 ubwc_enc_version; > - /* can be read from register 0x58 */ > - u32 ubwc_dec_version; > - u32 ubwc_swizzle; > - u32 ubwc_static; > - u32 highest_bank_bit; > - u32 macrotile_mode; > -}; > - > struct msm_mdss { > struct device *dev; > > @@ -185,12 +175,6 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss) > return 0; > } > > -#define UBWC_1_0 0x10000000 > -#define UBWC_2_0 0x20000000 > -#define UBWC_3_0 0x30000000 > -#define UBWC_4_0 0x40000000 > -#define UBWC_4_3 0x40030000 > - > static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss) > { > const struct msm_mdss_data *data = msm_mdss->mdss_data; > @@ -236,6 +220,18 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss) > } > } > > +const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev) > +{ > + struct msm_mdss *mdss; > + > + if (!dev) > + return ERR_PTR(-EINVAL); > + > + mdss = dev_get_drvdata(dev); > + > + return mdss->mdss_data; > +} > + > static int msm_mdss_enable(struct msm_mdss *msm_mdss) > { > int ret; > diff --git a/drivers/gpu/drm/msm/msm_mdss.h b/drivers/gpu/drm/msm/msm_mdss.h > new file mode 100644 > index 000000000000..02bbab42adbc > --- /dev/null > +++ b/drivers/gpu/drm/msm/msm_mdss.h > @@ -0,0 +1,27 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2018, The Linux Foundation > + */ Fix the copyright year . Apart from that, Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
On Thu, 27 Jul 2023 at 00:21, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote: > > DPU programming requires knowledge of some of UBWC parameters. This > > results in duplication of UBWC data between MDSS and DPU drivers. Export > > the required data from MDSS driver. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/msm_mdss.c | 30 +++++++++++++----------------- > > drivers/gpu/drm/msm/msm_mdss.h | 27 +++++++++++++++++++++++++++ > > 2 files changed, 40 insertions(+), 17 deletions(-) > > create mode 100644 drivers/gpu/drm/msm/msm_mdss.h > > > > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c > > index d1e57099b517..ed836c659688 100644 > > --- a/drivers/gpu/drm/msm/msm_mdss.c > > +++ b/drivers/gpu/drm/msm/msm_mdss.c > > @@ -13,7 +13,7 @@ > > #include <linux/pm_runtime.h> > > #include <linux/reset.h> > > > > -#include "msm_drv.h" > > +#include "msm_mdss.h" > > #include "msm_kms.h" > > > > #define HW_REV 0x0 > > @@ -26,16 +26,6 @@ > > > > #define MIN_IB_BW 400000000UL /* Min ib vote 400MB */ > > > > -struct msm_mdss_data { > > - u32 ubwc_enc_version; > > - /* can be read from register 0x58 */ > > - u32 ubwc_dec_version; > > - u32 ubwc_swizzle; > > - u32 ubwc_static; > > - u32 highest_bank_bit; > > - u32 macrotile_mode; > > -}; > > - > > struct msm_mdss { > > struct device *dev; > > > > @@ -185,12 +175,6 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss) > > return 0; > > } > > > > -#define UBWC_1_0 0x10000000 > > -#define UBWC_2_0 0x20000000 > > -#define UBWC_3_0 0x30000000 > > -#define UBWC_4_0 0x40000000 > > -#define UBWC_4_3 0x40030000 > > - > > static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss) > > { > > const struct msm_mdss_data *data = msm_mdss->mdss_data; > > @@ -236,6 +220,18 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss) > > } > > } > > > > +const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev) > > +{ > > + struct msm_mdss *mdss; > > + > > + if (!dev) > > + return ERR_PTR(-EINVAL); > > + > > + mdss = dev_get_drvdata(dev); > > + > > + return mdss->mdss_data; > > +} > > + > > static int msm_mdss_enable(struct msm_mdss *msm_mdss) > > { > > int ret; > > diff --git a/drivers/gpu/drm/msm/msm_mdss.h b/drivers/gpu/drm/msm/msm_mdss.h > > new file mode 100644 > > index 000000000000..02bbab42adbc > > --- /dev/null > > +++ b/drivers/gpu/drm/msm/msm_mdss.h > > @@ -0,0 +1,27 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * Copyright (c) 2018, The Linux Foundation > > + */ > > Fix the copyright year . No, the copyright year is correct. The data was moved from the c file (which has this copyright) and there are no copyrightable additions. > > Apart from that, > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote: > As we are going to use MDSS data for DPU programming, populate missing > MDSS data. The UBWC 1.0 and no UBWC cases do not require MDSS > programming, so skip them. > Can you pls point me to the downstream references you used for msm8998? Was that just taken from catalog? If so I would ask for the reference for the catalog too. As per the register the decoder version is 0x0 and not 1. Even encoder version is 0 from what i see and not 1. Thats why a dec_version of UBWC_1_0 is not doing anything i assume. Some additional questions: 1) Does the whole chunk in dpu_hw_sspp_setup_format() which handles ubwc programming need to be protected by if (ctx->ubwc) now ? 2) The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx. What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the catalog for the encoder version in the first place? Because looking at the registers UBWC_x_x is the correct value. > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/msm_mdss.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c > index ed836c659688..9bb7be4b9ebb 100644 > --- a/drivers/gpu/drm/msm/msm_mdss.c > +++ b/drivers/gpu/drm/msm/msm_mdss.c > @@ -264,6 +264,10 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss) > * UBWC_n and the rest of params comes from hw data. > */ > switch (msm_mdss->mdss_data->ubwc_dec_version) { > + case 0: /* no UBWC */ > + case UBWC_1_0: > + /* do nothing */ > + break; > case UBWC_2_0: > msm_mdss_setup_ubwc_dec_20(msm_mdss); > break; > @@ -502,10 +506,22 @@ static int mdss_remove(struct platform_device *pdev) > return 0; > } > > +static const struct msm_mdss_data msm8998_data = { > + .ubwc_enc_version = UBWC_1_0, > + .ubwc_dec_version = UBWC_1_0, > + .highest_bank_bit = 1, > +}; > + > +static const struct msm_mdss_data qcm2290_data = { > + /* no UBWC */ > + .highest_bank_bit = 0x2, > +}; > + > static const struct msm_mdss_data sc7180_data = { > .ubwc_enc_version = UBWC_2_0, > .ubwc_dec_version = UBWC_2_0, > .ubwc_static = 0x1e, > + .highest_bank_bit = 0x3, > }; > > static const struct msm_mdss_data sc7280_data = { > @@ -550,6 +566,7 @@ static const struct msm_mdss_data sm6115_data = { > .ubwc_dec_version = UBWC_2_0, > .ubwc_swizzle = 7, > .ubwc_static = 0x11f, > + .highest_bank_bit = 0x1, > }; > > static const struct msm_mdss_data sm8250_data = { > @@ -574,8 +591,8 @@ static const struct msm_mdss_data sm8550_data = { > > static const struct of_device_id mdss_dt_match[] = { > { .compatible = "qcom,mdss" }, > - { .compatible = "qcom,msm8998-mdss" }, > - { .compatible = "qcom,qcm2290-mdss" }, > + { .compatible = "qcom,msm8998-mdss", .data = &msm8998_data }, > + { .compatible = "qcom,qcm2290-mdss", .data = &qcm2290_data }, > { .compatible = "qcom,sdm845-mdss", .data = &sdm845_data }, > { .compatible = "qcom,sc7180-mdss", .data = &sc7180_data }, > { .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
On Thu, 27 Jul 2023 at 01:30, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote: > > As we are going to use MDSS data for DPU programming, populate missing > > MDSS data. The UBWC 1.0 and no UBWC cases do not require MDSS > > programming, so skip them. > > > > Can you pls point me to the downstream references you used for msm8998? msm-3.18, drivers/video/msm/mdss/mdss_mdp.c See the function mdss_mdp_hw_rev_caps_init(). It sets has_ubwc for MDP 1.07 (msm8996), 1.14 (msm8937) / 1.16 (msm8953) and 3.0 (msm8998). > Was that just taken from catalog? If so I would ask for the reference > for the catalog too. > > As per the register the decoder version is 0x0 and not 1. > > Even encoder version is 0 from what i see and not 1. Thats why a > dec_version of UBWC_1_0 is not doing anything i assume. > > Some additional questions: > > 1) Does the whole chunk in dpu_hw_sspp_setup_format() which handles ubwc > programming need to be protected by if (ctx->ubwc) now ? It's hard to discuss the question which is irrelevant for this patch. Nevertheless, yes, it needs to be protected because e.g. qcm2290 doesn't have UBWC support. > 2) The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx. > What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the > catalog for the encoder version in the first place? Because looking at > the registers UBWC_x_x is the correct value. Huh. This definitely should be asked next to the code that you wish to discuss. The DPU_HW_UBWC_VER_xx values come from the first DPU revision. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/msm_mdss.c | 21 +++++++++++++++++++-- > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c > > index ed836c659688..9bb7be4b9ebb 100644 > > --- a/drivers/gpu/drm/msm/msm_mdss.c > > +++ b/drivers/gpu/drm/msm/msm_mdss.c > > @@ -264,6 +264,10 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss) > > * UBWC_n and the rest of params comes from hw data. > > */ > > switch (msm_mdss->mdss_data->ubwc_dec_version) { > > + case 0: /* no UBWC */ > > + case UBWC_1_0: > > + /* do nothing */ > > + break; > > case UBWC_2_0: > > msm_mdss_setup_ubwc_dec_20(msm_mdss); > > break; > > @@ -502,10 +506,22 @@ static int mdss_remove(struct platform_device *pdev) > > return 0; > > } > > > > +static const struct msm_mdss_data msm8998_data = { > > + .ubwc_enc_version = UBWC_1_0, > > + .ubwc_dec_version = UBWC_1_0, > > + .highest_bank_bit = 1, > > +}; > > + > > +static const struct msm_mdss_data qcm2290_data = { > > + /* no UBWC */ > > + .highest_bank_bit = 0x2, > > +}; > > + > > static const struct msm_mdss_data sc7180_data = { > > .ubwc_enc_version = UBWC_2_0, > > .ubwc_dec_version = UBWC_2_0, > > .ubwc_static = 0x1e, > > + .highest_bank_bit = 0x3, > > }; > > > > static const struct msm_mdss_data sc7280_data = { > > @@ -550,6 +566,7 @@ static const struct msm_mdss_data sm6115_data = { > > .ubwc_dec_version = UBWC_2_0, > > .ubwc_swizzle = 7, > > .ubwc_static = 0x11f, > > + .highest_bank_bit = 0x1, > > }; > > > > static const struct msm_mdss_data sm8250_data = { > > @@ -574,8 +591,8 @@ static const struct msm_mdss_data sm8550_data = { > > > > static const struct of_device_id mdss_dt_match[] = { > > { .compatible = "qcom,mdss" }, > > - { .compatible = "qcom,msm8998-mdss" }, > > - { .compatible = "qcom,qcm2290-mdss" }, > > + { .compatible = "qcom,msm8998-mdss", .data = &msm8998_data }, > > + { .compatible = "qcom,qcm2290-mdss", .data = &qcm2290_data }, > > { .compatible = "qcom,sdm845-mdss", .data = &sdm845_data }, > > { .compatible = "qcom,sc7180-mdss", .data = &sc7180_data }, > > { .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
On 7/26/2023 3:58 PM, Dmitry Baryshkov wrote: > On Thu, 27 Jul 2023 at 01:30, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote: >>> As we are going to use MDSS data for DPU programming, populate missing >>> MDSS data. The UBWC 1.0 and no UBWC cases do not require MDSS >>> programming, so skip them. >>> >> >> Can you pls point me to the downstream references you used for msm8998? > > msm-3.18, drivers/video/msm/mdss/mdss_mdp.c > > See the function mdss_mdp_hw_rev_caps_init(). It sets has_ubwc for MDP > 1.07 (msm8996), 1.14 (msm8937) / 1.16 (msm8953) and 3.0 (msm8998). > It sets has_ubwc but I still cannot locate where it says version is 1.0. Because the 0x58 register reads 0 and not 1 for msm8998. >> Was that just taken from catalog? If so I would ask for the reference >> for the catalog too. >> >> As per the register the decoder version is 0x0 and not 1. >> >> Even encoder version is 0 from what i see and not 1. Thats why a >> dec_version of UBWC_1_0 is not doing anything i assume. >> >> Some additional questions: >> >> 1) Does the whole chunk in dpu_hw_sspp_setup_format() which handles ubwc >> programming need to be protected by if (ctx->ubwc) now ? > > It's hard to discuss the question which is irrelevant for this patch. > Nevertheless, yes, it needs to be protected because e.g. qcm2290 > doesn't have UBWC support. > Sorry but your commit text made me look into this function and wonder now what happens to that code. But I will continue this in this next patch so that you can see the code and the question together. >> 2) The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx. >> What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the >> catalog for the encoder version in the first place? Because looking at >> the registers UBWC_x_x is the correct value. > > Huh. This definitely should be asked next to the code that you wish to > discuss. The DPU_HW_UBWC_VER_xx values come from the first DPU > revision. > Sure, so again the next patch. >> >> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/msm_mdss.c | 21 +++++++++++++++++++-- >>> 1 file changed, 19 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c >>> index ed836c659688..9bb7be4b9ebb 100644 >>> --- a/drivers/gpu/drm/msm/msm_mdss.c >>> +++ b/drivers/gpu/drm/msm/msm_mdss.c >>> @@ -264,6 +264,10 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss) >>> * UBWC_n and the rest of params comes from hw data. >>> */ >>> switch (msm_mdss->mdss_data->ubwc_dec_version) { >>> + case 0: /* no UBWC */ >>> + case UBWC_1_0: >>> + /* do nothing */ >>> + break; >>> case UBWC_2_0: >>> msm_mdss_setup_ubwc_dec_20(msm_mdss); >>> break; >>> @@ -502,10 +506,22 @@ static int mdss_remove(struct platform_device *pdev) >>> return 0; >>> } >>> >>> +static const struct msm_mdss_data msm8998_data = { >>> + .ubwc_enc_version = UBWC_1_0, >>> + .ubwc_dec_version = UBWC_1_0, >>> + .highest_bank_bit = 1, >>> +}; >>> + >>> +static const struct msm_mdss_data qcm2290_data = { >>> + /* no UBWC */ >>> + .highest_bank_bit = 0x2, >>> +}; >>> + >>> static const struct msm_mdss_data sc7180_data = { >>> .ubwc_enc_version = UBWC_2_0, >>> .ubwc_dec_version = UBWC_2_0, >>> .ubwc_static = 0x1e, >>> + .highest_bank_bit = 0x3, >>> }; >>> >>> static const struct msm_mdss_data sc7280_data = { >>> @@ -550,6 +566,7 @@ static const struct msm_mdss_data sm6115_data = { >>> .ubwc_dec_version = UBWC_2_0, >>> .ubwc_swizzle = 7, >>> .ubwc_static = 0x11f, >>> + .highest_bank_bit = 0x1, >>> }; >>> >>> static const struct msm_mdss_data sm8250_data = { >>> @@ -574,8 +591,8 @@ static const struct msm_mdss_data sm8550_data = { >>> >>> static const struct of_device_id mdss_dt_match[] = { >>> { .compatible = "qcom,mdss" }, >>> - { .compatible = "qcom,msm8998-mdss" }, >>> - { .compatible = "qcom,qcm2290-mdss" }, >>> + { .compatible = "qcom,msm8998-mdss", .data = &msm8998_data }, >>> + { .compatible = "qcom,qcm2290-mdss", .data = &qcm2290_data }, >>> { .compatible = "qcom,sdm845-mdss", .data = &sdm845_data }, >>> { .compatible = "qcom,sc7180-mdss", .data = &sc7180_data }, >>> { .compatible = "qcom,sc7280-mdss", .data = &sc7280_data }, > > >
On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote: > Switch to using data from MDSS driver to program the SSPP fetch and UBWC > configuration. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++------- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 7 +++++-- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 16 +++++++++++++++- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 + > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 3 ++- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 ++ > 6 files changed, 36 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > index cf70a9bd1034..bfd82c2921af 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > @@ -8,6 +8,8 @@ > #include "dpu_hw_sspp.h" > #include "dpu_kms.h" > > +#include "msm_mdss.h" > + > #include <drm/drm_file.h> > > #define DPU_FETCH_CONFIG_RESET_VALUE 0x00000087 > @@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct dpu_sw_pipe *pipe, > DPU_REG_WRITE(c, SSPP_FETCH_CONFIG, > DPU_FETCH_CONFIG_RESET_VALUE | > ctx->ubwc->highest_bank_bit << 18); Does this needs to be protected with if ctx->ubwc check? > - switch (ctx->ubwc->ubwc_version) { > - case DPU_HW_UBWC_VER_10: > + switch (ctx->ubwc->ubwc_enc_version) { > + case UBWC_1_0: The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx. What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the catalog for the encoder version in the first place? Because looking at the registers UBWC_x_x is the correct value. If we cannot find the reason why, it should be noted in the commit text that the values we are using did change. > fast_clear = fmt->alpha_enable ? BIT(31) : 0; > DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL, > fast_clear | (ctx->ubwc->ubwc_swizzle & 0x1) | > BIT(8) | > (ctx->ubwc->highest_bank_bit << 4)); > break; > - case DPU_HW_UBWC_VER_20: > + case UBWC_2_0: > fast_clear = fmt->alpha_enable ? BIT(31) : 0; > DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL, > fast_clear | (ctx->ubwc->ubwc_swizzle) | > (ctx->ubwc->highest_bank_bit << 4)); > break; > - case DPU_HW_UBWC_VER_30: > + case UBWC_3_0: > DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL, > BIT(30) | (ctx->ubwc->ubwc_swizzle) | > (ctx->ubwc->highest_bank_bit << 4)); > break; > - case DPU_HW_UBWC_VER_40: > + case UBWC_4_0: > DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL, > DPU_FORMAT_IS_YUV(fmt) ? 0 : BIT(30)); > break; > @@ -793,7 +795,9 @@ static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp, > } > > struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx, > - void __iomem *addr, const struct dpu_mdss_cfg *catalog) > + void __iomem *addr, > + const struct dpu_mdss_cfg *catalog, > + const struct msm_mdss_data *mdss_data) > { > struct dpu_hw_sspp *hw_pipe; > const struct dpu_sspp_cfg *cfg; > @@ -813,7 +817,7 @@ struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx, > > /* Assign ops */ > hw_pipe->catalog = catalog; > - hw_pipe->ubwc = catalog->ubwc; > + hw_pipe->ubwc = mdss_data; > hw_pipe->idx = idx; > hw_pipe->cap = cfg; > _setup_layer_ops(hw_pipe, hw_pipe->cap->features); > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > index 74b98b6b3bc3..8d4ae9d24674 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > @@ -351,7 +351,7 @@ struct dpu_hw_sspp { > struct dpu_hw_blk base; > struct dpu_hw_blk_reg_map hw; > const struct dpu_mdss_cfg *catalog; > - const struct dpu_ubwc_cfg *ubwc; > + const struct msm_mdss_data *ubwc; > > /* Pipe */ > enum dpu_sspp idx; > @@ -368,9 +368,12 @@ struct dpu_kms; > * @idx: Pipe index for which driver object is required > * @addr: Mapped register io address of MDP > * @catalog : Pointer to mdss catalog data > + * @mdss_data: UBWC / MDSS configuration data > */ > struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx, > - void __iomem *addr, const struct dpu_mdss_cfg *catalog); > + void __iomem *addr, > + const struct dpu_mdss_cfg *catalog, > + const struct msm_mdss_data *mdss_data); > > /** > * dpu_hw_sspp_destroy(): Destroys SSPP driver context > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 0e7a68714e9e..a4293dc0b61b 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -22,6 +22,7 @@ > > #include "msm_drv.h" > #include "msm_mmu.h" > +#include "msm_mdss.h" > #include "msm_gem.h" > #include "disp/msm_disp_snapshot.h" > > @@ -1066,7 +1067,20 @@ static int dpu_kms_hw_init(struct msm_kms *kms) > goto power_error; > } > > - rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mmio); > + dpu_kms->mdss = msm_mdss_get_mdss_data(dpu_kms->pdev->dev.parent); > + if (IS_ERR(dpu_kms->mdss)) { > + rc = PTR_ERR(dpu_kms->mdss); > + DPU_ERROR("failed to get MDSS data: %d\n", rc); > + goto power_error; > + } > + > + if (!dpu_kms->mdss) { > + rc = -EINVAL; > + DPU_ERROR("NULL MDSS data\n"); > + goto power_error; > + } > + > + rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mdss, dpu_kms->mmio); > if (rc) { > DPU_ERROR("rm init failed: %d\n", rc); > goto power_error; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > index aca39a4689f4..797b4ff3e806 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > @@ -69,6 +69,7 @@ struct dpu_kms { > struct msm_kms base; > struct drm_device *dev; > const struct dpu_mdss_cfg *catalog; > + const struct msm_mdss_data *mdss; > > /* io/register spaces: */ > void __iomem *mmio, *vbif[VBIF_MAX], *reg_dma; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > index f4dda88a73f7..9ee493465c4b 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > @@ -100,6 +100,7 @@ int dpu_rm_destroy(struct dpu_rm *rm) > > int dpu_rm_init(struct dpu_rm *rm, > const struct dpu_mdss_cfg *cat, > + const struct msm_mdss_data *mdss_data, > void __iomem *mmio) > { > int rc, i; > @@ -268,7 +269,7 @@ int dpu_rm_init(struct dpu_rm *rm, > continue; > } > > - hw = dpu_hw_sspp_init(sspp->id, mmio, cat); > + hw = dpu_hw_sspp_init(sspp->id, mmio, cat, mdss_data); > if (IS_ERR(hw)) { > rc = PTR_ERR(hw); > DPU_ERROR("failed sspp object creation: err %d\n", rc); > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > index d62c2edb2460..2b551566cbf4 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > @@ -40,11 +40,13 @@ struct dpu_rm { > * for all HW blocks. > * @rm: DPU Resource Manager handle > * @cat: Pointer to hardware catalog > + * @mdss_data: Pointer to MDSS / UBWC configuration > * @mmio: mapped register io address of MDP > * @Return: 0 on Success otherwise -ERROR > */ > int dpu_rm_init(struct dpu_rm *rm, > const struct dpu_mdss_cfg *cat, > + const struct msm_mdss_data *mdss_data, > void __iomem *mmio); > > /**
On Thu, 27 Jul 2023 at 02:20, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote: > > Switch to using data from MDSS driver to program the SSPP fetch and UBWC > > configuration. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++------- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 7 +++++-- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 16 +++++++++++++++- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 + > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 3 ++- > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 ++ > > 6 files changed, 36 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > > index cf70a9bd1034..bfd82c2921af 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > > @@ -8,6 +8,8 @@ > > #include "dpu_hw_sspp.h" > > #include "dpu_kms.h" > > > > +#include "msm_mdss.h" > > + > > #include <drm/drm_file.h> > > > > #define DPU_FETCH_CONFIG_RESET_VALUE 0x00000087 > > @@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct dpu_sw_pipe *pipe, > > DPU_REG_WRITE(c, SSPP_FETCH_CONFIG, > > DPU_FETCH_CONFIG_RESET_VALUE | > > ctx->ubwc->highest_bank_bit << 18); > > Does this needs to be protected with if ctx->ubwc check? Yes... And it should have been already. > > > - switch (ctx->ubwc->ubwc_version) { > > - case DPU_HW_UBWC_VER_10: > > + switch (ctx->ubwc->ubwc_enc_version) { > > + case UBWC_1_0: > > The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx. > What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the > catalog for the encoder version in the first place? Because looking at > the registers UBWC_x_x is the correct value. > > If we cannot find the reason why, it should be noted in the commit text > that the values we are using did change. Huh? This is just an enum. It isn't a part of uABI, nor it is written to the hardware. > > > fast_clear = fmt->alpha_enable ? BIT(31) : 0; > > DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL, > > fast_clear | (ctx->ubwc->ubwc_swizzle & 0x1) | > > BIT(8) | > > (ctx->ubwc->highest_bank_bit << 4)); > > break; > > - case DPU_HW_UBWC_VER_20: > > + case UBWC_2_0: > > fast_clear = fmt->alpha_enable ? BIT(31) : 0; > > DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL, > > fast_clear | (ctx->ubwc->ubwc_swizzle) | > > (ctx->ubwc->highest_bank_bit << 4)); > > break; > > - case DPU_HW_UBWC_VER_30: > > + case UBWC_3_0: > > DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL, > > BIT(30) | (ctx->ubwc->ubwc_swizzle) | > > (ctx->ubwc->highest_bank_bit << 4)); > > break; > > - case DPU_HW_UBWC_VER_40: > > + case UBWC_4_0: > > DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL, > > DPU_FORMAT_IS_YUV(fmt) ? 0 : BIT(30)); > > break; > > @@ -793,7 +795,9 @@ static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp, > > } > > > > struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx, > > - void __iomem *addr, const struct dpu_mdss_cfg *catalog) > > + void __iomem *addr, > > + const struct dpu_mdss_cfg *catalog, > > + const struct msm_mdss_data *mdss_data) > > { > > struct dpu_hw_sspp *hw_pipe; > > const struct dpu_sspp_cfg *cfg; > > @@ -813,7 +817,7 @@ struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx, > > > > /* Assign ops */ > > hw_pipe->catalog = catalog; > > - hw_pipe->ubwc = catalog->ubwc; > > + hw_pipe->ubwc = mdss_data; > > hw_pipe->idx = idx; > > hw_pipe->cap = cfg; > > _setup_layer_ops(hw_pipe, hw_pipe->cap->features); > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > > index 74b98b6b3bc3..8d4ae9d24674 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > > @@ -351,7 +351,7 @@ struct dpu_hw_sspp { > > struct dpu_hw_blk base; > > struct dpu_hw_blk_reg_map hw; > > const struct dpu_mdss_cfg *catalog; > > - const struct dpu_ubwc_cfg *ubwc; > > + const struct msm_mdss_data *ubwc; > > > > /* Pipe */ > > enum dpu_sspp idx; > > @@ -368,9 +368,12 @@ struct dpu_kms; > > * @idx: Pipe index for which driver object is required > > * @addr: Mapped register io address of MDP > > * @catalog : Pointer to mdss catalog data > > + * @mdss_data: UBWC / MDSS configuration data > > */ > > struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx, > > - void __iomem *addr, const struct dpu_mdss_cfg *catalog); > > + void __iomem *addr, > > + const struct dpu_mdss_cfg *catalog, > > + const struct msm_mdss_data *mdss_data); > > > > /** > > * dpu_hw_sspp_destroy(): Destroys SSPP driver context > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > index 0e7a68714e9e..a4293dc0b61b 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > @@ -22,6 +22,7 @@ > > > > #include "msm_drv.h" > > #include "msm_mmu.h" > > +#include "msm_mdss.h" > > #include "msm_gem.h" > > #include "disp/msm_disp_snapshot.h" > > > > @@ -1066,7 +1067,20 @@ static int dpu_kms_hw_init(struct msm_kms *kms) > > goto power_error; > > } > > > > - rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mmio); > > + dpu_kms->mdss = msm_mdss_get_mdss_data(dpu_kms->pdev->dev.parent); > > + if (IS_ERR(dpu_kms->mdss)) { > > + rc = PTR_ERR(dpu_kms->mdss); > > + DPU_ERROR("failed to get MDSS data: %d\n", rc); > > + goto power_error; > > + } > > + > > + if (!dpu_kms->mdss) { > > + rc = -EINVAL; > > + DPU_ERROR("NULL MDSS data\n"); > > + goto power_error; > > + } > > + > > + rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mdss, dpu_kms->mmio); > > if (rc) { > > DPU_ERROR("rm init failed: %d\n", rc); > > goto power_error; > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > index aca39a4689f4..797b4ff3e806 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > @@ -69,6 +69,7 @@ struct dpu_kms { > > struct msm_kms base; > > struct drm_device *dev; > > const struct dpu_mdss_cfg *catalog; > > + const struct msm_mdss_data *mdss; > > > > /* io/register spaces: */ > > void __iomem *mmio, *vbif[VBIF_MAX], *reg_dma; > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > > index f4dda88a73f7..9ee493465c4b 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > > @@ -100,6 +100,7 @@ int dpu_rm_destroy(struct dpu_rm *rm) > > > > int dpu_rm_init(struct dpu_rm *rm, > > const struct dpu_mdss_cfg *cat, > > + const struct msm_mdss_data *mdss_data, > > void __iomem *mmio) > > { > > int rc, i; > > @@ -268,7 +269,7 @@ int dpu_rm_init(struct dpu_rm *rm, > > continue; > > } > > > > - hw = dpu_hw_sspp_init(sspp->id, mmio, cat); > > + hw = dpu_hw_sspp_init(sspp->id, mmio, cat, mdss_data); > > if (IS_ERR(hw)) { > > rc = PTR_ERR(hw); > > DPU_ERROR("failed sspp object creation: err %d\n", rc); > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > > index d62c2edb2460..2b551566cbf4 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > > @@ -40,11 +40,13 @@ struct dpu_rm { > > * for all HW blocks. > > * @rm: DPU Resource Manager handle > > * @cat: Pointer to hardware catalog > > + * @mdss_data: Pointer to MDSS / UBWC configuration > > * @mmio: mapped register io address of MDP > > * @Return: 0 on Success otherwise -ERROR > > */ > > int dpu_rm_init(struct dpu_rm *rm, > > const struct dpu_mdss_cfg *cat, > > + const struct msm_mdss_data *mdss_data, > > void __iomem *mmio); > > > > /**
On 7/27/2023 1:39 AM, Dmitry Baryshkov wrote: > On Thu, 27 Jul 2023 at 02:20, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote: >>> Switch to using data from MDSS driver to program the SSPP fetch and UBWC >>> configuration. >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++------- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 7 +++++-- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 16 +++++++++++++++- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 + >>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 3 ++- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 ++ >>> 6 files changed, 36 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>> index cf70a9bd1034..bfd82c2921af 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>> @@ -8,6 +8,8 @@ >>> #include "dpu_hw_sspp.h" >>> #include "dpu_kms.h" >>> >>> +#include "msm_mdss.h" >>> + >>> #include <drm/drm_file.h> >>> >>> #define DPU_FETCH_CONFIG_RESET_VALUE 0x00000087 >>> @@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct dpu_sw_pipe *pipe, >>> DPU_REG_WRITE(c, SSPP_FETCH_CONFIG, >>> DPU_FETCH_CONFIG_RESET_VALUE | >>> ctx->ubwc->highest_bank_bit << 18); >> >> Does this needs to be protected with if ctx->ubwc check? > > Yes... And it should have been already. > >> >>> - switch (ctx->ubwc->ubwc_version) { >>> - case DPU_HW_UBWC_VER_10: >>> + switch (ctx->ubwc->ubwc_enc_version) { >>> + case UBWC_1_0: >> >> The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx. >> What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the >> catalog for the encoder version in the first place? Because looking at >> the registers UBWC_x_x is the correct value. >> >> If we cannot find the reason why, it should be noted in the commit text >> that the values we are using did change. > > Huh? This is just an enum. It isn't a part of uABI, nor it is written > to the hardware. > The reason is that, this switch case is moving from comparing one set of values to totally different ones. So atleast that should be noted. First thing that struck me was this point because the enums UBWC_x_x and DPU_HW_UBWC_VER_xx dont match. >> >>> fast_clear = fmt->alpha_enable ? BIT(31) : 0; >>> DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL, >>> fast_clear | (ctx->ubwc->ubwc_swizzle & 0x1) | >>> BIT(8) | >>> (ctx->ubwc->highest_bank_bit << 4)); >>> break; >>> - case DPU_HW_UBWC_VER_20: >>> + case UBWC_2_0: >>> fast_clear = fmt->alpha_enable ? BIT(31) : 0; >>> DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL, >>> fast_clear | (ctx->ubwc->ubwc_swizzle) | >>> (ctx->ubwc->highest_bank_bit << 4)); >>> break; >>> - case DPU_HW_UBWC_VER_30: >>> + case UBWC_3_0: >>> DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL, >>> BIT(30) | (ctx->ubwc->ubwc_swizzle) | >>> (ctx->ubwc->highest_bank_bit << 4)); >>> break; >>> - case DPU_HW_UBWC_VER_40: >>> + case UBWC_4_0: >>> DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL, >>> DPU_FORMAT_IS_YUV(fmt) ? 0 : BIT(30)); >>> break; >>> @@ -793,7 +795,9 @@ static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp, >>> } >>> >>> struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx, >>> - void __iomem *addr, const struct dpu_mdss_cfg *catalog) >>> + void __iomem *addr, >>> + const struct dpu_mdss_cfg *catalog, >>> + const struct msm_mdss_data *mdss_data) >>> { >>> struct dpu_hw_sspp *hw_pipe; >>> const struct dpu_sspp_cfg *cfg; >>> @@ -813,7 +817,7 @@ struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx, >>> >>> /* Assign ops */ >>> hw_pipe->catalog = catalog; >>> - hw_pipe->ubwc = catalog->ubwc; >>> + hw_pipe->ubwc = mdss_data; >>> hw_pipe->idx = idx; >>> hw_pipe->cap = cfg; >>> _setup_layer_ops(hw_pipe, hw_pipe->cap->features); >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h >>> index 74b98b6b3bc3..8d4ae9d24674 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h >>> @@ -351,7 +351,7 @@ struct dpu_hw_sspp { >>> struct dpu_hw_blk base; >>> struct dpu_hw_blk_reg_map hw; >>> const struct dpu_mdss_cfg *catalog; >>> - const struct dpu_ubwc_cfg *ubwc; >>> + const struct msm_mdss_data *ubwc; >>> >>> /* Pipe */ >>> enum dpu_sspp idx; >>> @@ -368,9 +368,12 @@ struct dpu_kms; >>> * @idx: Pipe index for which driver object is required >>> * @addr: Mapped register io address of MDP >>> * @catalog : Pointer to mdss catalog data >>> + * @mdss_data: UBWC / MDSS configuration data >>> */ >>> struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx, >>> - void __iomem *addr, const struct dpu_mdss_cfg *catalog); >>> + void __iomem *addr, >>> + const struct dpu_mdss_cfg *catalog, >>> + const struct msm_mdss_data *mdss_data); >>> >>> /** >>> * dpu_hw_sspp_destroy(): Destroys SSPP driver context >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>> index 0e7a68714e9e..a4293dc0b61b 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>> @@ -22,6 +22,7 @@ >>> >>> #include "msm_drv.h" >>> #include "msm_mmu.h" >>> +#include "msm_mdss.h" >>> #include "msm_gem.h" >>> #include "disp/msm_disp_snapshot.h" >>> >>> @@ -1066,7 +1067,20 @@ static int dpu_kms_hw_init(struct msm_kms *kms) >>> goto power_error; >>> } >>> >>> - rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mmio); >>> + dpu_kms->mdss = msm_mdss_get_mdss_data(dpu_kms->pdev->dev.parent); >>> + if (IS_ERR(dpu_kms->mdss)) { >>> + rc = PTR_ERR(dpu_kms->mdss); >>> + DPU_ERROR("failed to get MDSS data: %d\n", rc); >>> + goto power_error; >>> + } >>> + >>> + if (!dpu_kms->mdss) { >>> + rc = -EINVAL; >>> + DPU_ERROR("NULL MDSS data\n"); >>> + goto power_error; >>> + } >>> + >>> + rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mdss, dpu_kms->mmio); >>> if (rc) { >>> DPU_ERROR("rm init failed: %d\n", rc); >>> goto power_error; >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >>> index aca39a4689f4..797b4ff3e806 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >>> @@ -69,6 +69,7 @@ struct dpu_kms { >>> struct msm_kms base; >>> struct drm_device *dev; >>> const struct dpu_mdss_cfg *catalog; >>> + const struct msm_mdss_data *mdss; >>> >>> /* io/register spaces: */ >>> void __iomem *mmio, *vbif[VBIF_MAX], *reg_dma; >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> index f4dda88a73f7..9ee493465c4b 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> @@ -100,6 +100,7 @@ int dpu_rm_destroy(struct dpu_rm *rm) >>> >>> int dpu_rm_init(struct dpu_rm *rm, >>> const struct dpu_mdss_cfg *cat, >>> + const struct msm_mdss_data *mdss_data, >>> void __iomem *mmio) >>> { >>> int rc, i; >>> @@ -268,7 +269,7 @@ int dpu_rm_init(struct dpu_rm *rm, >>> continue; >>> } >>> >>> - hw = dpu_hw_sspp_init(sspp->id, mmio, cat); >>> + hw = dpu_hw_sspp_init(sspp->id, mmio, cat, mdss_data); >>> if (IS_ERR(hw)) { >>> rc = PTR_ERR(hw); >>> DPU_ERROR("failed sspp object creation: err %d\n", rc); >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h >>> index d62c2edb2460..2b551566cbf4 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h >>> @@ -40,11 +40,13 @@ struct dpu_rm { >>> * for all HW blocks. >>> * @rm: DPU Resource Manager handle >>> * @cat: Pointer to hardware catalog >>> + * @mdss_data: Pointer to MDSS / UBWC configuration >>> * @mmio: mapped register io address of MDP >>> * @Return: 0 on Success otherwise -ERROR >>> */ >>> int dpu_rm_init(struct dpu_rm *rm, >>> const struct dpu_mdss_cfg *cat, >>> + const struct msm_mdss_data *mdss_data, >>> void __iomem *mmio); >>> >>> /** > > >
On 7/27/2023 8:26 AM, Dmitry Baryshkov wrote: > On 27/07/2023 18:24, Abhinav Kumar wrote: >> >> >> On 7/27/2023 1:39 AM, Dmitry Baryshkov wrote: >>> On Thu, 27 Jul 2023 at 02:20, Abhinav Kumar >>> <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote: >>>>> Switch to using data from MDSS driver to program the SSPP fetch and >>>>> UBWC >>>>> configuration. >>>>> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>> --- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++------- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 7 +++++-- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 16 +++++++++++++++- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 + >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 3 ++- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 ++ >>>>> 6 files changed, 36 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>>>> index cf70a9bd1034..bfd82c2921af 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>>>> @@ -8,6 +8,8 @@ >>>>> #include "dpu_hw_sspp.h" >>>>> #include "dpu_kms.h" >>>>> >>>>> +#include "msm_mdss.h" >>>>> + >>>>> #include <drm/drm_file.h> >>>>> >>>>> #define DPU_FETCH_CONFIG_RESET_VALUE 0x00000087 >>>>> @@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct >>>>> dpu_sw_pipe *pipe, >>>>> DPU_REG_WRITE(c, SSPP_FETCH_CONFIG, >>>>> DPU_FETCH_CONFIG_RESET_VALUE | >>>>> ctx->ubwc->highest_bank_bit << 18); >>>> >>>> Does this needs to be protected with if ctx->ubwc check? >>> >>> Yes... And it should have been already. >>> >>>> >>>>> - switch (ctx->ubwc->ubwc_version) { >>>>> - case DPU_HW_UBWC_VER_10: >>>>> + switch (ctx->ubwc->ubwc_enc_version) { >>>>> + case UBWC_1_0: >>>> >>>> The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx. >>>> What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in >>>> the >>>> catalog for the encoder version in the first place? Because looking at >>>> the registers UBWC_x_x is the correct value. >>>> >>>> If we cannot find the reason why, it should be noted in the commit text >>>> that the values we are using did change. >>> >>> Huh? This is just an enum. It isn't a part of uABI, nor it is written >>> to the hardware. >>> >> >> The reason is that, this switch case is moving from comparing one set >> of values to totally different ones. So atleast that should be noted. >> >> First thing that struck me was this point because the enums UBWC_x_x >> and DPU_HW_UBWC_VER_xx dont match. >> > > Do you have any proposed text in mind? > I was doing some checking about this. The issue was that when this enum was made, it missed using the SDE_HW_UBWC_VER macro 75 enum { 76 DPU_HW_UBWC_VER_10 = 0x100, 77 DPU_HW_UBWC_VER_20 = 0x200, 78 DPU_HW_UBWC_VER_30 = 0x300, 79 DPU_HW_UBWC_VER_40 = 0x400, 80 }; 81 so something like this: 183 */ 184 enum { 185 SDE_HW_UBWC_VER_10 = SDE_HW_UBWC_VER(0x100), 186 SDE_HW_UBWC_VER_20 = SDE_HW_UBWC_VER(0x200), 187 SDE_HW_UBWC_VER_30 = SDE_HW_UBWC_VER(0x300), 188 SDE_HW_UBWC_VER_40 = SDE_HW_UBWC_VER(0x400), 189 SDE_HW_UBWC_VER_43 = SDE_HW_UBWC_VER(0x431), 190 }; This macro handles that conversion under the hood. So I would write something like this "This also corrects the usage of UBWC version which was incorrect from the beginning because of the enum storing the DPU_HW_UBWC_*** missed out the conversion to the full UBWC version"
On Fri, 28 Jul 2023 at 22:25, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 7/27/2023 8:26 AM, Dmitry Baryshkov wrote: > > On 27/07/2023 18:24, Abhinav Kumar wrote: > >> > >> > >> On 7/27/2023 1:39 AM, Dmitry Baryshkov wrote: > >>> On Thu, 27 Jul 2023 at 02:20, Abhinav Kumar > >>> <quic_abhinavk@quicinc.com> wrote: > >>>> > >>>> > >>>> > >>>> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote: > >>>>> Switch to using data from MDSS driver to program the SSPP fetch and > >>>>> UBWC > >>>>> configuration. > >>>>> > >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>>>> --- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++------- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 7 +++++-- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 16 +++++++++++++++- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 + > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 3 ++- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 ++ > >>>>> 6 files changed, 36 insertions(+), 11 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > >>>>> index cf70a9bd1034..bfd82c2921af 100644 > >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > >>>>> @@ -8,6 +8,8 @@ > >>>>> #include "dpu_hw_sspp.h" > >>>>> #include "dpu_kms.h" > >>>>> > >>>>> +#include "msm_mdss.h" > >>>>> + > >>>>> #include <drm/drm_file.h> > >>>>> > >>>>> #define DPU_FETCH_CONFIG_RESET_VALUE 0x00000087 > >>>>> @@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct > >>>>> dpu_sw_pipe *pipe, > >>>>> DPU_REG_WRITE(c, SSPP_FETCH_CONFIG, > >>>>> DPU_FETCH_CONFIG_RESET_VALUE | > >>>>> ctx->ubwc->highest_bank_bit << 18); > >>>> > >>>> Does this needs to be protected with if ctx->ubwc check? > >>> > >>> Yes... And it should have been already. > >>> > >>>> > >>>>> - switch (ctx->ubwc->ubwc_version) { > >>>>> - case DPU_HW_UBWC_VER_10: > >>>>> + switch (ctx->ubwc->ubwc_enc_version) { > >>>>> + case UBWC_1_0: > >>>> > >>>> The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx. > >>>> What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in > >>>> the > >>>> catalog for the encoder version in the first place? Because looking at > >>>> the registers UBWC_x_x is the correct value. > >>>> > >>>> If we cannot find the reason why, it should be noted in the commit text > >>>> that the values we are using did change. > >>> > >>> Huh? This is just an enum. It isn't a part of uABI, nor it is written > >>> to the hardware. > >>> > >> > >> The reason is that, this switch case is moving from comparing one set > >> of values to totally different ones. So atleast that should be noted. > >> > >> First thing that struck me was this point because the enums UBWC_x_x > >> and DPU_HW_UBWC_VER_xx dont match. > >> > > > > Do you have any proposed text in mind? > > > > I was doing some checking about this. The issue was that when this enum > was made, it missed using the SDE_HW_UBWC_VER macro > > > 75 enum { > 76 DPU_HW_UBWC_VER_10 = 0x100, > 77 DPU_HW_UBWC_VER_20 = 0x200, > 78 DPU_HW_UBWC_VER_30 = 0x300, > 79 DPU_HW_UBWC_VER_40 = 0x400, > 80 }; > 81 > > so something like this: > > 183 */ > 184 enum { > 185 SDE_HW_UBWC_VER_10 = SDE_HW_UBWC_VER(0x100), > 186 SDE_HW_UBWC_VER_20 = SDE_HW_UBWC_VER(0x200), > 187 SDE_HW_UBWC_VER_30 = SDE_HW_UBWC_VER(0x300), > 188 SDE_HW_UBWC_VER_40 = SDE_HW_UBWC_VER(0x400), > 189 SDE_HW_UBWC_VER_43 = SDE_HW_UBWC_VER(0x431), > 190 }; > > This macro handles that conversion under the hood. > > So I would write something like this > > "This also corrects the usage of UBWC version which was incorrect from > the beginning because of the enum storing the DPU_HW_UBWC_*** missed out > the conversion to the full UBWC version" I don't like the word 'correcting', as it makes one think there was an issue beforehand. There were no real issues. So I'll try to cook something up for the next iteration.