From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753354AbbDBSHZ (ORCPT ); Thu, 2 Apr 2015 14:07:25 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:53704 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752555AbbDBSHX (ORCPT ); Thu, 2 Apr 2015 14:07:23 -0400 Message-ID: <3b757ab0549f700dafb87abb2cda3ac5.squirrel@www.codeaurora.org> In-Reply-To: References: <1427922770-13721-1-git-send-email-jilaiw@codeaurora.org> Date: Thu, 2 Apr 2015 18:07:22 -0000 Subject: Re: [PATCH 2/3] drm:msm: Initial Add Writeback Support From: jilaiw@codeaurora.org To: "Emil Velikov" Cc: "Jilai Wang" , "ML dri-devel" , "linux-arm-msm" , "Linux-Kernel@Vger. Kernel. Org" User-Agent: SquirrelMail/1.4.22-4.el6 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT X-Priority: 3 (Normal) Importance: Normal Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thanks Emil. Please check the comments embedded and for the rest I will update the code. > 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 ? If the devices have WB connector, it will be added in the device tree. > > >> +++ 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 ? Since the devm_kzalloc function is used here, the system should take care freeing the memory. > > >> +++ 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 >