Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752597AbbDBLsG (ORCPT ); Thu, 2 Apr 2015 07:48:06 -0400 Received: from mail-yh0-f52.google.com ([209.85.213.52]:34366 "EHLO mail-yh0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750771AbbDBLsC (ORCPT ); Thu, 2 Apr 2015 07:48:02 -0400 MIME-Version: 1.0 In-Reply-To: <1427922770-13721-1-git-send-email-jilaiw@codeaurora.org> References: <1427922770-13721-1-git-send-email-jilaiw@codeaurora.org> Date: Thu, 2 Apr 2015 12:48:01 +0100 Message-ID: Subject: Re: [PATCH 2/3] drm:msm: Initial Add Writeback Support From: Emil Velikov To: Jilai Wang Cc: ML dri-devel , linux-arm-msm , "Linux-Kernel@Vger. Kernel. Org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5607 Lines: 187 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/