| Message ID | 1446632158-19998-2-git-send-email-Ying.Liu@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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) {
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(-)