Use drmm_mode_config_init() and drop the explicit calls to drm_mode_config_cleanup().
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de --- drivers/gpu/drm/imx/imx-drm-core.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index 3421043a558d..d10887b9b2e4 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -20,6 +20,7 @@ #include <drm/drm_fb_helper.h> #include <drm/drm_gem_cma_helper.h> #include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_of.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> @@ -222,7 +223,9 @@ static int imx_drm_bind(struct device *dev) drm->mode_config.allow_fb_modifiers = true; drm->mode_config.normalize_zpos = true;
- drm_mode_config_init(drm); + ret = drmm_mode_config_init(drm); + if (ret) + return ret;
ret = drm_vblank_init(drm, MAX_CRTC); if (ret) @@ -261,7 +264,6 @@ static int imx_drm_bind(struct device *dev) drm_kms_helper_poll_fini(drm); component_unbind_all(drm->dev, drm); err_kms: - drm_mode_config_cleanup(drm); drm_dev_put(drm);
return ret; @@ -277,11 +279,9 @@ static void imx_drm_unbind(struct device *dev)
component_unbind_all(drm->dev, drm);
- drm_mode_config_cleanup(drm); + drm_dev_put(drm);
dev_set_drvdata(dev, NULL); - - drm_dev_put(drm); }
static const struct component_master_ops imx_drm_ops = {
Move bridge creation into probe, during bind only create the encoder and attach the bridge. Use drmm_kzalloc() to align encoder memory lifetime with the drm device, and use drmm_add_action_or_reset() to make sure drm_encoder_cleanup() is called before the memory is freed.
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de --- drivers/gpu/drm/imx/dw_hdmi-imx.c | 108 ++++++++++++++---------------- 1 file changed, 51 insertions(+), 57 deletions(-)
diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c index a4f178c1d9bc..b5106792725f 100644 --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c @@ -15,23 +15,32 @@
#include <drm/bridge/dw_hdmi.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_bridge.h> #include <drm/drm_edid.h> #include <drm/drm_encoder.h> +#include <drm/drm_managed.h> #include <drm/drm_of.h> #include <drm/drm_simple_kms_helper.h>
#include "imx-drm.h"
+struct imx_hdmi; + +struct imx_hdmi_encoder { + struct drm_encoder encoder; + struct imx_hdmi *hdmi; +}; + struct imx_hdmi { struct device *dev; - struct drm_encoder encoder; + struct drm_bridge *bridge; struct dw_hdmi *hdmi; struct regmap *regmap; };
static inline struct imx_hdmi *enc_to_imx_hdmi(struct drm_encoder *e) { - return container_of(e, struct imx_hdmi, encoder); + return container_of(e, struct imx_hdmi_encoder, encoder)->hdmi; }
static const struct dw_hdmi_mpll_config imx_mpll_cfg[] = { @@ -98,23 +107,6 @@ static const struct dw_hdmi_phy_config imx_phy_config[] = { { ~0UL, 0x0000, 0x0000, 0x0000} };
-static int dw_hdmi_imx_parse_dt(struct imx_hdmi *hdmi) -{ - struct device_node *np = hdmi->dev->of_node; - - hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "gpr"); - if (IS_ERR(hdmi->regmap)) { - dev_err(hdmi->dev, "Unable to get gpr\n"); - return PTR_ERR(hdmi->regmap); - } - - return 0; -} - -static void dw_hdmi_imx_encoder_disable(struct drm_encoder *encoder) -{ -} - static void dw_hdmi_imx_encoder_enable(struct drm_encoder *encoder) { struct imx_hdmi *hdmi = enc_to_imx_hdmi(encoder); @@ -140,7 +132,6 @@ static int dw_hdmi_imx_atomic_check(struct drm_encoder *encoder,
static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs = { .enable = dw_hdmi_imx_encoder_enable, - .disable = dw_hdmi_imx_encoder_disable, .atomic_check = dw_hdmi_imx_atomic_check, };
@@ -195,68 +186,51 @@ static const struct of_device_id dw_hdmi_imx_dt_ids[] = { }; MODULE_DEVICE_TABLE(of, dw_hdmi_imx_dt_ids);
+static void dw_hdmi_imx_encoder_cleanup(struct drm_device *drm, void *data) +{ + struct drm_encoder *encoder = data; + + drm_encoder_cleanup(encoder); +} + static int dw_hdmi_imx_bind(struct device *dev, struct device *master, void *data) { - struct platform_device *pdev = to_platform_device(dev); - const struct dw_hdmi_plat_data *plat_data; - const struct of_device_id *match; struct drm_device *drm = data; struct drm_encoder *encoder; - struct imx_hdmi *hdmi; + struct imx_hdmi_encoder *hdmi_encoder; + struct imx_hdmi *hdmi = dev_get_drvdata(dev); int ret;
- if (!pdev->dev.of_node) - return -ENODEV; - - hdmi = dev_get_drvdata(dev); - memset(hdmi, 0, sizeof(*hdmi)); + hdmi_encoder = drmm_kzalloc(drm, sizeof(*hdmi_encoder), GFP_KERNEL); + if (!hdmi_encoder) + return -ENOMEM;
- match = of_match_node(dw_hdmi_imx_dt_ids, pdev->dev.of_node); - plat_data = match->data; - hdmi->dev = &pdev->dev; - encoder = &hdmi->encoder; + hdmi_encoder->hdmi = hdmi; + encoder = &hdmi_encoder->encoder;
ret = imx_drm_encoder_parse_of(drm, encoder, dev->of_node); if (ret) return ret;
- ret = dw_hdmi_imx_parse_dt(hdmi); - if (ret < 0) - return ret; - drm_encoder_helper_add(encoder, &dw_hdmi_imx_encoder_helper_funcs); drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
- hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data); - - /* - * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(), - * which would have called the encoder cleanup. Do it manually. - */ - if (IS_ERR(hdmi->hdmi)) { - ret = PTR_ERR(hdmi->hdmi); - drm_encoder_cleanup(encoder); - } - - return ret; -} - -static void dw_hdmi_imx_unbind(struct device *dev, struct device *master, - void *data) -{ - struct imx_hdmi *hdmi = dev_get_drvdata(dev); + ret = drmm_add_action_or_reset(drm, dw_hdmi_imx_encoder_cleanup, encoder); + if (ret) + return ret;
- dw_hdmi_unbind(hdmi->hdmi); + return drm_bridge_attach(encoder, hdmi->bridge, NULL, 0); }
static const struct component_ops dw_hdmi_imx_ops = { .bind = dw_hdmi_imx_bind, - .unbind = dw_hdmi_imx_unbind, };
static int dw_hdmi_imx_probe(struct platform_device *pdev) { + struct device_node *np = pdev->dev.of_node; + const struct of_device_id *match = of_match_node(dw_hdmi_imx_dt_ids, np); struct imx_hdmi *hdmi;
hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL); @@ -264,13 +238,33 @@ static int dw_hdmi_imx_probe(struct platform_device *pdev) return -ENOMEM;
platform_set_drvdata(pdev, hdmi); + hdmi->dev = &pdev->dev; + + hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "gpr"); + if (IS_ERR(hdmi->regmap)) { + dev_err(hdmi->dev, "Unable to get gpr\n"); + return PTR_ERR(hdmi->regmap); + } + + hdmi->hdmi = dw_hdmi_probe(pdev, match->data); + if (IS_ERR(hdmi->hdmi)) + return PTR_ERR(hdmi->hdmi); + + hdmi->bridge = of_drm_find_bridge(np); + if (!hdmi->bridge) { + dev_err(hdmi->dev, "Unable to find bridge\n"); + return -ENODEV; + }
return component_add(&pdev->dev, &dw_hdmi_imx_ops); }
static int dw_hdmi_imx_remove(struct platform_device *pdev) { + struct imx_hdmi *hdmi = platform_get_drvdata(pdev); + component_del(&pdev->dev, &dw_hdmi_imx_ops); + dw_hdmi_remove(hdmi->hdmi);
return 0; }
Use drmm_kzalloc() to align encoder memory lifetime with the drm device, and use drmm_add_action_or_reset() to make sure drm_encoder_cleanup() is called before the memory is freed.
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de --- drivers/gpu/drm/imx/imx-ldb.c | 67 +++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c index af757d1e21fe..ef26a2960db9 100644 --- a/drivers/gpu/drm/imx/imx-ldb.c +++ b/drivers/gpu/drm/imx/imx-ldb.c @@ -22,6 +22,7 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> #include <drm/drm_fb_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_of.h> #include <drm/drm_panel.h> #include <drm/drm_print.h> @@ -62,7 +63,6 @@ struct imx_ldb_channel { struct i2c_adapter *ddc; int chno; void *edid; - int edid_len; struct drm_display_mode mode; int mode_valid; u32 bus_format; @@ -408,6 +408,13 @@ static int imx_ldb_get_clk(struct imx_ldb *ldb, int chno) return PTR_ERR_OR_ZERO(ldb->clk_pll[chno]); }
+static void imx_ldb_encoder_cleanup(struct drm_device *drm, void *data) +{ + struct drm_encoder *encoder = data; + + drm_encoder_cleanup(encoder); +} + static int imx_ldb_register(struct drm_device *drm, struct imx_ldb_channel *imx_ldb_ch) { @@ -432,6 +439,10 @@ static int imx_ldb_register(struct drm_device *drm, drm_encoder_helper_add(encoder, &imx_ldb_encoder_helper_funcs); drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_LVDS);
+ ret = drmm_add_action_or_reset(drm, imx_ldb_encoder_cleanup, encoder); + if (ret) + return ret; + if (imx_ldb_ch->bridge) { ret = drm_bridge_attach(&imx_ldb_ch->encoder, imx_ldb_ch->bridge, NULL, 0); @@ -536,15 +547,14 @@ static int imx_ldb_panel_ddc(struct device *dev, }
if (!channel->ddc) { + int edid_len; + /* if no DDC available, fallback to hardcoded EDID */ dev_dbg(dev, "no ddc available\n");
- edidp = of_get_property(child, "edid", - &channel->edid_len); + edidp = of_get_property(child, "edid", &edid_len); if (edidp) { - channel->edid = kmemdup(edidp, - channel->edid_len, - GFP_KERNEL); + channel->edid = kmemdup(edidp, edid_len, GFP_KERNEL); } else if (!channel->panel) { /* fallback to display-timings node */ ret = of_get_drm_display_mode(child, @@ -558,6 +568,19 @@ static int imx_ldb_panel_ddc(struct device *dev, return 0; }
+static void imx_ldb_cleanup(struct drm_device *drm, void *data) +{ + struct imx_ldb *imx_ldb = data; + int i; + + for (i = 0; i < 2; i++) { + struct imx_ldb_channel *channel = &imx_ldb->channel[i]; + + kfree(channel->edid); + i2c_put_adapter(channel->ddc); + } +} + static int imx_ldb_bind(struct device *dev, struct device *master, void *data) { struct drm_device *drm = data; @@ -570,8 +593,13 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data) int ret; int i;
- imx_ldb = dev_get_drvdata(dev); - memset(imx_ldb, 0, sizeof(*imx_ldb)); + imx_ldb = drmm_kzalloc(drm, sizeof(*imx_ldb), GFP_KERNEL); + if (!imx_ldb) + return -ENOMEM; + + ret = drmm_add_action_or_reset(drm, imx_ldb_cleanup, imx_ldb); + if (ret) + return ret;
imx_ldb->regmap = syscon_regmap_lookup_by_phandle(np, "gpr"); if (IS_ERR(imx_ldb->regmap)) { @@ -686,35 +714,12 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data) return ret; }
-static void imx_ldb_unbind(struct device *dev, struct device *master, - void *data) -{ - struct imx_ldb *imx_ldb = dev_get_drvdata(dev); - int i; - - for (i = 0; i < 2; i++) { - struct imx_ldb_channel *channel = &imx_ldb->channel[i]; - - kfree(channel->edid); - i2c_put_adapter(channel->ddc); - } -} - static const struct component_ops imx_ldb_ops = { .bind = imx_ldb_bind, - .unbind = imx_ldb_unbind, };
static int imx_ldb_probe(struct platform_device *pdev) { - struct imx_ldb *imx_ldb; - - imx_ldb = devm_kzalloc(&pdev->dev, sizeof(*imx_ldb), GFP_KERNEL); - if (!imx_ldb) - return -ENOMEM; - - platform_set_drvdata(pdev, imx_ldb); - return component_add(&pdev->dev, &imx_ldb_ops); }
Move devres regmap, clock, and interrupt requests into probe. Use drmm_kzalloc() to align encoder memory lifetime with the drm device, and use drmm_add_action_or_reset() to make sure drm_encoder_cleanup() is called before the memory is freed.
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de --- drivers/gpu/drm/imx/imx-tve.c | 95 +++++++++++++++++++++-------------- 1 file changed, 57 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c index ef3c25d87d87..257a06f6e408 100644 --- a/drivers/gpu/drm/imx/imx-tve.c +++ b/drivers/gpu/drm/imx/imx-tve.c @@ -19,6 +19,7 @@
#include <drm/drm_atomic_helper.h> #include <drm/drm_fb_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h>
@@ -99,9 +100,13 @@ enum { TVE_MODE_VGA, };
-struct imx_tve { +struct imx_tve_encoder { struct drm_connector connector; struct drm_encoder encoder; + struct imx_tve *tve; +}; + +struct imx_tve { struct device *dev; int mode; int di_hsync_pin; @@ -118,12 +123,12 @@ struct imx_tve {
static inline struct imx_tve *con_to_tve(struct drm_connector *c) { - return container_of(c, struct imx_tve, connector); + return container_of(c, struct imx_tve_encoder, connector)->tve; }
static inline struct imx_tve *enc_to_tve(struct drm_encoder *e) { - return container_of(e, struct imx_tve, encoder); + return container_of(e, struct imx_tve_encoder, encoder)->tve; }
static void tve_enable(struct imx_tve *tve) @@ -418,7 +423,7 @@ static int tve_clk_init(struct imx_tve *tve, void __iomem *base) init.parent_names = (const char **)&tve_di_parent;
tve->clk_hw_di.init = &init; - tve->di_clk = clk_register(tve->dev, &tve->clk_hw_di); + tve->di_clk = devm_clk_register(tve->dev, &tve->clk_hw_di); if (IS_ERR(tve->di_clk)) { dev_err(tve->dev, "failed to register TVE output clock: %ld\n", PTR_ERR(tve->di_clk)); @@ -428,31 +433,45 @@ static int tve_clk_init(struct imx_tve *tve, void __iomem *base) return 0; }
-static int imx_tve_register(struct drm_device *drm, struct imx_tve *tve) +static void imx_tve_encoder_cleanup(struct drm_device *drm, void *ptr) +{ + struct drm_encoder *encoder = ptr; + + drm_encoder_cleanup(encoder); +} + +static int imx_tve_register(struct drm_device *drm, struct imx_tve_encoder *tvee) { + struct imx_tve *tve = tvee->tve; + struct drm_encoder *encoder = &tvee->encoder; + struct drm_connector *connector = &tvee->connector; int encoder_type; int ret;
encoder_type = tve->mode == TVE_MODE_VGA ? DRM_MODE_ENCODER_DAC : DRM_MODE_ENCODER_TVDAC;
- ret = imx_drm_encoder_parse_of(drm, &tve->encoder, tve->dev->of_node); + ret = imx_drm_encoder_parse_of(drm, encoder, tve->dev->of_node); if (ret) return ret;
- drm_encoder_helper_add(&tve->encoder, &imx_tve_encoder_helper_funcs); - drm_simple_encoder_init(drm, &tve->encoder, encoder_type); + drm_encoder_helper_add(encoder, &imx_tve_encoder_helper_funcs); + ret = drm_simple_encoder_init(drm, encoder, encoder_type); + if (ret) + return ret;
- drm_connector_helper_add(&tve->connector, - &imx_tve_connector_helper_funcs); - drm_connector_init_with_ddc(drm, &tve->connector, - &imx_tve_connector_funcs, - DRM_MODE_CONNECTOR_VGA, - tve->ddc); + ret = drmm_add_action_or_reset(drm, imx_tve_encoder_cleanup, encoder); + if (ret) + return ret;
- drm_connector_attach_encoder(&tve->connector, &tve->encoder); + drm_connector_helper_add(connector, &imx_tve_connector_helper_funcs); + ret = drm_connector_init_with_ddc(drm, connector, + &imx_tve_connector_funcs, + DRM_MODE_CONNECTOR_VGA, tve->ddc); + if (ret) + return ret;
- return 0; + return drm_connector_attach_encoder(connector, encoder); }
static void imx_tve_disable_regulator(void *data) @@ -502,8 +521,26 @@ static const int of_get_tve_mode(struct device_node *np)
static int imx_tve_bind(struct device *dev, struct device *master, void *data) { - struct platform_device *pdev = to_platform_device(dev); struct drm_device *drm = data; + struct imx_tve *tve = dev_get_drvdata(dev); + struct imx_tve_encoder *tvee; + + tvee = drmm_kzalloc(drm, sizeof(*tvee), GFP_KERNEL); + if (!tvee) + return -ENOMEM; + + tvee->tve = tve; + + return imx_tve_register(drm, tvee); +} + +static const struct component_ops imx_tve_ops = { + .bind = imx_tve_bind, +}; + +static int imx_tve_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; struct device_node *ddc_node; struct imx_tve *tve; @@ -513,8 +550,9 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data) int irq; int ret;
- tve = dev_get_drvdata(dev); - memset(tve, 0, sizeof(*tve)); + tve = devm_kzalloc(dev, sizeof(*tve), GFP_KERNEL); + if (!tve) + return -ENOMEM;
tve->dev = dev;
@@ -621,25 +659,6 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data) if (ret) return ret;
- ret = imx_tve_register(drm, tve); - if (ret) - return ret; - - return 0; -} - -static const struct component_ops imx_tve_ops = { - .bind = imx_tve_bind, -}; - -static int imx_tve_probe(struct platform_device *pdev) -{ - struct imx_tve *tve; - - tve = devm_kzalloc(&pdev->dev, sizeof(*tve), GFP_KERNEL); - if (!tve) - return -ENOMEM; - platform_set_drvdata(pdev, tve);
return component_add(&pdev->dev, &imx_tve_ops);
Use drmm_kzalloc() to align encoder memory lifetime with the drm device, and use drmm_add_action_or_reset() to make sure drm_encoder_cleanup() is called before the memory is freed.
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de --- drivers/gpu/drm/imx/parallel-display.c | 50 +++++++++++++------------- 1 file changed, 26 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c index 8232f512b9ed..182d88d7f666 100644 --- a/drivers/gpu/drm/imx/parallel-display.c +++ b/drivers/gpu/drm/imx/parallel-display.c @@ -15,6 +15,7 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> #include <drm/drm_fb_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_of.h> #include <drm/drm_panel.h> #include <drm/drm_probe_helper.h> @@ -28,7 +29,6 @@ struct imx_parallel_display { struct drm_bridge bridge; struct device *dev; void *edid; - int edid_len; u32 bus_format; u32 bus_flags; struct drm_display_mode mode; @@ -259,6 +259,13 @@ static const struct drm_bridge_funcs imx_pd_bridge_funcs = { .atomic_get_output_bus_fmts = imx_pd_bridge_atomic_get_output_bus_fmts, };
+static void imx_pd_encoder_cleanup(struct drm_device *drm, void *ptr) +{ + struct drm_encoder *encoder = ptr; + + drm_encoder_cleanup(encoder); +} + static int imx_pd_register(struct drm_device *drm, struct imx_parallel_display *imxpd) { @@ -276,7 +283,13 @@ static int imx_pd_register(struct drm_device *drm, */ imxpd->connector.dpms = DRM_MODE_DPMS_OFF;
- drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE); + ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE); + if (ret) + return ret; + + ret = drmm_add_action_or_reset(drm, imx_pd_encoder_cleanup, encoder); + if (ret) + return ret;
imxpd->bridge.funcs = &imx_pd_bridge_funcs; drm_bridge_attach(encoder, &imxpd->bridge, NULL, 0); @@ -310,12 +323,14 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data) struct device_node *np = dev->of_node; const u8 *edidp; struct imx_parallel_display *imxpd; + int edid_len; int ret; u32 bus_format = 0; const char *fmt;
- imxpd = dev_get_drvdata(dev); - memset(imxpd, 0, sizeof(*imxpd)); + imxpd = drmm_kzalloc(drm, sizeof(*imxpd), GFP_KERNEL); + if (!imxpd) + return -ENOMEM;
/* port@1 is the output port */ ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, @@ -323,9 +338,13 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data) if (ret && ret != -ENODEV) return ret;
- edidp = of_get_property(np, "edid", &imxpd->edid_len); - if (edidp) - imxpd->edid = kmemdup(edidp, imxpd->edid_len, GFP_KERNEL); + edidp = of_get_property(np, "edid", &edid_len); + if (edidp) { + imxpd->edid = drmm_kmalloc(drm, edid_len, GFP_KERNEL); + if (!imxpd->edid) + return -ENOMEM; + memcpy(imxpd->edid, edidp, edid_len); + }
ret = of_property_read_string(np, "interface-pix-fmt", &fmt); if (!ret) { @@ -349,29 +368,12 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data) return 0; }
-static void imx_pd_unbind(struct device *dev, struct device *master, - void *data) -{ - struct imx_parallel_display *imxpd = dev_get_drvdata(dev); - - kfree(imxpd->edid); -} - static const struct component_ops imx_pd_ops = { .bind = imx_pd_bind, - .unbind = imx_pd_unbind, };
static int imx_pd_probe(struct platform_device *pdev) { - struct imx_parallel_display *imxpd; - - imxpd = devm_kzalloc(&pdev->dev, sizeof(*imxpd), GFP_KERNEL); - if (!imxpd) - return -ENOMEM; - - platform_set_drvdata(pdev, imxpd); - return component_add(&pdev->dev, &imx_pd_ops); }
On Wed, 2020-07-22 at 15:30 +0200, Philipp Zabel wrote: [...]
and use drmm_add_action_or_reset() to make sure drm_encoder_cleanup() is called before the memory is freed.
[...]
@@ -259,6 +259,13 @@ static const struct drm_bridge_funcs imx_pd_bridge_funcs = { .atomic_get_output_bus_fmts = imx_pd_bridge_atomic_get_output_bus_fmts, };
+static void imx_pd_encoder_cleanup(struct drm_device *drm, void *ptr) +{
- struct drm_encoder *encoder = ptr;
- drm_encoder_cleanup(encoder);
+}
static int imx_pd_register(struct drm_device *drm, struct imx_parallel_display *imxpd) { @@ -276,7 +283,13 @@ static int imx_pd_register(struct drm_device *drm, */ imxpd->connector.dpms = DRM_MODE_DPMS_OFF;
- drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE);
- ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE);
- if (ret)
return ret;- ret = drmm_add_action_or_reset(drm, imx_pd_encoder_cleanup, encoder);
- if (ret)
return ret;
This is only required because this is a component driver: our drmm_kzalloc() is called after drmm_mode_config_init(), so we can't rely on drm_mode_config_init_release() for cleanup. That is only called after drmres already freed our memory.
regards Philipp
On Wed, Jul 22, 2020 at 04:01:53PM +0200, Philipp Zabel wrote:
On Wed, 2020-07-22 at 15:30 +0200, Philipp Zabel wrote: [...]
and use drmm_add_action_or_reset() to make sure drm_encoder_cleanup() is called before the memory is freed.
[...]
@@ -259,6 +259,13 @@ static const struct drm_bridge_funcs imx_pd_bridge_funcs = { .atomic_get_output_bus_fmts = imx_pd_bridge_atomic_get_output_bus_fmts, };
+static void imx_pd_encoder_cleanup(struct drm_device *drm, void *ptr) +{
- struct drm_encoder *encoder = ptr;
- drm_encoder_cleanup(encoder);
+}
static int imx_pd_register(struct drm_device *drm, struct imx_parallel_display *imxpd) { @@ -276,7 +283,13 @@ static int imx_pd_register(struct drm_device *drm, */ imxpd->connector.dpms = DRM_MODE_DPMS_OFF;
- drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE);
- ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE);
- if (ret)
return ret;- ret = drmm_add_action_or_reset(drm, imx_pd_encoder_cleanup, encoder);
- if (ret)
return ret;This is only required because this is a component driver: our drmm_kzalloc() is called after drmm_mode_config_init(), so we can't rely on drm_mode_config_init_release() for cleanup. That is only called after drmres already freed our memory.
Yeah I know about the inversion, which is why I haven't yet started typing the mass conversion for all the drm objects. I think the explicit drmm_add_action_or_reset is indeed the way to go, except we probably want some helpers to wrap the allocation, drm_foo_init and adding the reset action all into one thing (plus you can stuff the reset action into the allocation instead of the kfree action only, even nicer that way).
But that's maybe for later, and good to have some examples in drivers already converted over as guidance.
On the series: Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
But way too late for solid review :-)
Cheers, Daniel
regards Philipp _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Use drmm_kzalloc() to align plane memory lifetime with the drm device, and use drmm_add_action_or_reset() to make sure drm_plane_cleanup() is called before the memory is freed. Also handle error return values of the plane property creation functions.
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de --- drivers/gpu/drm/imx/ipuv3-plane.c | 34 +++++++++++++++++-------------- 1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index 6776ebb3246d..9543e4c2907a 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -11,6 +11,7 @@ #include <drm/drm_fourcc.h> #include <drm/drm_gem_cma_helper.h> #include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_plane_helper.h>
#include <video/imx-ipu-v3.h> @@ -262,16 +263,6 @@ void ipu_plane_disable_deferred(struct drm_plane *plane) } EXPORT_SYMBOL_GPL(ipu_plane_disable_deferred);
-static void ipu_plane_destroy(struct drm_plane *plane) -{ - struct ipu_plane *ipu_plane = to_ipu_plane(plane); - - DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__); - - drm_plane_cleanup(plane); - kfree(ipu_plane); -} - static void ipu_plane_state_reset(struct drm_plane *plane) { unsigned int zpos = (plane->type == DRM_PLANE_TYPE_PRIMARY) ? 0 : 1; @@ -336,7 +327,6 @@ static bool ipu_plane_format_mod_supported(struct drm_plane *plane, static const struct drm_plane_funcs ipu_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, - .destroy = ipu_plane_destroy, .reset = ipu_plane_state_reset, .atomic_duplicate_state = ipu_plane_duplicate_state, .atomic_destroy_state = ipu_plane_destroy_state, @@ -822,6 +812,13 @@ int ipu_planes_assign_pre(struct drm_device *dev, } EXPORT_SYMBOL_GPL(ipu_planes_assign_pre);
+static void ipu_plane_cleanup(struct drm_device *dev, void *data) +{ + struct ipu_plane *ipu_plane = data; + + drm_plane_cleanup(&ipu_plane->base); +} + struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu, int dma, int dp, unsigned int possible_crtcs, enum drm_plane_type type) @@ -834,7 +831,7 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu, DRM_DEBUG_KMS("channel %d, dp flow %d, possible_crtcs=0x%x\n", dma, dp, possible_crtcs);
- ipu_plane = kzalloc(sizeof(*ipu_plane), GFP_KERNEL); + ipu_plane = drmm_kzalloc(dev, sizeof(*ipu_plane), GFP_KERNEL); if (!ipu_plane) { DRM_ERROR("failed to allocate plane\n"); return ERR_PTR(-ENOMEM); @@ -853,16 +850,23 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu, modifiers, type, NULL); if (ret) { DRM_ERROR("failed to initialize plane\n"); - kfree(ipu_plane); return ERR_PTR(ret); }
+ ret = drmm_add_action_or_reset(dev, ipu_plane_cleanup, ipu_plane); + if (ret) + return ERR_PTR(ret); + drm_plane_helper_add(&ipu_plane->base, &ipu_plane_helper_funcs);
if (dp == IPU_DP_FLOW_SYNC_BG || dp == IPU_DP_FLOW_SYNC_FG) - drm_plane_create_zpos_property(&ipu_plane->base, zpos, 0, 1); + ret = drm_plane_create_zpos_property(&ipu_plane->base, zpos, 0, + 1); else - drm_plane_create_zpos_immutable_property(&ipu_plane->base, 0); + ret = drm_plane_create_zpos_immutable_property(&ipu_plane->base, + 0); + if (ret) + return ERR_PTR(ret);
return ipu_plane; }
Use drm managed resources to get and put IPU resources automatically.
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de --- drivers/gpu/drm/imx/ipuv3-crtc.c | 25 +------------------------ drivers/gpu/drm/imx/ipuv3-plane.c | 29 ++++++++++++++++++++--------- drivers/gpu/drm/imx/ipuv3-plane.h | 3 --- 3 files changed, 21 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 2256c9789fc2..b0dacbadaf52 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -385,29 +385,14 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, drm_crtc_init_with_planes(drm, crtc, &ipu_crtc->plane[0]->base, NULL, &ipu_crtc_funcs, NULL);
- ret = ipu_plane_get_resources(ipu_crtc->plane[0]); - if (ret) { - dev_err(ipu_crtc->dev, "getting plane 0 resources failed with %d.\n", - ret); - goto err_put_resources; - } - /* If this crtc is using the DP, add an overlay plane */ if (pdata->dp >= 0 && pdata->dma[1] > 0) { ipu_crtc->plane[1] = ipu_plane_init(drm, ipu, pdata->dma[1], IPU_DP_FLOW_SYNC_FG, drm_crtc_mask(&ipu_crtc->base), DRM_PLANE_TYPE_OVERLAY); - if (IS_ERR(ipu_crtc->plane[1])) { + if (IS_ERR(ipu_crtc->plane[1])) ipu_crtc->plane[1] = NULL; - } else { - ret = ipu_plane_get_resources(ipu_crtc->plane[1]); - if (ret) { - dev_err(ipu_crtc->dev, "getting plane 1 " - "resources failed with %d.\n", ret); - goto err_put_plane0_res; - } - } }
ipu_crtc->irq = ipu_plane_irq(ipu_crtc->plane[0]); @@ -422,11 +407,6 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
return 0;
-err_put_plane1_res: - if (ipu_crtc->plane[1]) - ipu_plane_put_resources(ipu_crtc->plane[1]); -err_put_plane0_res: - ipu_plane_put_resources(ipu_crtc->plane[0]); err_put_resources: ipu_put_resources(ipu_crtc);
@@ -453,9 +433,6 @@ static void ipu_drm_unbind(struct device *dev, struct device *master, struct ipu_crtc *ipu_crtc = dev_get_drvdata(dev);
ipu_put_resources(ipu_crtc); - if (ipu_crtc->plane[1]) - ipu_plane_put_resources(ipu_crtc->plane[1]); - ipu_plane_put_resources(ipu_crtc->plane[0]); }
static const struct component_ops ipu_crtc_ops = { diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index 9543e4c2907a..d7464051514f 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -143,8 +143,10 @@ drm_plane_state_to_vbo(struct drm_plane_state *state) fb->format->cpp[2] * x - eba; }
-void ipu_plane_put_resources(struct ipu_plane *ipu_plane) +static void ipu_plane_put_resources(struct drm_device *dev, void *ptr) { + struct ipu_plane *ipu_plane = ptr; + if (!IS_ERR_OR_NULL(ipu_plane->dp)) ipu_dp_put(ipu_plane->dp); if (!IS_ERR_OR_NULL(ipu_plane->dmfc)) @@ -155,7 +157,8 @@ void ipu_plane_put_resources(struct ipu_plane *ipu_plane) ipu_idmac_put(ipu_plane->alpha_ch); }
-int ipu_plane_get_resources(struct ipu_plane *ipu_plane) +static int ipu_plane_get_resources(struct drm_device *dev, + struct ipu_plane *ipu_plane) { int ret; int alpha_ch; @@ -167,6 +170,10 @@ int ipu_plane_get_resources(struct ipu_plane *ipu_plane) return ret; }
+ ret = drmm_add_action_or_reset(dev, ipu_plane_put_resources, ipu_plane); + if (ret) + return ret; + alpha_ch = ipu_channel_alpha_channel(ipu_plane->dma); if (alpha_ch >= 0) { ipu_plane->alpha_ch = ipu_idmac_get(ipu_plane->ipu, alpha_ch); @@ -182,7 +189,7 @@ int ipu_plane_get_resources(struct ipu_plane *ipu_plane) if (IS_ERR(ipu_plane->dmfc)) { ret = PTR_ERR(ipu_plane->dmfc); DRM_ERROR("failed to get dmfc: ret %d\n", ret); - goto err_out; + return ret; }
if (ipu_plane->dp_flow >= 0) { @@ -190,15 +197,11 @@ int ipu_plane_get_resources(struct ipu_plane *ipu_plane) if (IS_ERR(ipu_plane->dp)) { ret = PTR_ERR(ipu_plane->dp); DRM_ERROR("failed to get dp flow: %d\n", ret); - goto err_out; + return ret; } }
return 0; -err_out: - ipu_plane_put_resources(ipu_plane); - - return ret; }
static bool ipu_plane_separate_alpha(struct ipu_plane *ipu_plane) @@ -849,7 +852,8 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu, ARRAY_SIZE(ipu_plane_formats), modifiers, type, NULL); if (ret) { - DRM_ERROR("failed to initialize plane\n"); + DRM_ERROR("failed to initialize %s plane\n", + zpos ? "overlay" : "primary"); return ERR_PTR(ret); }
@@ -868,5 +872,12 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu, if (ret) return ERR_PTR(ret);
+ ret = ipu_plane_get_resources(dev, ipu_plane); + if (ret) { + DRM_ERROR("failed to get %s plane resources: %pe\n", + zpos ? "overlay" : "primary", &ret); + return ERR_PTR(ret); + } + return ipu_plane; } diff --git a/drivers/gpu/drm/imx/ipuv3-plane.h b/drivers/gpu/drm/imx/ipuv3-plane.h index ffacbcdd2f98..6d544e6ce63f 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.h +++ b/drivers/gpu/drm/imx/ipuv3-plane.h @@ -41,9 +41,6 @@ int ipu_plane_mode_set(struct ipu_plane *plane, struct drm_crtc *crtc, uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h, bool interlaced);
-int ipu_plane_get_resources(struct ipu_plane *plane); -void ipu_plane_put_resources(struct ipu_plane *plane); - int ipu_plane_irq(struct ipu_plane *plane);
void ipu_plane_disable(struct ipu_plane *ipu_plane, bool disable_dp_channel);
Use drmm_kzalloc() to align crtc memory lifetime with the drm device, and use drmm_add_action_or_reset() to make sure IPU resources are released and drm_crtc_cleanup() is called before the memory is freed.
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de --- drivers/gpu/drm/imx/ipuv3-crtc.c | 75 ++++++++++++++------------------ 1 file changed, 33 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index b0dacbadaf52..0e2f4b30d9ba 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -20,6 +20,7 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_gem_cma_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_probe_helper.h> #include <drm/drm_vblank.h>
@@ -166,7 +167,6 @@ static void ipu_disable_vblank(struct drm_crtc *crtc)
static const struct drm_crtc_funcs ipu_crtc_funcs = { .set_config = drm_atomic_helper_set_config, - .destroy = drm_crtc_cleanup, .page_flip = drm_atomic_helper_page_flip, .reset = imx_drm_crtc_reset, .atomic_duplicate_state = imx_drm_crtc_duplicate_state, @@ -323,37 +323,42 @@ static const struct drm_crtc_helper_funcs ipu_helper_funcs = { .atomic_enable = ipu_crtc_atomic_enable, };
-static void ipu_put_resources(struct ipu_crtc *ipu_crtc) +static void ipu_put_resources(struct drm_device *dev, void *ptr) { + struct ipu_crtc *ipu_crtc = ptr; + if (!IS_ERR_OR_NULL(ipu_crtc->dc)) ipu_dc_put(ipu_crtc->dc); if (!IS_ERR_OR_NULL(ipu_crtc->di)) ipu_di_put(ipu_crtc->di); }
-static int ipu_get_resources(struct ipu_crtc *ipu_crtc, +static int ipu_get_resources(struct drm_device *dev, struct ipu_crtc *ipu_crtc, struct ipu_client_platformdata *pdata) { struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent); int ret;
ipu_crtc->dc = ipu_dc_get(ipu, pdata->dc); - if (IS_ERR(ipu_crtc->dc)) { - ret = PTR_ERR(ipu_crtc->dc); - goto err_out; - } + if (IS_ERR(ipu_crtc->dc)) + return PTR_ERR(ipu_crtc->dc); + + ret = drmm_add_action_or_reset(dev, ipu_put_resources, ipu_crtc); + if (ret) + return ret;
ipu_crtc->di = ipu_di_get(ipu, pdata->di); - if (IS_ERR(ipu_crtc->di)) { - ret = PTR_ERR(ipu_crtc->di); - goto err_out; - } + if (IS_ERR(ipu_crtc->di)) + return PTR_ERR(ipu_crtc->di);
return 0; -err_out: - ipu_put_resources(ipu_crtc); +}
- return ret; +static void ipu_crtc_cleanup(struct drm_device *drm, void *ptr) +{ + struct drm_crtc *crtc = ptr; + + drm_crtc_cleanup(crtc); }
static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, @@ -364,7 +369,7 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, int dp = -EINVAL; int ret;
- ret = ipu_get_resources(ipu_crtc, pdata); + ret = ipu_get_resources(drm, ipu_crtc, pdata); if (ret) { dev_err(ipu_crtc->dev, "getting resources failed with %d.\n", ret); @@ -377,13 +382,19 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, DRM_PLANE_TYPE_PRIMARY); if (IS_ERR(ipu_crtc->plane[0])) { ret = PTR_ERR(ipu_crtc->plane[0]); - goto err_put_resources; + return ret; }
crtc->port = pdata->of_node; drm_crtc_helper_add(crtc, &ipu_helper_funcs); - drm_crtc_init_with_planes(drm, crtc, &ipu_crtc->plane[0]->base, NULL, - &ipu_crtc_funcs, NULL); + ret = drm_crtc_init_with_planes(drm, crtc, &ipu_crtc->plane[0]->base, + NULL, &ipu_crtc_funcs, NULL); + if (ret) + return ret; + + ret = drmm_add_action_or_reset(drm, ipu_crtc_cleanup, crtc); + if (ret) + return ret;
/* If this crtc is using the DP, add an overlay plane */ if (pdata->dp >= 0 && pdata->dma[1] > 0) { @@ -400,17 +411,12 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, "imx_drm", ipu_crtc); if (ret < 0) { dev_err(ipu_crtc->dev, "irq request failed with %d.\n", ret); - goto err_put_plane1_res; + return ret; } /* Only enable IRQ when we actually need it to trigger work. */ disable_irq(ipu_crtc->irq);
return 0; - -err_put_resources: - ipu_put_resources(ipu_crtc); - - return ret; }
static int ipu_drm_bind(struct device *dev, struct device *master, void *data) @@ -419,31 +425,22 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data) struct drm_device *drm = data; struct ipu_crtc *ipu_crtc;
- ipu_crtc = dev_get_drvdata(dev); - memset(ipu_crtc, 0, sizeof(*ipu_crtc)); + ipu_crtc = drmm_kzalloc(drm, sizeof(*ipu_crtc), GFP_KERNEL); + if (!ipu_crtc) + return -ENOMEM;
ipu_crtc->dev = dev;
return ipu_crtc_init(ipu_crtc, pdata, drm); }
-static void ipu_drm_unbind(struct device *dev, struct device *master, - void *data) -{ - struct ipu_crtc *ipu_crtc = dev_get_drvdata(dev); - - ipu_put_resources(ipu_crtc); -} - static const struct component_ops ipu_crtc_ops = { .bind = ipu_drm_bind, - .unbind = ipu_drm_unbind, };
static int ipu_drm_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct ipu_crtc *ipu_crtc; int ret;
if (!dev->platform_data) @@ -453,12 +450,6 @@ static int ipu_drm_probe(struct platform_device *pdev) if (ret) return ret;
- ipu_crtc = devm_kzalloc(dev, sizeof(*ipu_crtc), GFP_KERNEL); - if (!ipu_crtc) - return -ENOMEM; - - dev_set_drvdata(dev, ipu_crtc); - return component_add(dev, &ipu_crtc_ops); }
dri-devel@lists.freedesktop.org
-
daniel＠ffwll.ch -
Philipp Zabel