[PATCH v2 1/3] drm/msm/mdss: convert UBWC setup to use match data
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Fri Jan 20 05:26:33 UTC 2023
On Fri, 20 Jan 2023 at 00:54, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>
>
>
> On 1/17/2023 5:04 PM, Dmitry Baryshkov wrote:
> > To simplify adding new platforms and to make settings more obvious,
> > rewrite the UBWC setup to use the data structure to pass platform config
> > rather than just calling the functions direcly.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>
> I was reviewing this series and
> https://patchwork.freedesktop.org/series/111732/ together.
>
> More I think about it, it seems like we are duplicating the same values
> here and in the catalog.
>
> Yes, these two are different drivers.
>
> But now that you are adding the UBWC entries here using the compatible
> string so you are creating something like a "catalog" here.
>
> In that case, why dont we remove the entries from dpu catalog and in the
> DPU driver get the parent's match data as we know that the msm_mdss is
> the parent of DPU driver
I should give it a thought, especially since we are trying to clean up
the DPU catalog.
>
> Let me know your thoughts.
>
> > ---
> > drivers/gpu/drm/msm/msm_mdss.c | 181 +++++++++++++++++++--------------
> > 1 file changed, 105 insertions(+), 76 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> > index 02646e4bb4cd..799672b88716 100644
> > --- a/drivers/gpu/drm/msm/msm_mdss.c
> > +++ b/drivers/gpu/drm/msm/msm_mdss.c
> > @@ -16,9 +16,6 @@
> > #include "msm_drv.h"
> > #include "msm_kms.h"
> >
> > -/* for DPU_HW_* defines */
> > -#include "disp/dpu1/dpu_hw_catalog.h"
> > -
> > #define HW_REV 0x0
> > #define HW_INTR_STATUS 0x0010
> >
> > @@ -29,6 +26,16 @@
> >
> > #define MIN_IB_BW 400000000UL /* Min ib vote 400MB */
> >
> > +struct msm_mdss_data {
> > + u32 ubwc_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;
> >
> > @@ -40,6 +47,7 @@ struct msm_mdss {
> > unsigned long enabled_mask;
> > struct irq_domain *domain;
> > } irq_controller;
> > + const struct msm_mdss_data *mdss_data;
> > struct icc_path *path[2];
> > u32 num_paths;
> > };
> > @@ -182,46 +190,40 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> > #define UBWC_3_0 0x30000000
> > #define UBWC_4_0 0x40000000
> >
> > -static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> > - u32 ubwc_static)
> > +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
> > {
> > - writel_relaxed(ubwc_static, msm_mdss->mmio + UBWC_STATIC);
> > + const struct msm_mdss_data *data = msm_mdss->mdss_data;
> > +
> > + writel_relaxed(data->ubwc_static, msm_mdss->mmio + UBWC_STATIC);
> > }
> >
> > -static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss,
> > - unsigned int ubwc_version,
> > - u32 ubwc_swizzle,
> > - u32 highest_bank_bit,
> > - u32 macrotile_mode)
> > +static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
> > {
> > - u32 value = (ubwc_swizzle & 0x1) |
> > - (highest_bank_bit & 0x3) << 4 |
> > - (macrotile_mode & 0x1) << 12;
> > + const struct msm_mdss_data *data = msm_mdss->mdss_data;
> > + u32 value = (data->ubwc_swizzle & 0x1) |
> > + (data->highest_bank_bit & 0x3) << 4 |
> > + (data->macrotile_mode & 0x1) << 12;
> >
> > - if (ubwc_version == UBWC_3_0)
> > + if (data->ubwc_version == UBWC_3_0)
> > value |= BIT(10);
> >
> > - if (ubwc_version == UBWC_1_0)
> > + if (data->ubwc_version == UBWC_1_0)
> > value |= BIT(8);
> >
> > writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> > }
> >
> > -static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss,
> > - unsigned int ubwc_version,
> > - u32 ubwc_swizzle,
> > - u32 ubwc_static,
> > - u32 highest_bank_bit,
> > - u32 macrotile_mode)
> > +static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
> > {
> > - u32 value = (ubwc_swizzle & 0x7) |
> > - (ubwc_static & 0x1) << 3 |
> > - (highest_bank_bit & 0x7) << 4 |
> > - (macrotile_mode & 0x1) << 12;
> > + const struct msm_mdss_data *data = msm_mdss->mdss_data;
> > + u32 value = (data->ubwc_swizzle & 0x7) |
> > + (data->ubwc_static & 0x1) << 3 |
> > + (data->highest_bank_bit & 0x7) << 4 |
> > + (data->macrotile_mode & 0x1) << 12;
> >
> > writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >
> > - if (ubwc_version == UBWC_3_0) {
> > + if (data->ubwc_version == UBWC_3_0) {
> > writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> > writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> > } else {
> > @@ -233,7 +235,6 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss,
> > static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> > {
> > int ret;
> > - u32 hw_rev;
> >
> > /*
> > * Several components have AXI clocks that can only be turned on if
> > @@ -249,57 +250,36 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> > }
> >
> > /*
> > - * HW_REV requires MDSS_MDP_CLK, which is not enabled by the mdss on
> > - * mdp5 hardware. Skip reading it for now.
> > + * Register access requires MDSS_MDP_CLK, which is not enabled by the
> > + * mdss on mdp5 hardware. Skip it for now.
> > */
> > - if (msm_mdss->is_mdp5)
> > + if (msm_mdss->is_mdp5 || !msm_mdss->mdss_data)
> > return 0;
> >
> > - hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
> > - dev_dbg(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
> > - dev_dbg(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
> > - readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
> > -
> > /*
> > * ubwc config is part of the "mdss" region which is not accessible
> > * from the rest of the driver. hardcode known configurations here
> > *
> > * Decoder version can be read from the UBWC_DEC_HW_VERSION reg,
> > - * UBWC_n and the rest of params comes from hw_catalog.
> > - * Unforunately this driver can not access hw catalog, so we have to
> > - * hardcode them here.
> > + * UBWC_n and the rest of params comes from hw data.
> > */
> > - switch (hw_rev) {
> > - case DPU_HW_VER_500:
> > - case DPU_HW_VER_501:
> > - msm_mdss_setup_ubwc_dec_30(msm_mdss, UBWC_3_0, 0, 2, 0);
> > - break;
> > - case DPU_HW_VER_600:
> > - /* TODO: highest_bank_bit = 2 for LP_DDR4 */
> > - msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
> > + switch (msm_mdss->mdss_data->ubwc_dec_version) {
> > + case UBWC_2_0:
> > + msm_mdss_setup_ubwc_dec_20(msm_mdss);
> > break;
> > - case DPU_HW_VER_620:
> > - /* UBWC_2_0 */
> > - msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x1e);
> > + case UBWC_3_0:
> > + msm_mdss_setup_ubwc_dec_30(msm_mdss);
> > break;
> > - case DPU_HW_VER_630:
> > - /* UBWC_2_0 */
> > - msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x11f);
> > + case UBWC_4_0:
> > + msm_mdss_setup_ubwc_dec_40(msm_mdss);
> > break;
> > - case DPU_HW_VER_700:
> > - /* TODO: highest_bank_bit = 2 for LP_DDR4 */
> > - msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
> > - break;
> > - case DPU_HW_VER_720:
> > - msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_3_0, 6, 1, 1, 1);
> > - break;
> > - case DPU_HW_VER_800:
> > - msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 2, 1);
> > - break;
> > - case DPU_HW_VER_810:
> > - case DPU_HW_VER_900:
> > - /* TODO: highest_bank_bit = 2 for LP_DDR4 */
> > - msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
> > + default:
> > + dev_err(msm_mdss->dev, "Unuspported UBWC decoder version %x\n",
> > + msm_mdss->mdss_data->ubwc_dec_version);
> > + dev_err(msm_mdss->dev, "HW_REV: 0x%x\n",
> > + readl_relaxed(msm_mdss->mmio + HW_REV));
> > + dev_err(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
> > + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
>
> Why do you still have these register reads in default?
> If the purpose was to catch any missed chipsets, that would not be
> possible right? Because that means the compat table entry is missing for
> this in that case the msm_mdss driver wont probe.
Compat entry can be missing, but the data might be null or lame.
Granted that the ubwc_dec_hw_version is not a part of the downstream
dtsi, I'd prefer to have a way to read it while porting.
>
>
> > break;
> > }
> >
> > @@ -490,6 +470,8 @@ static int mdss_probe(struct platform_device *pdev)
> > if (IS_ERR(mdss))
> > return PTR_ERR(mdss);
> >
> > + mdss->mdss_data = of_device_get_match_data(&pdev->dev);
> > +
> > platform_set_drvdata(pdev, mdss);
> >
> > /*
> > @@ -519,21 +501,68 @@ static int mdss_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > +static const struct msm_mdss_data sc7180_data = {
> > + .ubwc_version = UBWC_2_0,
> > + .ubwc_dec_version = UBWC_2_0,
> > + .ubwc_static = 0x1e,
> > +};
> > +
> > +static const struct msm_mdss_data sc7280_data = {
> > + .ubwc_version = UBWC_3_0,
> > + .ubwc_dec_version = UBWC_4_0,
> > + .ubwc_swizzle = 6,
> > + .ubwc_static = 1,
> > + .highest_bank_bit = 1,
> > + .macrotile_mode = 1,
> > +};
> > +
> > +static const struct msm_mdss_data sc8280xp_data = {
> > + .ubwc_version = UBWC_4_0,
> > + .ubwc_dec_version = UBWC_4_0,
> > + .ubwc_swizzle = 6,
> > + .ubwc_static = 1,
> > + .highest_bank_bit = 2,
> > + .macrotile_mode = 1,
> > +};
> > +
> > +static const struct msm_mdss_data sm8150_data = {
> > + .ubwc_version = UBWC_3_0,
> > + .ubwc_dec_version = UBWC_3_0,
> > + .highest_bank_bit = 2,
> > +};
> > +
> > +static const struct msm_mdss_data sm6115_data = {
> > + .ubwc_version = UBWC_1_0,
> > + .ubwc_dec_version = UBWC_2_0,
> > + .ubwc_swizzle = 7,
> > + .ubwc_static = 0x11f,
> > +};
> > +
> > +static const struct msm_mdss_data sm8250_data = {
> > + .ubwc_version = UBWC_4_0,
> > + .ubwc_dec_version = UBWC_3_0,
> > + .ubwc_swizzle = 6,
> > + .ubwc_static = 1,
> > + /* TODO: highest_bank_bit = 2 for LP_DDR4 */
> > + .highest_bank_bit = 3,
> > + .macrotile_mode = 1,
> > +};
> > +
> > static const struct of_device_id mdss_dt_match[] = {
> > { .compatible = "qcom,mdss" },
> > { .compatible = "qcom,msm8998-mdss" },
> > { .compatible = "qcom,qcm2290-mdss" },
> > { .compatible = "qcom,sdm845-mdss" },
> > - { .compatible = "qcom,sc7180-mdss" },
> > - { .compatible = "qcom,sc7280-mdss" },
> > + { .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
> > + { .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
> > { .compatible = "qcom,sc8180x-mdss" },
> > - { .compatible = "qcom,sc8280xp-mdss" },
> > - { .compatible = "qcom,sm6115-mdss" },
> > - { .compatible = "qcom,sm8150-mdss" },
> > - { .compatible = "qcom,sm8250-mdss" },
> > - { .compatible = "qcom,sm8350-mdss" },
> > - { .compatible = "qcom,sm8450-mdss" },
> > - { .compatible = "qcom,sm8550-mdss" },
> > + { .compatible = "qcom,sc8280xp-mdss", .data = &sc8280xp_data },
> > + { .compatible = "qcom,sm6115-mdss", .data = &sm6115_data },
> > + { .compatible = "qcom,sm8150-mdss", .data = &sm8150_data },
> > + { .compatible = "qcom,sm8250-mdss", .data = &sm8250_data },
> > + { .compatible = "qcom,sm8350-mdss", .data = &sm8250_data },
> > + { .compatible = "qcom,sm8450-mdss", .data = &sm8250_data },
> > + { .compatible = "qcom,sm8550-mdss", .data = &sm8250_data },
> > {}
> > };
> > MODULE_DEVICE_TABLE(of, mdss_dt_match);
--
With best wishes
Dmitry
More information about the dri-devel
mailing list