Date: Fri, 6 Nov 2015 18:38:36 +0800
From: Liu Ying <Ying.Liu@freescale.com>
To: Philipp Zabel <p.zabel@pengutronix.de>
CC: <dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        <airlied@linux.ie>
Subject: Re: [PATCH 2/2] drm/imx: Remove the primary plane created by
 create_primary_plane()
Message-ID: <20151106103835.GB2036@shlinux2>
References: <1446632158-19998-1-git-send-email-Ying.Liu@freescale.com>
 <1446632158-19998-2-git-send-email-Ying.Liu@freescale.com>
 <1446804348.4569.1.camel@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <1446804348.4569.1.camel@pengutronix.de>
User-Agent: Mutt/1.5.21 (2010-09-15)
SpamDiagnosticOutput: 1:23
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Nov 2015 10:40:41.0581
 (UTC)
X-MS-Exchange-CrossTenant-Id: 710a03f5-10f6-4d38-9ff4-a80b81da590d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=710a03f5-10f6-4d38-9ff4-a80b81da590d;Ip=[192.88.168.50];Helo=[tx30smr01.am.freescale.net]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLUPR0301MB2001
X-Microsoft-Exchange-Diagnostics: 1;BLUPR0301MB2004;2:tOVb+cqK4jTEnOMa5PH+eoS4+lc9RsE6aloLL4xqlf8YFVFFDH2f2VJxTFzZhQnFEAXSiwMLxGvEDqWSmflsWzafthBWUlZOIL+0pAAWsY56hfgwgQ+cnEEC5itZz7NAfi70fjf4YvMuQ5yrIY6SgEmRu4MvaszrKpmhceTk0/M=;23:hrsZ3Wrn9ZxrGB4p4WZCAqcIGC6HMuj0UWPXn5KEXt91xxcEnzg2E4lvmjeQUVhK1WnswChclC0szqeuF6JJuaCTkniR6jhnehcAuJ3fo9QofuNLcxVULFbkwI/EmTgWEpnjtOFkZvXKMciaBctmkX+iZnOmI5kEN0AvRMmbUNURny/fVo0hTIU539KygGje
X-OriginatorOrg: freescale.com
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2981
Lines: 64

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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/