From: Daniel Vetter <daniel@ffwll.ch> To: Liu Ying <Ying.Liu@freescale.com> Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] drm/imx: Remove the primary plane created by create_primary_plane() Date: Mon, 16 Nov 2015 17:00:21 +0100 [thread overview] Message-ID: <20151116160021.GX16848@phenom.ffwll.local> (raw) In-Reply-To: <1446632158-19998-2-git-send-email-Ying.Liu@freescale.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: Liu Ying <Ying.Liu@freescale.com> Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 2/2] drm/imx: Remove the primary plane created by create_primary_plane() Date: Mon, 16 Nov 2015 17:00:21 +0100 [thread overview] Message-ID: <20151116160021.GX16848@phenom.ffwll.local> (raw) In-Reply-To: <1446632158-19998-2-git-send-email-Ying.Liu@freescale.com> 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2015-11-16 16:00 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-11-04 10:15 [PATCH 1/2] drm/imx: ipuv3-crtc: Return error if ipu_plane_init() fails for primary plane Liu Ying 2015-11-04 10:15 ` Liu Ying 2015-11-04 10:15 ` [PATCH 2/2] drm/imx: Remove the primary plane created by create_primary_plane() Liu Ying 2015-11-04 10:15 ` Liu Ying 2015-11-06 10:05 ` Philipp Zabel 2015-11-06 10:05 ` Philipp Zabel 2015-11-06 10:38 ` Liu Ying 2015-11-06 10:38 ` Liu Ying 2015-11-16 16:00 ` Daniel Vetter [this message] 2015-11-16 16:00 ` Daniel Vetter 2015-11-17 2:10 ` Liu Ying 2015-11-17 2:10 ` Liu Ying 2015-11-06 10:05 ` [PATCH 1/2] drm/imx: ipuv3-crtc: Return error if ipu_plane_init() fails for primary plane Philipp Zabel 2015-11-06 10:05 ` Philipp Zabel 2015-11-06 10:25 ` Liu Ying 2015-11-06 10:25 ` Liu Ying
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20151116160021.GX16848@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=Ying.Liu@freescale.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.