[PATCH 1/2] drm/imx: ipuv3-crtc: Return error if ipu_plane_init() fails for primary plane
For primary plane initialization failure cases, ipu_plane_init() may return a pointer encoded by ERR_PTR(). So, we should bailout instead of use that pointer blindly.
Signed-off-by: Liu Ying Ying.Liu@freescale.com --- drivers/gpu/drm/imx/ipuv3-crtc.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 7bc8301..8d68697 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -370,6 +370,10 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, id = imx_drm_crtc_id(ipu_crtc->imx_crtc); ipu_crtc->plane[0] = ipu_plane_init(ipu_crtc->base.dev, ipu, pdata->dma[0], dp, BIT(id), true); + if (IS_ERR(ipu_crtc->plane[0])) { + ret = PTR_ERR(ipu_crtc->plane[0]); + goto err_put_resources; + } ret = ipu_plane_get_resources(ipu_crtc->plane[0]); if (ret) { dev_err(ipu_crtc->dev, "getting plane 0 resources failed with %d.\n",
Since we are using ipu_plane_init() to add one primary plane for each IPU CRTC, it's unnecessary to create the safe one by using the helper create_primary_plane().
Furthermore, the safe one is attached to crtc->primary, which actually carries a framebuffer(crtc->primary->fb) created by the fbdev helper to build up the fbcon. Instead, the one created by ipu_plane_init() is dangling, but it is the one actually does ipu_plane_mode_set() for the fbcon. This may causes the mismatch bewteen ipu_plane->enabled(true) and ipu_plane->base.fb(NULL). Thus, it brings a NULL pointer dereference issue in ipu_plane_mode_set() when we try to additionally touch the IDMAC channel of the ipu_plane. This issue could be reproduced by running the drm modetest with command line 'modetest -P 19:1024x768@XR24' on the i.MX6Q SabreSD platform(single LVDS display). This patch binds the plane created by ipu_plane_init() with crtc->primary and removes the safe one to address this issue.
Signed-off-by: Liu Ying Ying.Liu@freescale.com --- drivers/gpu/drm/imx/imx-drm-core.c | 3 ++- drivers/gpu/drm/imx/ipuv3-crtc.c | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index 6faa735..08eceeb 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -373,7 +373,8 @@ int imx_drm_add_crtc(struct drm_device *drm, struct drm_crtc *crtc, drm_crtc_helper_add(crtc, imx_drm_crtc->imx_drm_helper_funcs.crtc_helper_funcs);
- drm_crtc_init(drm, crtc, + /* The related primary plane will be created in ipu_plane_init(). */ + drm_crtc_init_with_planes(drm, crtc, NULL, NULL, imx_drm_crtc->imx_drm_helper_funcs.crtc_funcs);
return 0; diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 8d68697..d27143f 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -343,6 +343,11 @@ err_out: return ret; }
+static inline void ipu_crtc_set_primary_plane(struct ipu_crtc *ipu_crtc) +{ + ipu_crtc->base.primary = &ipu_crtc->plane[0]->base; +} + static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, struct ipu_client_platformdata *pdata, struct drm_device *drm) { @@ -380,6 +385,7 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, ret); goto err_remove_crtc; } + ipu_crtc_set_primary_plane(ipu_crtc);
/* If this crtc is using the DP, add an overlay plane */ if (pdata->dp >= 0 && pdata->dma[1] > 0) {
Am Mittwoch, den 04.11.2015, 18:15 +0800 schrieb Liu Ying:
Since we are using ipu_plane_init() to add one primary plane for each IPU CRTC, it's unnecessary to create the safe one by using the helper create_primary_plane().
Furthermore, the safe one is attached to crtc->primary, which actually carries a framebuffer(crtc->primary->fb) created by the fbdev helper to build up the fbcon. Instead, the one created by ipu_plane_init() is dangling, but it is the one actually does ipu_plane_mode_set() for the fbcon. This may causes the mismatch bewteen ipu_plane->enabled(true) and ipu_plane->base.fb(NULL). Thus, it brings a NULL pointer dereference issue in ipu_plane_mode_set() when we try to additionally touch the IDMAC channel of the ipu_plane. This issue could be reproduced by running the drm modetest with command line 'modetest -P 19:1024x768@XR24' on the i.MX6Q SabreSD platform(single LVDS display). This patch binds the plane created by ipu_plane_init() with crtc->primary and removes the safe one to address this issue.
Signed-off-by: Liu Ying Ying.Liu@freescale.com
drivers/gpu/drm/imx/imx-drm-core.c | 3 ++- drivers/gpu/drm/imx/ipuv3-crtc.c | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index 6faa735..08eceeb 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -373,7 +373,8 @@ int imx_drm_add_crtc(struct drm_device *drm, struct drm_crtc *crtc, drm_crtc_helper_add(crtc, imx_drm_crtc->imx_drm_helper_funcs.crtc_helper_funcs);
- drm_crtc_init(drm, crtc,
- /* The related primary plane will be created in ipu_plane_init(). */
- drm_crtc_init_with_planes(drm, crtc, NULL, NULL, imx_drm_crtc->imx_drm_helper_funcs.crtc_funcs);
We rather should generate the plane first and add the primary_plane parameter to imx_drm_add_crtc than calling drm_crtc_init_with_planes without planes.
regards Philipp
On Fri, Nov 06, 2015 at 11:05:48AM +0100, Philipp Zabel wrote:
Am Mittwoch, den 04.11.2015, 18:15 +0800 schrieb Liu Ying:
Since we are using ipu_plane_init() to add one primary plane for each IPU CRTC, it's unnecessary to create the safe one by using the helper create_primary_plane().
Furthermore, the safe one is attached to crtc->primary, which actually carries a framebuffer(crtc->primary->fb) created by the fbdev helper to build up the fbcon. Instead, the one created by ipu_plane_init() is dangling, but it is the one actually does ipu_plane_mode_set() for the fbcon. This may causes the mismatch bewteen ipu_plane->enabled(true) and ipu_plane->base.fb(NULL). Thus, it brings a NULL pointer dereference issue in ipu_plane_mode_set() when we try to additionally touch the IDMAC channel of the ipu_plane. This issue could be reproduced by running the drm modetest with command line 'modetest -P 19:1024x768@XR24' on the i.MX6Q SabreSD platform(single LVDS display). This patch binds the plane created by ipu_plane_init() with crtc->primary and removes the safe one to address this issue.
Signed-off-by: Liu Ying Ying.Liu@freescale.com
drivers/gpu/drm/imx/imx-drm-core.c | 3 ++- drivers/gpu/drm/imx/ipuv3-crtc.c | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index 6faa735..08eceeb 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -373,7 +373,8 @@ int imx_drm_add_crtc(struct drm_device *drm, struct drm_crtc *crtc, drm_crtc_helper_add(crtc, imx_drm_crtc->imx_drm_helper_funcs.crtc_helper_funcs);
- drm_crtc_init(drm, crtc,
- /* The related primary plane will be created in ipu_plane_init(). */
- drm_crtc_init_with_planes(drm, crtc, NULL, NULL, imx_drm_crtc->imx_drm_helper_funcs.crtc_funcs);
We rather should generate the plane first and add the primary_plane parameter to imx_drm_add_crtc than calling drm_crtc_init_with_planes without planes.
Yes, that is the regular way. I thought about that, though I took the easy approach here.
The question is that we currently generate the crtc pipe number in imx_drm_add_crtc() first, and then pass it to ipu_plane_init(). Do you think it would be good to add OF alias id for IPU, pass the id to ipu_crtc_init() via struct ipu_soc and determine the pipe number in ipu_crtc_init() by using the IPU id and pdata->di? This way, ipu_plane_init() may know the pipe number in the first place. Or, any other suggestions?
Regards, Liu Ying
regards Philipp
On Wed, Nov 04, 2015 at 06:15:58PM +0800, Liu Ying wrote:
Since we are using ipu_plane_init() to add one primary plane for each IPU CRTC, it's unnecessary to create the safe one by using the helper create_primary_plane().
Furthermore, the safe one is attached to crtc->primary, which actually carries a framebuffer(crtc->primary->fb) created by the fbdev helper to build up the fbcon. Instead, the one created by ipu_plane_init() is dangling, but it is the one actually does ipu_plane_mode_set() for the fbcon. This may causes the mismatch bewteen ipu_plane->enabled(true) and ipu_plane->base.fb(NULL). Thus, it brings a NULL pointer dereference issue in ipu_plane_mode_set() when we try to additionally touch the IDMAC channel of the ipu_plane. This issue could be reproduced by running the drm modetest with command line 'modetest -P 19:1024x768@XR24' on the i.MX6Q SabreSD platform(single LVDS display). This patch binds the plane created by ipu_plane_init() with crtc->primary and removes the safe one to address this issue.
Signed-off-by: Liu Ying Ying.Liu@freescale.com
drivers/gpu/drm/imx/imx-drm-core.c | 3 ++- drivers/gpu/drm/imx/ipuv3-crtc.c | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index 6faa735..08eceeb 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -373,7 +373,8 @@ int imx_drm_add_crtc(struct drm_device *drm, struct drm_crtc *crtc, drm_crtc_helper_add(crtc, imx_drm_crtc->imx_drm_helper_funcs.crtc_helper_funcs);
- drm_crtc_init(drm, crtc,
/* The related primary plane will be created in ipu_plane_init(). */
drm_crtc_init_with_planes(drm, crtc, NULL, NULL, imx_drm_crtc->imx_drm_helper_funcs.crtc_funcs);
return 0;
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 8d68697..d27143f 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -343,6 +343,11 @@ err_out: return ret; }
+static inline void ipu_crtc_set_primary_plane(struct ipu_crtc *ipu_crtc) +{
- ipu_crtc->base.primary = &ipu_crtc->plane[0]->base;
This is quite a hack. Better would be to reorg the code so that when you call drm_crtc_init_with_planes the primary plane has been created already. -Daniel
+}
static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, struct ipu_client_platformdata *pdata, struct drm_device *drm) { @@ -380,6 +385,7 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, ret); goto err_remove_crtc; }
ipu_crtc_set_primary_plane(ipu_crtc);
/* If this crtc is using the DP, add an overlay plane */ if (pdata->dp >= 0 && pdata->dma[1] > 0) {
-- 2.5.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Nov 16, 2015 at 05:00:21PM +0100, Daniel Vetter wrote:
On Wed, Nov 04, 2015 at 06:15:58PM +0800, Liu Ying wrote:
Since we are using ipu_plane_init() to add one primary plane for each IPU CRTC, it's unnecessary to create the safe one by using the helper create_primary_plane().
Furthermore, the safe one is attached to crtc->primary, which actually carries a framebuffer(crtc->primary->fb) created by the fbdev helper to build up the fbcon. Instead, the one created by ipu_plane_init() is dangling, but it is the one actually does ipu_plane_mode_set() for the fbcon. This may causes the mismatch bewteen ipu_plane->enabled(true) and ipu_plane->base.fb(NULL). Thus, it brings a NULL pointer dereference issue in ipu_plane_mode_set() when we try to additionally touch the IDMAC channel of the ipu_plane. This issue could be reproduced by running the drm modetest with command line 'modetest -P 19:1024x768@XR24' on the i.MX6Q SabreSD platform(single LVDS display). This patch binds the plane created by ipu_plane_init() with crtc->primary and removes the safe one to address this issue.
Signed-off-by: Liu Ying Ying.Liu@freescale.com
drivers/gpu/drm/imx/imx-drm-core.c | 3 ++- drivers/gpu/drm/imx/ipuv3-crtc.c | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index 6faa735..08eceeb 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -373,7 +373,8 @@ int imx_drm_add_crtc(struct drm_device *drm, struct drm_crtc *crtc, drm_crtc_helper_add(crtc, imx_drm_crtc->imx_drm_helper_funcs.crtc_helper_funcs);
- drm_crtc_init(drm, crtc,
/* The related primary plane will be created in ipu_plane_init(). */
drm_crtc_init_with_planes(drm, crtc, NULL, NULL, imx_drm_crtc->imx_drm_helper_funcs.crtc_funcs);
return 0;
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 8d68697..d27143f 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -343,6 +343,11 @@ err_out: return ret; }
+static inline void ipu_crtc_set_primary_plane(struct ipu_crtc *ipu_crtc) +{
- ipu_crtc->base.primary = &ipu_crtc->plane[0]->base;
This is quite a hack. Better would be to reorg the code so that when you call drm_crtc_init_with_planes the primary plane has been created already. -Daniel
Thanks for your comments. Philipp has generated a patch[1] to address this. I acked it conditionally.
[1] http://www.spinics.net/lists/dri-devel/msg93700.html
Regards, Liu Ying
+}
static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, struct ipu_client_platformdata *pdata, struct drm_device *drm) { @@ -380,6 +385,7 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, ret); goto err_remove_crtc; }
ipu_crtc_set_primary_plane(ipu_crtc);
/* If this crtc is using the DP, add an overlay plane */ if (pdata->dp >= 0 && pdata->dma[1] > 0) {
-- 2.5.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Hi Liu,
Am Mittwoch, den 04.11.2015, 18:15 +0800 schrieb Liu Ying:
For primary plane initialization failure cases, ipu_plane_init() may return a pointer encoded by ERR_PTR(). So, we should bailout instead of use that pointer blindly.
Signed-off-by: Liu Ying Ying.Liu@freescale.com
drivers/gpu/drm/imx/ipuv3-crtc.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 7bc8301..8d68697 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -370,6 +370,10 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, id = imx_drm_crtc_id(ipu_crtc->imx_crtc); ipu_crtc->plane[0] = ipu_plane_init(ipu_crtc->base.dev, ipu, pdata->dma[0], dp, BIT(id), true);
- if (IS_ERR(ipu_crtc->plane[0])) {
ret = PTR_ERR(ipu_crtc->plane[0]);goto err_put_resources;
That should be "goto err_remove_crtc;"
regards Philipp
On Fri, Nov 06, 2015 at 11:05:54AM +0100, Philipp Zabel wrote:
Hi Liu,
Am Mittwoch, den 04.11.2015, 18:15 +0800 schrieb Liu Ying:
For primary plane initialization failure cases, ipu_plane_init() may return a pointer encoded by ERR_PTR(). So, we should bailout instead of use that pointer blindly.
Signed-off-by: Liu Ying Ying.Liu@freescale.com
drivers/gpu/drm/imx/ipuv3-crtc.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 7bc8301..8d68697 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -370,6 +370,10 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, id = imx_drm_crtc_id(ipu_crtc->imx_crtc); ipu_crtc->plane[0] = ipu_plane_init(ipu_crtc->base.dev, ipu, pdata->dma[0], dp, BIT(id), true);
- if (IS_ERR(ipu_crtc->plane[0])) {
ret = PTR_ERR(ipu_crtc->plane[0]);goto err_put_resources;That should be "goto err_remove_crtc;"
Ah, yes, you're right. I'll fix this. BTW, there is nothing to free the ipu_plane allocated in ipu_plane_init() in case something goes wrong after ipu_plane_init(). Of course, we may fix this with another patch.
Thanks, Liu Ying
regards Philipp
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Liu Ying -
Philipp Zabel