Hi Philipp,

Some more potential bugs I discovered today while trying to configure
Mediatek DRM to use just the MIPI/DSI path (no HDMI).

On Tue, Jan 12, 2016 at 11:15 PM, Philipp Zabel [off-list ref]
wrote:

From: CK Hu <redacted>

This patch adds an initial DRM driver for the Mediatek MT8173 DISP
subsystem. It currently supports two fixed output streams from the
OVL0/OVL1 sources to the DSI0/DPI0 sinks, respectively.

Signed-off-by: CK Hu <redacted>
Signed-off-by: YT Shen <redacted>
Signed-off-by: Daniel Kurtz <redacted>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

[snip]

+int mtk_drm_crtc_create(struct drm_device *drm_dev,
+                       const enum mtk_ddp_comp_id *path, unsigned int

path_len)

+{
+       struct mtk_drm_private *priv = drm_dev->dev_private;
+       struct device *dev = drm_dev->dev;
+       struct mtk_drm_crtc *mtk_crtc;
+       enum drm_plane_type type;
+       unsigned int zpos;
+       int pipe = priv->num_pipes;
+       int ret;
+       int i;
+
+       mtk_crtc = devm_kzalloc(dev, sizeof(*mtk_crtc), GFP_KERNEL);
+       if (!mtk_crtc)
+               return -ENOMEM;
+
+       mtk_crtc->config_regs = priv->config_regs;
+       mtk_crtc->ddp_comp_nr = path_len;
+       mtk_crtc->ddp_comp = devm_kmalloc_array(dev,

mtk_crtc->ddp_comp_nr,

+

sizeof(*mtk_crtc->ddp_comp),

+                                               GFP_KERNEL);
+
+       mtk_crtc->mutex = mtk_disp_mutex_get(priv->mutex_dev, pipe);
+       if (IS_ERR(mtk_crtc->mutex)) {
+               ret = PTR_ERR(mtk_crtc->mutex);
+               dev_err(dev, "Failed to get mutex: %d\n", ret);
+               return ret;
+       }
+
+       for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
+               enum mtk_ddp_comp_id comp_id = path[i];
+               struct mtk_ddp_comp *comp;
+
+               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.

+                       ret = -ENODEV;
+                       goto unprepare;
+               }
+
+               ret = clk_prepare(comp->clk);
+               if (ret) {
+                       dev_err(dev,
+                               "Failed to prepare clock for component

%s: %d\n",

+                               priv->comp_node[comp_id]->full_name, ret);
+                       goto unprepare;
+               }
+
+               mtk_crtc->ddp_comp[i] = comp;
+       }
+
+       for (zpos = 0; zpos < OVL_LAYER_NR; zpos++) {
+               type = (zpos == 0) ? DRM_PLANE_TYPE_PRIMARY :
+                               (zpos == 1) ? DRM_PLANE_TYPE_CURSOR :
+                                               DRM_PLANE_TYPE_OVERLAY;
+               ret = mtk_plane_init(drm_dev, &mtk_crtc->planes[zpos],
+                                    BIT(pipe), type, zpos);
+               if (ret)
+                       goto unprepare;
+       }
+
+       ret = mtk_drm_crtc_init(drm_dev, mtk_crtc,

&mtk_crtc->planes[0].base,

+                               &mtk_crtc->planes[1].base, pipe);
+       if (ret < 0)
+               goto unprepare;
+
+       priv->crtc[pipe] = &mtk_crtc->base;
+       priv->num_pipes++;
+
+       return 0;
+
+unprepare:
+       while (--i >= 0)
+               clk_unprepare(mtk_crtc->ddp_comp[i]->clk);
+
+       return ret;
+}

b/drivers/gpu/drm/mediatek/mtk_drm_drv.c

[snip]

+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()?

+
+       .get_vblank_counter = drm_vblank_count,
+       .enable_vblank = mtk_drm_crtc_enable_vblank,
+       .disable_vblank = mtk_drm_crtc_disable_vblank,
+
+       .gem_free_object = mtk_drm_gem_free_object,
+       .gem_vm_ops = &mtk_drm_gem_vm_ops,
+       .dumb_create = mtk_drm_gem_dumb_create,
+       .dumb_map_offset = mtk_drm_gem_dumb_map_offset,
+       .dumb_destroy = drm_gem_dumb_destroy,
+
+       .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
+       .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
+       .gem_prime_export = drm_gem_prime_export,
+       .gem_prime_import = drm_gem_prime_import,
+       .gem_prime_get_sg_table = mtk_gem_prime_get_sg_table,
+       .gem_prime_mmap = mtk_drm_gem_mmap_buf,
+       .fops = &mtk_drm_fops,
+
+       .name = DRIVER_NAME,
+       .desc = DRIVER_DESC,
+       .date = DRIVER_DATE,
+       .major = DRIVER_MAJOR,
+       .minor = DRIVER_MINOR,
+};
+
+static int compare_of(struct device *dev, void *data)
+{
+       return dev->of_node == data;
+}
+
+static int mtk_drm_bind(struct device *dev)
+{
+       struct mtk_drm_private *private = dev_get_drvdata(dev);
+       struct drm_device *drm;
+       int ret;
+
+       drm = drm_dev_alloc(&mtk_drm_driver, dev);
+       if (!drm)
+               return -ENOMEM;
+
+       drm_dev_set_unique(drm, dev_name(dev));
+
+       ret = drm_dev_register(drm, 0);
+       if (ret < 0)
+               goto err_free;
+
+       drm->dev_private = private;
+       private->drm = drm;
+
+       ret = mtk_drm_kms_init(drm);
+       if (ret < 0)
+               goto err_unregister;
+
+       return 0;
+
+err_unregister:
+       drm_dev_unregister(drm);
+err_free:
+       drm_dev_unref(drm);
+       return ret;
+}