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

[Daniel Kurtz <djkurtz@chromium.org>]

[-- Attachment #1.1: Type: text/plain, Size: 6685 bytes --]

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 <p.zabel@pengutronix.de>
wrote:
> From: CK Hu <ck.hu@mediatek.com>
>
> 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 <ck.hu@mediatek.com>
> Signed-off-by: YT Shen <yt.shen@mediatek.com>
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> 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;
> +}

> 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

[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;
> +}

[-- Attachment #1.2: Type: text/html, Size: 14208 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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