[PATCH 1/2] drm/imx: ipuv3-crtc: Return error if ipu_plane_init() fails for primary plane

[PATCH 1/2] drm/imx: ipuv3-crtc: Return error if ipu_plane_init() fails for primary plane

[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;
+	}
 	ret = ipu_plane_get_resources(ipu_crtc->plane[0]);
 	if (ret) {
 		dev_err(ipu_crtc->dev, "getting plane 0 resources failed with %d.\n",
-- 
2.5.0

[PATCH 2/2] drm/imx: Remove the primary plane created by create_primary_plane()

[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);
 
 	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) {
-- 
2.5.0

Re: [PATCH 2/2] drm/imx: Remove the primary plane created by create_primary_plane()

[Philipp Zabel]

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

Re: [PATCH 2/2] drm/imx: Remove the primary plane created by create_primary_plane()

[Liu Ying]

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
> 

Re: [PATCH 2/2] drm/imx: Remove the primary plane created by create_primary_plane()

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2] drm/imx: Remove the primary plane created by create_primary_plane()

[Liu Ying]

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

Re: [PATCH 1/2] drm/imx: ipuv3-crtc: Return error if ipu_plane_init() fails for primary plane

[Philipp Zabel]

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

Re: [PATCH 1/2] drm/imx: ipuv3-crtc: Return error if ipu_plane_init() fails for primary plane

[Liu Ying]

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
>