| Message ID | 20200722133042.30140-5-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] drm/imx: drop explicit drm_mode_config_cleanup | expand |
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
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); }
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(-)