This patch adds a helper ipu_plane_cleanup() to cleanup a IPU plane. It can be used in the bailout path of ipu_crtc_init(), for instance.
Signed-off-by: Liu Ying Ying.Liu@freescale.com --- This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-plane.c | 6 ++++++ drivers/gpu/drm/imx/ipuv3-plane.h | 2 ++ 2 files changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index e2ff410..e60b382 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -410,3 +410,9 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
return ipu_plane; } + +void ipu_plane_cleanup(struct ipu_plane *ipu_plane) +{ + drm_plane_cleanup(&ipu_plane->base); + kfree(ipu_plane); +} diff --git a/drivers/gpu/drm/imx/ipuv3-plane.h b/drivers/gpu/drm/imx/ipuv3-plane.h index 3a443b4..dd2239c 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.h +++ b/drivers/gpu/drm/imx/ipuv3-plane.h @@ -36,6 +36,8 @@ 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);
+void ipu_plane_cleanup(struct ipu_plane *ipu_plane); + /* Init IDMAC, DMFC, DP */ int ipu_plane_mode_set(struct ipu_plane *plane, struct drm_crtc *crtc, struct drm_display_mode *mode,
To avoid memory leakage, we need to cleanup the initialized ipu planes in the bailout path of ipu_crtc_init().
Signed-off-by: Liu Ying Ying.Liu@freescale.com --- This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-crtc.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 67813ca..59f44df 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -371,7 +371,7 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, ipu_crtc->dev->of_node); if (ret) { dev_err(ipu_crtc->dev, "adding crtc failed with %d.\n", ret); - goto err_put_resources; + goto err_cleanup_plane0; }
ret = ipu_plane_get_resources(ipu_crtc->plane[0]); @@ -402,9 +402,14 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, return 0;
err_put_plane_res: + if (ipu_crtc->plane[1]) + ipu_plane_cleanup(ipu_crtc->plane[1]); + ipu_plane_put_resources(ipu_crtc->plane[0]); err_remove_crtc: imx_drm_remove_crtc(ipu_crtc->imx_crtc); +err_cleanup_plane0: + ipu_plane_cleanup(ipu_crtc->plane[0]); err_put_resources: ipu_put_resources(ipu_crtc);
Am Freitag, den 20.11.2015, 16:14 +0800 schrieb Liu Ying:
To avoid memory leakage, we need to cleanup the initialized ipu planes in the bailout path of ipu_crtc_init().
Signed-off-by: Liu Ying Ying.Liu@freescale.com
This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-crtc.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 67813ca..59f44df 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -371,7 +371,7 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, ipu_crtc->dev->of_node); if (ret) { dev_err(ipu_crtc->dev, "adding crtc failed with %d.\n", ret);
goto err_put_resources;
goto err_cleanup_plane0;}
ret = ipu_plane_get_resources(ipu_crtc->plane[0]);
@@ -402,9 +402,14 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, return 0;
err_put_plane_res:
- if (ipu_crtc->plane[1])
ipu_plane_cleanup(ipu_crtc->plane[1]);- ipu_plane_put_resources(ipu_crtc->plane[0]);
err_remove_crtc: imx_drm_remove_crtc(ipu_crtc->imx_crtc); +err_cleanup_plane0:
- ipu_plane_cleanup(ipu_crtc->plane[0]);
err_put_resources: ipu_put_resources(ipu_crtc);
I think we can use ipu_plane_destroy as-is instead of ipu_plane_put_resources + ipu_plane_cleanup.
regards Philipp
To avoid memory leakage, we need to cleanup ipu planes in ipu_drm_unbind().
Signed-off-by: Liu Ying Ying.Liu@freescale.com --- This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-crtc.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 59f44df..467905c 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -446,6 +446,11 @@ static void ipu_drm_unbind(struct device *dev, struct device *master, imx_drm_remove_crtc(ipu_crtc->imx_crtc);
ipu_plane_put_resources(ipu_crtc->plane[0]); + + if (ipu_crtc->plane[1]) + ipu_plane_cleanup(ipu_crtc->plane[1]); + ipu_plane_cleanup(ipu_crtc->plane[0]); + ipu_put_resources(ipu_crtc); }
Am Freitag, den 20.11.2015, 16:14 +0800 schrieb Liu Ying:
To avoid memory leakage, we need to cleanup ipu planes in ipu_drm_unbind().
Signed-off-by: Liu Ying Ying.Liu@freescale.com
This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-crtc.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 59f44df..467905c 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -446,6 +446,11 @@ static void ipu_drm_unbind(struct device *dev, struct device *master, imx_drm_remove_crtc(ipu_crtc->imx_crtc);
ipu_plane_put_resources(ipu_crtc->plane[0]);
- if (ipu_crtc->plane[1])
ipu_plane_cleanup(ipu_crtc->plane[1]);- ipu_plane_cleanup(ipu_crtc->plane[0]);
- ipu_put_resources(ipu_crtc);
}
Shouldn't this already be handled by the DRM core calling the plane->destroy callbacks from drm_mode_config_cleanup (called by imx_drm_driver_unload shortly after component_unbind_all)?
regards Philipp
On Mon, Nov 23, 2015 at 12:48:14PM +0100, Philipp Zabel wrote:
Am Freitag, den 20.11.2015, 16:14 +0800 schrieb Liu Ying:
To avoid memory leakage, we need to cleanup ipu planes in ipu_drm_unbind().
Signed-off-by: Liu Ying Ying.Liu@freescale.com
This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-crtc.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 59f44df..467905c 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -446,6 +446,11 @@ static void ipu_drm_unbind(struct device *dev, struct device *master, imx_drm_remove_crtc(ipu_crtc->imx_crtc);
ipu_plane_put_resources(ipu_crtc->plane[0]);
- if (ipu_crtc->plane[1])
ipu_plane_cleanup(ipu_crtc->plane[1]);- ipu_plane_cleanup(ipu_crtc->plane[0]);
- ipu_put_resources(ipu_crtc);
}
Shouldn't this already be handled by the DRM core calling the plane->destroy callbacks from drm_mode_config_cleanup (called by imx_drm_driver_unload shortly after component_unbind_all)?
I take drm_mode_config_cleanup() as the final goal keeper. The component ->unbind() may clean things up by itself other than rely on the master's behaviour. Otherwise, we even don't need to call ipu_plane_put_resources() here.
Regards, Liu Ying
regards Philipp
To reduce code duplication, we can use the helper ipu_plane_cleanup() in ipu_plane_destroy().
Signed-off-by: Liu Ying Ying.Liu@freescale.com --- This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-plane.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index e60b382..b3ed207 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -369,8 +369,7 @@ static void ipu_plane_destroy(struct drm_plane *plane) DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
ipu_disable_plane(plane); - drm_plane_cleanup(plane); - kfree(ipu_plane); + ipu_plane_cleanup(ipu_plane); }
static struct drm_plane_funcs ipu_plane_funcs = {
Am Freitag, den 20.11.2015, 16:14 +0800 schrieb Liu Ying:
To reduce code duplication, we can use the helper ipu_plane_cleanup() in ipu_plane_destroy().
Signed-off-by: Liu Ying Ying.Liu@freescale.com
This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-plane.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index e60b382..b3ed207 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -369,8 +369,7 @@ static void ipu_plane_destroy(struct drm_plane *plane) DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
ipu_disable_plane(plane);
- drm_plane_cleanup(plane);
- kfree(ipu_plane);
- ipu_plane_cleanup(ipu_plane);
}
static struct drm_plane_funcs ipu_plane_funcs = {
This could be merged into the first patch, but I don't think ipu_plane_cleanup is necessary at all.
regards Philipp
On Mon, Nov 23, 2015 at 12:48:12PM +0100, Philipp Zabel wrote:
Am Freitag, den 20.11.2015, 16:14 +0800 schrieb Liu Ying:
To reduce code duplication, we can use the helper ipu_plane_cleanup() in ipu_plane_destroy().
Signed-off-by: Liu Ying Ying.Liu@freescale.com
This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-plane.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index e60b382..b3ed207 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -369,8 +369,7 @@ static void ipu_plane_destroy(struct drm_plane *plane) DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
ipu_disable_plane(plane);
- drm_plane_cleanup(plane);
- kfree(ipu_plane);
- ipu_plane_cleanup(ipu_plane);
}
static struct drm_plane_funcs ipu_plane_funcs = {
This could be merged into the first patch, but I don't think ipu_plane_cleanup is necessary at all.
IMHO, it doesn't hurt to split it up as two patches :)
Regards, Liu Ying
regards Philipp
This patch changes the dev_info() call to dev_dbg() in ipu_plane_update() to print out the information that a plane's CRTC is changed, because this kind of information is only useful for debugging.
Signed-off-by: Liu Ying Ying.Liu@freescale.com --- This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index b3ed207..b24bf94 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -338,7 +338,7 @@ static int ipu_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, }
if (crtc != plane->crtc) - dev_info(plane->dev->dev, "crtc change: %p -> %p\n", + dev_dbg(plane->dev->dev, "crtc change: %p -> %p\n", plane->crtc, crtc); plane->crtc = crtc;
Am Freitag, den 20.11.2015, 16:14 +0800 schrieb Liu Ying:
This patch changes the dev_info() call to dev_dbg() in ipu_plane_update() to print out the information that a plane's CRTC is changed, because this kind of information is only useful for debugging.
Signed-off-by: Liu Ying Ying.Liu@freescale.com
This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index b3ed207..b24bf94 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -338,7 +338,7 @@ static int ipu_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, }
if (crtc != plane->crtc)
dev_info(plane->dev->dev, "crtc change: %p -> %p\n",
dev_dbg(plane->dev->dev, "crtc change: %p -> %p\n", plane->crtc, crtc);
This change is separate from the others, I have applied it.
thanks Philipp
Hi Philipp,
2015-11-23 19:48 GMT+08:00 Philipp Zabel p.zabel@pengutronix.de:
Am Freitag, den 20.11.2015, 16:14 +0800 schrieb Liu Ying:
This patch changes the dev_info() call to dev_dbg() in ipu_plane_update() to print out the information that a plane's CRTC is changed, because this kind of information is only useful for debugging.
Signed-off-by: Liu Ying Ying.Liu@freescale.com
This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index b3ed207..b24bf94 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -338,7 +338,7 @@ static int ipu_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, }
if (crtc != plane->crtc)
dev_info(plane->dev->dev, "crtc change: %p -> %p\n",
dev_dbg(plane->dev->dev, "crtc change: %p -> %p\n", plane->crtc, crtc); plane->crtc = crtc;This change is separate from the others, I have applied it.
I don't find this patch on any imx-drm branch in your open git repository. Do I miss anything?
Regards, Liu Ying
thanks Philipp
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Liu,
Am Freitag, den 22.01.2016, 10:29 +0800 schrieb Liu Ying:
Hi Philipp,
2015-11-23 19:48 GMT+08:00 Philipp Zabel p.zabel@pengutronix.de:
Am Freitag, den 20.11.2015, 16:14 +0800 schrieb Liu Ying:
This patch changes the dev_info() call to dev_dbg() in ipu_plane_update() to print out the information that a plane's CRTC is changed, because this kind of information is only useful for debugging.
Signed-off-by: Liu Ying Ying.Liu@freescale.com
This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index b3ed207..b24bf94 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -338,7 +338,7 @@ static int ipu_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, }
if (crtc != plane->crtc)
dev_info(plane->dev->dev, "crtc change: %p -> %p\n",
dev_dbg(plane->dev->dev, "crtc change: %p -> %p\n", plane->crtc, crtc); plane->crtc = crtc;This change is separate from the others, I have applied it.
I don't find this patch on any imx-drm branch in your open git repository. Do I miss anything?
It's in the imx-drm/next branch, I have waited for v4.5-rc1 before pushing it.
regards Philipp
Am Freitag, den 20.11.2015, 16:14 +0800 schrieb Liu Ying:
This patch adds a helper ipu_plane_cleanup() to cleanup a IPU plane. It can be used in the bailout path of ipu_crtc_init(), for instance.
Signed-off-by: Liu Ying Ying.Liu@freescale.com
This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-plane.c | 6 ++++++ drivers/gpu/drm/imx/ipuv3-plane.h | 2 ++ 2 files changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index e2ff410..e60b382 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -410,3 +410,9 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
return ipu_plane; }
+void ipu_plane_cleanup(struct ipu_plane *ipu_plane) +{
- drm_plane_cleanup(&ipu_plane->base);
- kfree(ipu_plane);
+}
The name says cleanup, but that's not what it does. This function should be named ipu_plane_free, or ipu_plane_destroy. Actually, we have that already.
regards Philipp
On Mon, Nov 23, 2015 at 12:48:13PM +0100, Philipp Zabel wrote:
Am Freitag, den 20.11.2015, 16:14 +0800 schrieb Liu Ying:
This patch adds a helper ipu_plane_cleanup() to cleanup a IPU plane. It can be used in the bailout path of ipu_crtc_init(), for instance.
Signed-off-by: Liu Ying Ying.Liu@freescale.com
This patch applies to the imx-drm/fixes branch of Philipp Zabel's open git.
drivers/gpu/drm/imx/ipuv3-plane.c | 6 ++++++ drivers/gpu/drm/imx/ipuv3-plane.h | 2 ++ 2 files changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index e2ff410..e60b382 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -410,3 +410,9 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
return ipu_plane; }
+void ipu_plane_cleanup(struct ipu_plane *ipu_plane) +{
- drm_plane_cleanup(&ipu_plane->base);
- kfree(ipu_plane);
+}
The name says cleanup, but that's not what it does. This function should be named ipu_plane_free, or ipu_plane_destroy. Actually, we have that already.
Since ipu_crtc_init() may call ipu_plane_init()/ipu_plane_get_resources() for a plane, the bailout path deserves the same granularity to clarity the logic. It looks somewhat awkward to use the callback plane->destroy() to tear down the plane in the bailout path or to export the static function ipu_plane_destroy() and use it directly.
I prefer to change ipu_plane_cleanup to ipu_plane_free and follow the fine granularity way.
Regards, Liu Ying
regards Philipp
dri-devel@lists.freedesktop.org
-
Liu Ying -
Liu Ying -
Philipp Zabel