Re: [PATCH 2/3] drm:msm: Initial Add Writeback Support

[Emil Velikov <emil.l.velikov@gmail.com>]

Hi Jilai,

Just a few questions, not really a review as I'm not that familiar
with the code.


> +++ b/drivers/gpu/drm/msm/Kconfig
> @@ -27,6 +27,16 @@ config DRM_MSM_FBDEV
>           support. Note that this support also provide the linux console
>           support on top of the MSM modesetting driver.
>
> +config DRM_MSM_WB
> +       bool "Enable writeback support for MSM modesetting driver"
> +       depends on DRM_MSM
> +       depends on VIDEO_V4L2
> +       select VIDEOBUF2_CORE
> +       default y
> +       help
> +         Choose this option if you have a need to support writeback
> +         connector.
> +
Is it worth mentioning which devices/SoCs have such connector ?


> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -1,4 +1,5 @@
> -ccflags-y := -Iinclude/drm -Idrivers/gpu/drm/msm
> +ccflags-y := -Iinclude/drm -Idrivers/gpu/drm/msm -Idrivers/gpu/drm/msm/mdp_wb
> +ccflags-$(CONFIG_DRM_MSM_WB) += -Idrivers/gpu/drm/msm/mdp/mdp_wb
>
I think you only want the second line here.


> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_wb_encoder.c

> +static struct msm_bus_paths mdp_bus_usecases[] = { {
> +               .num_paths = 1,
> +               .vectors = &mdp_bus_vectors[0],
> +}, {
> +               .num_paths = 1,
> +               .vectors = &mdp_bus_vectors[1],
> +} };
The following formatting seems more common:

static struct foo foo1[] = {
    {
        bar
     }
};


> +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb.c

> +int msm_wb_modeset_init(struct msm_wb *wb,
> +       struct drm_device *dev, struct drm_encoder *encoder)
> +{
> +       struct msm_drm_private *priv = dev->dev_private;
> +       int ret;
> +
> +       wb->dev = dev;
> +       wb->encoder = encoder;
> +
> +       wb->connector = msm_wb_connector_init(wb);
> +       if (IS_ERR(wb->connector)) {
> +               ret = PTR_ERR(wb->connector);
> +               dev_err(dev->dev, "failed to create WB connector: %d\n", ret);
> +               wb->connector = NULL;
> +               goto fail;
> +       }
> +
> +       priv->connectors[priv->num_connectors++] = wb->connector;
> +
> +       return 0;
> +
> +fail:
> +       if (wb->connector) {
> +               wb->connector->funcs->destroy(wb->connector);
> +               wb->connector = NULL;
> +       }
> +
Drop the unused if block ?


> +static struct msm_wb *msm_wb_init(struct platform_device *pdev)
> +{
> +       struct msm_wb *wb = NULL;
> +
> +       wb = devm_kzalloc(&pdev->dev, sizeof(*wb), GFP_KERNEL);
> +       if (!wb)
> +               return ERR_PTR(-ENOMEM);
> +
> +       wb->pdev = pdev;
> +       wb->priv_data = devm_kzalloc(&pdev->dev, sizeof(*wb->priv_data),
> +               GFP_KERNEL);
> +       if (!wb->priv_data)
> +               return ERR_PTR(-ENOMEM);
> +
> +       if (msm_wb_v4l2_init(wb)) {
> +               pr_err("%s: wb v4l2 init failed\n", __func__);
> +               return ERR_PTR(-ENODEV);
> +       }
> +
Seems like we're leaking wb and/or wb->priv_data. Add a label and
consolidate error handling in there ?


> +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb.h

> +#ifndef __WB_CONNECTOR_H__
> +#define __WB_CONNECTOR_H__
> +
The file is called mdp_wb.h, so one might want to change this to __MDP_WB_H__


> --- /dev/null
> +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb_v4l2.c
> @@ -0,0 +1,522 @@

> +static const struct msm_wb_fmt *get_format(u32 fourcc)
> +{
> +       const struct msm_wb_fmt *fmt;
> +       unsigned int k;
> +
> +       for (k = 0; k < ARRAY_SIZE(formats); k++) {
> +               fmt = &formats[k];
> +               if (fmt->fourcc == fourcc)
> +                       break;
> +       }
> +
> +       if (k == ARRAY_SIZE(formats))
> +               return NULL;
> +
> +       return &formats[k];
You could move the return within the loop, and drop the follow up conditional.


> +static void *msm_wb_vb2_attach_dmabuf(void *alloc_ctx, struct dma_buf *dbuf,
> +       unsigned long size, int write)
> +{
> +       struct msm_wb_v4l2_dev *dev = alloc_ctx;
> +       struct drm_device *drm_dev = dev->wb->dev;
> +       struct drm_gem_object *obj;
> +
> +       mutex_lock(&drm_dev->object_name_lock);
> +       obj = drm_dev->driver->gem_prime_import(drm_dev, dbuf);
> +       if (WARN_ON(!obj)) {
> +               mutex_unlock(&drm_dev->object_name_lock);
> +               v4l2_err(&dev->v4l2_dev, "Can't convert dmabuf to gem obj.\n");
> +               return NULL;
Shouldn't one return ERR_PTR here ? Consolidating the error paths to a
single label will be cleaner imho.


> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h

> @@ -224,18 +229,28 @@ struct drm_framebuffer *msm_framebuffer_create(struct drm_device *dev,
>
>  struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev);
>
> -struct hdmi;
>  int hdmi_modeset_init(struct hdmi *hdmi, struct drm_device *dev,
>                 struct drm_encoder *encoder);
>  void __init hdmi_register(void);
>  void __exit hdmi_unregister(void);
>
> -struct msm_edp;
Unrelated cosmetic changes ?


> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -534,6 +534,7 @@ void msm_gem_free_object(struct drm_gem_object *obj)
>                 if (msm_obj->pages)
>                         drm_free_large(msm_obj->pages);
>
> +               drm_prime_gem_destroy(obj, msm_obj->sgt);
Should this fix be a separate patch ?


Cheers,
Emil