Re: [PATCH v9 02/14] drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.

From: Philipp Zabel <p.zabel@pengutronix.de>
To: Daniel Kurtz <djkurtz@chromium.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Michael Turquette <mturquette@baylibre.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Jie Qiu <jie.qiu@mediatek.com>,
	Cawa Cheng <cawa.cheng@mediatek.com>,
	YT Shen <yt.shen@mediatek.com>,
	Yingjoe Chen <yingjoe.chen@mediatek.com>,
	"open list:OPEN FIRMWARE AND..." <devicetree@vger.kernel.org>,
	Jitao Shi <jitao.shi@mediatek.com>,
	Sasha Hauer <kernel@pengutronix.de>,
	Pawel Moll <pawel.moll@arm.com>,
	Ian Campbell <ijc+devicetree@hellion.org.uk>,
	Rob Herring <robh+dt@kernel.org>,
	"moderated list:ARM/Mediatek SoC support"
	<linux-mediatek@lists.infradead.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Paul Bolle <pebolle@tiscali.nl>,
	Stephen Boyd <sboyd@codeaurora.org>,
	Tomasz Figa <tfiga@chromium.org>,
	Kumar Gala <galak@codeaurora.org>
Subject: Re: [PATCH v9 02/14] drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.
Date: Wed, 03 Feb 2016 15:31:09 +0100	[thread overview]
Message-ID: <1454509869.3867.50.camel@pengutronix.de> (raw)
In-Reply-To: <CAGS+omCf7Xdc3gX1OZO4DUccVRJNx0+oVUsuqu_q_zTC_T8YpA@mail.gmail.com>

Hi Daniel,

Am Mittwoch, den 03.02.2016, 01:12 +0800 schrieb Daniel Kurtz:
[...]
> > +int mtk_drm_crtc_create(struct drm_device *drm_dev,
> > +                       const enum mtk_ddp_comp_id *path, unsigned int path_len)
> > +{
[...]
> > +       for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> > +               enum mtk_ddp_comp_id comp_id = path[i];
> > +               struct mtk_ddp_comp *comp;

I'll check for (priv->comp_node[comp_id] != NULL) here.

> > +               comp = priv->ddp_comp[comp_id];
> > +               if (!comp) {
> > +                       dev_err(dev, "Component %s not initialized\n",
> > +                               priv->comp_node[comp_id]->full_name);
> 
> If one of the components is disabled in .dtsi, then its
> priv->comp_node[comp_id] will be NULL here, and trying to full_name will OOPS.

^^

[...]
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > new file mode 100644
> > index 0000000..9db22b4
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
[...]
> > +static struct drm_driver mtk_drm_driver = {
> > +       .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME |
> > +                          DRIVER_ATOMIC,
> > +       .unload = mtk_drm_unload,
> > +       .set_busid = drm_platform_set_busid,
> 
> I think using drm_platform_set_busid() as our .set_busid may cause an OOPs if
> userspace does DRM_IOCTL_SET_VERSION.
> 
> drm_setversion() will calls drm_set_busid(), which calls
> dev->driver->set_busid()
> drm_platform_set_busid() accesses dev->platformdev->id.
> 
> However, dev->platformdev is only set by:
>   drm_platform_init()->drm_get_platform_dev()
> 
> And, since mtk_drm_bind() does the drm_dev_alloc() /
> drm_dev_register() itself instead
> of calling drm_get_platform_dev(), so dev->platformdev will still be NULL.
> 
> So, why don't we call drm_platform_init() instead and implement a .load callback
> to do mtk_drm_kms_init()?

This is deprecated, I had used it that way until v4.
Can we just drop the set_busid callback?

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel