[PATCH v3 2/5] drm/msm: remove extra indirection for msm_mdss
Abhinav Kumar
quic_abhinavk at quicinc.com
Sat Mar 5 00:42:42 UTC 2022
On 3/3/2022 7:21 PM, Dmitry Baryshkov wrote:
> Since now there is just one mdss subdriver, drop all the indirection,
> make msm_mdss struct completely opaque (and defined inside msm_mdss.c)
> and call mdss functions directly.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
> ---
> drivers/gpu/drm/msm/msm_drv.c | 29 +++----
> drivers/gpu/drm/msm/msm_kms.h | 16 ++--
> drivers/gpu/drm/msm/msm_mdss.c | 136 +++++++++++++++------------------
> 3 files changed, 81 insertions(+), 100 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 078c7e951a6e..f3f33b8c6eba 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -948,8 +948,8 @@ static int __maybe_unused msm_runtime_suspend(struct device *dev)
>
> DBG("");
>
> - if (mdss && mdss->funcs)
> - return mdss->funcs->disable(mdss);
> + if (mdss)
> + return msm_mdss_disable(mdss);
>
> return 0;
> }
> @@ -961,8 +961,8 @@ static int __maybe_unused msm_runtime_resume(struct device *dev)
>
> DBG("");
>
> - if (mdss && mdss->funcs)
> - return mdss->funcs->enable(mdss);
> + if (mdss)
> + return msm_mdss_enable(mdss);
>
> return 0;
> }
> @@ -1197,6 +1197,7 @@ static const struct component_master_ops msm_drm_ops = {
> static int msm_pdev_probe(struct platform_device *pdev)
> {
> struct component_match *match = NULL;
> + struct msm_mdss *mdss;
> struct msm_drm_private *priv;
> int ret;
>
> @@ -1208,20 +1209,22 @@ static int msm_pdev_probe(struct platform_device *pdev)
>
> switch (get_mdp_ver(pdev)) {
> case KMS_MDP5:
> - ret = msm_mdss_init(pdev, true);
> + mdss = msm_mdss_init(pdev, true);
> break;
> case KMS_DPU:
> - ret = msm_mdss_init(pdev, false);
> + mdss = msm_mdss_init(pdev, false);
> break;
> default:
> - ret = 0;
> + mdss = NULL;
> break;
> }
> - if (ret) {
> - platform_set_drvdata(pdev, NULL);
> + if (IS_ERR(mdss)) {
> + ret = PTR_ERR(mdss);
> return ret;
> }
>
> + priv->mdss = mdss;
> +
> if (get_mdp_ver(pdev)) {
> ret = add_display_components(pdev, &match);
> if (ret)
> @@ -1248,8 +1251,8 @@ static int msm_pdev_probe(struct platform_device *pdev)
> fail:
> of_platform_depopulate(&pdev->dev);
>
> - if (priv->mdss && priv->mdss->funcs)
> - priv->mdss->funcs->destroy(priv->mdss);
> + if (priv->mdss)
> + msm_mdss_destroy(priv->mdss);
>
> return ret;
> }
> @@ -1262,8 +1265,8 @@ static int msm_pdev_remove(struct platform_device *pdev)
> component_master_del(&pdev->dev, &msm_drm_ops);
> of_platform_depopulate(&pdev->dev);
>
> - if (mdss && mdss->funcs)
> - mdss->funcs->destroy(mdss);
> + if (mdss)
> + msm_mdss_destroy(mdss);
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index 10d5ae3e76df..09c219988884 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -201,18 +201,12 @@ struct msm_kms *dpu_kms_init(struct drm_device *dev);
> extern const struct of_device_id dpu_dt_match[];
> extern const struct of_device_id mdp5_dt_match[];
>
> -struct msm_mdss_funcs {
> - int (*enable)(struct msm_mdss *mdss);
> - int (*disable)(struct msm_mdss *mdss);
> - void (*destroy)(struct msm_mdss *mdss);
> -};
> -
> -struct msm_mdss {
> - struct device *dev;
> - const struct msm_mdss_funcs *funcs;
> -};
> +struct msm_mdss;
>
> -int msm_mdss_init(struct platform_device *pdev, bool is_mdp5);
> +struct msm_mdss *msm_mdss_init(struct platform_device *pdev, bool is_mdp5);
> +int msm_mdss_enable(struct msm_mdss *mdss);
> +int msm_mdss_disable(struct msm_mdss *mdss);
> +void msm_mdss_destroy(struct msm_mdss *mdss);
>
> #define for_each_crtc_mask(dev, crtc, crtc_mask) \
> drm_for_each_crtc(crtc, dev) \
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index 71f3277bde32..857eefbb8649 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -3,19 +3,16 @@
> * Copyright (c) 2018, The Linux Foundation
> */
>
> +#include <linux/clk.h>
> #include <linux/irq.h>
> #include <linux/irqchip.h>
> #include <linux/irqdesc.h>
> #include <linux/irqchip/chained_irq.h>
> -
> -#include "msm_drv.h"
> -#include "msm_kms.h"
> +#include <linux/pm_runtime.h>
>
> /* for DPU_HW_* defines */
> #include "disp/dpu1/dpu_hw_catalog.h"
>
> -#define to_dpu_mdss(x) container_of(x, struct dpu_mdss, base)
> -
> #define HW_REV 0x0
> #define HW_INTR_STATUS 0x0010
>
> @@ -23,8 +20,9 @@
> #define UBWC_CTRL_2 0x150
> #define UBWC_PREDICTION_MODE 0x154
>
> -struct dpu_mdss {
> - struct msm_mdss base;
> +struct msm_mdss {
> + struct device *dev;
> +
> void __iomem *mmio;
> struct clk_bulk_data *clocks;
> size_t num_clocks;
> @@ -36,22 +34,22 @@ struct dpu_mdss {
>
> static void msm_mdss_irq(struct irq_desc *desc)
> {
> - struct dpu_mdss *dpu_mdss = irq_desc_get_handler_data(desc);
> + struct msm_mdss *msm_mdss = irq_desc_get_handler_data(desc);
> struct irq_chip *chip = irq_desc_get_chip(desc);
> u32 interrupts;
>
> chained_irq_enter(chip, desc);
>
> - interrupts = readl_relaxed(dpu_mdss->mmio + HW_INTR_STATUS);
> + interrupts = readl_relaxed(msm_mdss->mmio + HW_INTR_STATUS);
>
> while (interrupts) {
> irq_hw_number_t hwirq = fls(interrupts) - 1;
> int rc;
>
> - rc = generic_handle_domain_irq(dpu_mdss->irq_controller.domain,
> + rc = generic_handle_domain_irq(msm_mdss->irq_controller.domain,
> hwirq);
> if (rc < 0) {
> - DRM_ERROR("handle irq fail: irq=%lu rc=%d\n",
> + dev_err(msm_mdss->dev, "handle irq fail: irq=%lu rc=%d\n",
> hwirq, rc);
> break;
> }
> @@ -64,28 +62,28 @@ static void msm_mdss_irq(struct irq_desc *desc)
>
> static void msm_mdss_irq_mask(struct irq_data *irqd)
> {
> - struct dpu_mdss *dpu_mdss = irq_data_get_irq_chip_data(irqd);
> + struct msm_mdss *msm_mdss = irq_data_get_irq_chip_data(irqd);
>
> /* memory barrier */
> smp_mb__before_atomic();
> - clear_bit(irqd->hwirq, &dpu_mdss->irq_controller.enabled_mask);
> + clear_bit(irqd->hwirq, &msm_mdss->irq_controller.enabled_mask);
> /* memory barrier */
> smp_mb__after_atomic();
> }
>
> static void msm_mdss_irq_unmask(struct irq_data *irqd)
> {
> - struct dpu_mdss *dpu_mdss = irq_data_get_irq_chip_data(irqd);
> + struct msm_mdss *msm_mdss = irq_data_get_irq_chip_data(irqd);
>
> /* memory barrier */
> smp_mb__before_atomic();
> - set_bit(irqd->hwirq, &dpu_mdss->irq_controller.enabled_mask);
> + set_bit(irqd->hwirq, &msm_mdss->irq_controller.enabled_mask);
> /* memory barrier */
> smp_mb__after_atomic();
> }
>
> static struct irq_chip msm_mdss_irq_chip = {
> - .name = "dpu_mdss",
> + .name = "msm_mdss",
> .irq_mask = msm_mdss_irq_mask,
> .irq_unmask = msm_mdss_irq_unmask,
> };
> @@ -95,12 +93,12 @@ static struct lock_class_key msm_mdss_lock_key, msm_mdss_request_key;
> static int msm_mdss_irqdomain_map(struct irq_domain *domain,
> unsigned int irq, irq_hw_number_t hwirq)
> {
> - struct dpu_mdss *dpu_mdss = domain->host_data;
> + struct msm_mdss *msm_mdss = domain->host_data;
>
> irq_set_lockdep_class(irq, &msm_mdss_lock_key, &msm_mdss_request_key);
> irq_set_chip_and_handler(irq, &msm_mdss_irq_chip, handle_level_irq);
>
> - return irq_set_chip_data(irq, dpu_mdss);
> + return irq_set_chip_data(irq, msm_mdss);
> }
>
> static const struct irq_domain_ops msm_mdss_irqdomain_ops = {
> @@ -108,34 +106,33 @@ static const struct irq_domain_ops msm_mdss_irqdomain_ops = {
> .xlate = irq_domain_xlate_onecell,
> };
>
> -static int _msm_mdss_irq_domain_add(struct dpu_mdss *dpu_mdss)
> +static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> {
> struct device *dev;
> struct irq_domain *domain;
>
> - dev = dpu_mdss->base.dev;
> + dev = msm_mdss->dev;
>
> domain = irq_domain_add_linear(dev->of_node, 32,
> - &msm_mdss_irqdomain_ops, dpu_mdss);
> + &msm_mdss_irqdomain_ops, msm_mdss);
> if (!domain) {
> - DRM_ERROR("failed to add irq_domain\n");
> + dev_err(dev, "failed to add irq_domain\n");
> return -EINVAL;
> }
>
> - dpu_mdss->irq_controller.enabled_mask = 0;
> - dpu_mdss->irq_controller.domain = domain;
> + msm_mdss->irq_controller.enabled_mask = 0;
> + msm_mdss->irq_controller.domain = domain;
>
> return 0;
> }
>
> -static int msm_mdss_enable(struct msm_mdss *mdss)
> +int msm_mdss_enable(struct msm_mdss *msm_mdss)
> {
> - struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss);
> int ret;
>
> - ret = clk_bulk_prepare_enable(dpu_mdss->num_clocks, dpu_mdss->clocks);
> + ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
> if (ret) {
> - DRM_ERROR("clock enable failed, ret:%d\n", ret);
> + dev_err(msm_mdss->dev, "clock enable failed, ret:%d\n", ret);
> return ret;
> }
>
> @@ -143,57 +140,48 @@ static int msm_mdss_enable(struct msm_mdss *mdss)
> * ubwc config is part of the "mdss" region which is not accessible
> * from the rest of the driver. hardcode known configurations here
> */
> - switch (readl_relaxed(dpu_mdss->mmio + HW_REV)) {
> + switch (readl_relaxed(msm_mdss->mmio + HW_REV)) {
> case DPU_HW_VER_500:
> case DPU_HW_VER_501:
> - writel_relaxed(0x420, dpu_mdss->mmio + UBWC_STATIC);
> + writel_relaxed(0x420, msm_mdss->mmio + UBWC_STATIC);
> break;
> case DPU_HW_VER_600:
> /* TODO: 0x102e for LP_DDR4 */
> - writel_relaxed(0x103e, dpu_mdss->mmio + UBWC_STATIC);
> - writel_relaxed(2, dpu_mdss->mmio + UBWC_CTRL_2);
> - writel_relaxed(1, dpu_mdss->mmio + UBWC_PREDICTION_MODE);
> + writel_relaxed(0x103e, msm_mdss->mmio + UBWC_STATIC);
> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> break;
> case DPU_HW_VER_620:
> - writel_relaxed(0x1e, dpu_mdss->mmio + UBWC_STATIC);
> + writel_relaxed(0x1e, msm_mdss->mmio + UBWC_STATIC);
> break;
> case DPU_HW_VER_720:
> - writel_relaxed(0x101e, dpu_mdss->mmio + UBWC_STATIC);
> + writel_relaxed(0x101e, msm_mdss->mmio + UBWC_STATIC);
> break;
> }
>
> return ret;
> }
>
> -static int msm_mdss_disable(struct msm_mdss *mdss)
> +int msm_mdss_disable(struct msm_mdss *msm_mdss)
> {
> - struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss);
> -
> - clk_bulk_disable_unprepare(dpu_mdss->num_clocks, dpu_mdss->clocks);
> + clk_bulk_disable_unprepare(msm_mdss->num_clocks, msm_mdss->clocks);
>
> return 0;
> }
>
> -static void msm_mdss_destroy(struct msm_mdss *mdss)
> +void msm_mdss_destroy(struct msm_mdss *msm_mdss)
> {
> - struct platform_device *pdev = to_platform_device(mdss->dev);
> - struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss);
> + struct platform_device *pdev = to_platform_device(msm_mdss->dev);
> int irq;
>
> - pm_runtime_suspend(mdss->dev);
> - pm_runtime_disable(mdss->dev);
> - irq_domain_remove(dpu_mdss->irq_controller.domain);
> - dpu_mdss->irq_controller.domain = NULL;
> + pm_runtime_suspend(msm_mdss->dev);
> + pm_runtime_disable(msm_mdss->dev);
> + irq_domain_remove(msm_mdss->irq_controller.domain);
> + msm_mdss->irq_controller.domain = NULL;
> irq = platform_get_irq(pdev, 0);
> irq_set_chained_handler_and_data(irq, NULL, NULL);
> }
>
> -static const struct msm_mdss_funcs mdss_funcs = {
> - .enable = msm_mdss_enable,
> - .disable = msm_mdss_disable,
> - .destroy = msm_mdss_destroy,
> -};
> -
> /*
> * MDP5 MDSS uses at most three specified clocks.
> */
> @@ -224,50 +212,46 @@ static int mdp5_mdss_parse_clock(struct platform_device *pdev, struct clk_bulk_d
> return num_clocks;
> }
>
> -int msm_mdss_init(struct platform_device *pdev, bool is_mdp5)
> +struct msm_mdss *msm_mdss_init(struct platform_device *pdev, bool is_mdp5)
> {
> - struct msm_drm_private *priv = platform_get_drvdata(pdev);
> - struct dpu_mdss *dpu_mdss;
> + struct msm_mdss *msm_mdss;
> int ret;
> int irq;
>
> - dpu_mdss = devm_kzalloc(&pdev->dev, sizeof(*dpu_mdss), GFP_KERNEL);
> - if (!dpu_mdss)
> - return -ENOMEM;
> + msm_mdss = devm_kzalloc(&pdev->dev, sizeof(*msm_mdss), GFP_KERNEL);
> + if (!msm_mdss)
> + return ERR_PTR(-ENOMEM);
>
> - dpu_mdss->mmio = msm_ioremap(pdev, "mdss");
> - if (IS_ERR(dpu_mdss->mmio))
> - return PTR_ERR(dpu_mdss->mmio);
> + msm_mdss->mmio = devm_platform_ioremap_resource_byname(pdev, "mdss");
> + if (IS_ERR(msm_mdss->mmio))
> + return ERR_CAST(msm_mdss->mmio);
>
> - DRM_DEBUG("mapped mdss address space @%pK\n", dpu_mdss->mmio);
> + dev_dbg(&pdev->dev, "mapped mdss address space @%pK\n", msm_mdss->mmio);
>
> if (is_mdp5)
> - ret = mdp5_mdss_parse_clock(pdev, &dpu_mdss->clocks);
> + ret = mdp5_mdss_parse_clock(pdev, &msm_mdss->clocks);
> else
> - ret = devm_clk_bulk_get_all(&pdev->dev, &dpu_mdss->clocks);
> + ret = devm_clk_bulk_get_all(&pdev->dev, &msm_mdss->clocks);
> if (ret < 0) {
> - DRM_ERROR("failed to parse clocks, ret=%d\n", ret);
> - return ret;
> + dev_err(&pdev->dev, "failed to parse clocks, ret=%d\n", ret);
> + return ERR_PTR(ret);
> }
> - dpu_mdss->num_clocks = ret;
> + msm_mdss->num_clocks = ret;
>
> - dpu_mdss->base.dev = &pdev->dev;
> - dpu_mdss->base.funcs = &mdss_funcs;
> + msm_mdss->dev = &pdev->dev;
>
> irq = platform_get_irq(pdev, 0);
> if (irq < 0)
> - return irq;
> + return ERR_PTR(irq);
>
> - ret = _msm_mdss_irq_domain_add(dpu_mdss);
> + ret = _msm_mdss_irq_domain_add(msm_mdss);
> if (ret)
> - return ret;
> + return ERR_PTR(ret);
>
> irq_set_chained_handler_and_data(irq, msm_mdss_irq,
> - dpu_mdss);
> -
> - priv->mdss = &dpu_mdss->base;
> + msm_mdss);
>
> pm_runtime_enable(&pdev->dev);
>
> - return 0;
> + return msm_mdss;
> }
More information about the dri-devel
mailing list