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

[Philipp Zabel]

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