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

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