From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EF321C433DF for ; Mon, 24 Aug 2020 21:26:49 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9F8182072D for ; Mon, 24 Aug 2020 21:26:49 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="SXTkS3fS" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9F8182072D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ravnborg.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=t71xYDVvUJu/RSEcmE9/NUq2A39BJ/f4iy0q2qmycLc=; b=SXTkS3fSDm7WyNYOLAKjRqv1q NO6JML+gZsS8N1NMDRSJ5l+Pu6luJW6eXypAqX1ALpv9VQ8KqYZTTqwbGHzgkpkXWaLUxqnP/nStJ rbsioQ68qJt6w5NAsUYDmLuVoNfRNt8p4lESTI2zcqojpYrDefSmqxARhJRB8bXiYdvK1rMJ+1HnE aRLPFZcVx/4b4Us9Y0GzJ2Z58JxJ1pUUjLZLaPtQGgnvfayWbcBWhyGmsCVk/yq0WVlT+Gw3Z/Amr SeWAX6OK4UXVAhIHvCsPR2i6eWs6NZGpT2eAQs6gkkJMH4UNFaXCu63teX9x4b7m/Dmm451pvOMTZ JLZPrnvDA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kAJxP-0006vD-5Q; Mon, 24 Aug 2020 21:24:47 +0000 Received: from asavdk3.altibox.net ([109.247.116.14]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kAJxD-0006rn-IU for linux-arm-kernel@lists.infradead.org; Mon, 24 Aug 2020 21:24:41 +0000 Received: from ravnborg.org (unknown [188.228.123.71]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by asavdk3.altibox.net (Postfix) with ESMTPS id 5C13420024; Mon, 24 Aug 2020 23:24:30 +0200 (CEST) Date: Mon, 24 Aug 2020 23:24:29 +0200 From: Sam Ravnborg To: Mauro Carvalho Chehab Subject: Re: [PATCH 00/49] DRM driver for Hikey 970 Message-ID: <20200824212429.GA106550@ravnborg.org> References: <20200819152120.GA106437@ravnborg.org> <20200819174027.70b39ee9@coco.lan> <20200819173558.GA3733@ravnborg.org> <20200821155801.0b820fc6@coco.lan> <20200821155505.GA300361@ravnborg.org> <20200824180225.1a515b6a@coco.lan> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200824180225.1a515b6a@coco.lan> X-CMAE-Score: 0 X-CMAE-Analysis: v=2.3 cv=f+hm+t6M c=1 sm=1 tr=0 a=S6zTFyMACwkrwXSdXUNehg==:117 a=S6zTFyMACwkrwXSdXUNehg==:17 a=kj9zAlcOel0A:10 a=BTeA3XvPAAAA:8 a=KKAkSRfTAAAA:8 a=dfO-HtcW0KOu99fHjzcA:9 a=whV8WjJfvz2m6cgv:21 a=YRhTcNNlbVQiQlz6:21 a=3cydBBslwyRf96m4:21 a=CjuIK1q_8ugA:10 a=tafbbOV3vt1XuEhzTjGK:22 a=cvBusfyB2V15izCimMoJ:22 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200824_172436_325134_B75D165E X-CRM114-Status: GOOD ( 31.94 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Neil Armstrong , Xinliang Liu , Wanchun Zheng , linuxarm@huawei.com, dri-devel , Andrzej Hajda , Laurent Pinchart , devel@driverdev.osuosl.org, Daniel Borkmann , John Fastabend , Xiubin Zhang , Wei Xu , David Airlie , Xinwei Kong , Tomi Valkeinen , Bogdan Togorean , Jakub Kicinski , Laurentiu Palcu , linux-media@vger.kernel.org, devicetree@vger.kernel.org, Liwei Cai , Jesper Dangaard Brouer , Manivannan Sadhasivam , Chen Feng , Alexei Starovoitov , linaro-mm-sig@lists.linaro.org, Rob Herring , mauro.chehab@huawei.com, Rob Clark , linux-arm-kernel@lists.infradead.org, Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Liuyao An , netdev@vger.kernel.org, Rongrong Zou , bpf@vger.kernel.org, "David S. Miller" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Mauro > Before posting the big patch series again, let me send the new > version folded into a single patch. Review 2/N The way output_poll_changed is used to set gpio_mux to select between the panel and the HDMI looks strange. But I do not know if there is a more correct way to do it. Other DRM people would need to help here if there is a better way to do it. I looked briefly af suspend/resume. I think this would benefit from using drm_mode_config_helper_suspend() and drm_mode_config_helper_resume() but I did not finalize the anlyzis. Other than this only some small details in the following. Sam > kirin9xx_drm_drv.c b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c > new file mode 100644 > index 000000000000..61b65f8b1ace > --- /dev/null > +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c > @@ -0,0 +1,277 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Hisilicon Kirin SoCs drm master driver > + * > + * Copyright (c) 2016 Linaro Limited. > + * Copyright (c) 2014-2016 Hisilicon Limited. > + * Copyright (c) 2014-2020, Huawei Technologies Co., Ltd > + * Author: > + * > + * > + */ > + > +#include > +#include > +#include Sort includes > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Sort includes > + > +#include "kirin9xx_dpe.h" > +#include "kirin9xx_drm_drv.h" > + > +static int kirin_drm_kms_cleanup(struct drm_device *dev) > +{ > + struct kirin_drm_private *priv = to_drm_private(dev); > + > + if (priv->fbdev) > + priv->fbdev = NULL; > + > + drm_kms_helper_poll_fini(dev); > + kirin9xx_dss_drm_cleanup(dev); > + > + return 0; > +} > + > +static void kirin_fbdev_output_poll_changed(struct drm_device *dev) > +{ > + struct kirin_drm_private *priv = to_drm_private(dev); > + > + dsi_set_output_client(dev); > + > + drm_fb_helper_hotplug_event(priv->fbdev); > +} > + > +static const struct drm_mode_config_funcs kirin_drm_mode_config_funcs = { > + .fb_create = drm_gem_fb_create, > + .output_poll_changed = kirin_fbdev_output_poll_changed, > + .atomic_check = drm_atomic_helper_check, > + .atomic_commit = drm_atomic_helper_commit, > +}; > + > +static int kirin_drm_kms_init(struct drm_device *dev) > +{ > + long kirin_type; > + int ret; > + > + dev_set_drvdata(dev->dev, dev); > + > + ret = drmm_mode_config_init(dev); > + if (ret) > + return ret; > + > + dev->mode_config.min_width = 0; > + dev->mode_config.min_height = 0; > + dev->mode_config.max_width = 2048; > + dev->mode_config.max_height = 2048; > + dev->mode_config.preferred_depth = 32; > + > + dev->mode_config.funcs = &kirin_drm_mode_config_funcs; > + > + /* display controller init */ > + kirin_type = (long)of_device_get_match_data(dev->dev); > + if (WARN_ON(!kirin_type)) > + return -ENODEV; > + > + ret = dss_drm_init(dev, kirin_type); > + if (ret) > + return ret; > + > + /* bind and init sub drivers */ > + ret = component_bind_all(dev->dev, dev); > + if (ret) { > + drm_err(dev, "failed to bind all component.\n"); > + return ret; > + } > + > + /* vblank init */ > + ret = drm_vblank_init(dev, dev->mode_config.num_crtc); > + if (ret) { > + drm_err(dev, "failed to initialize vblank.\n"); > + return ret; > + } > + /* with irq_enabled = true, we can use the vblank feature. */ > + dev->irq_enabled = true; > + > + /* reset all the states of crtc/plane/encoder/connector */ > + drm_mode_config_reset(dev); > + > + /* init kms poll for handling hpd */ > + drm_kms_helper_poll_init(dev); > + > + return 0; > +} > + > +DEFINE_DRM_GEM_CMA_FOPS(kirin_drm_fops); Move it to right above kirin_drm_driver where it is used > + > +static int kirin_drm_connectors_register(struct drm_device *dev) > +{ > + struct drm_connector_list_iter conn_iter; > + struct drm_connector *failed_connector; > + struct drm_connector *connector; > + int ret; > + > + mutex_lock(&dev->mode_config.mutex); > + drm_connector_list_iter_begin(dev, &conn_iter); > + drm_for_each_connector_iter(connector, &conn_iter) { > + ret = drm_connector_register(connector); > + if (ret) { > + failed_connector = connector; > + goto err; > + } > + } > + mutex_unlock(&dev->mode_config.mutex); > + > + return 0; > + > +err: > + drm_connector_list_iter_begin(dev, &conn_iter); > + drm_for_each_connector_iter(connector, &conn_iter) { > + if (failed_connector == connector) > + break; > + drm_connector_unregister(connector); > + } > + mutex_unlock(&dev->mode_config.mutex); > + > + return ret; > +} > + > +static struct drm_driver kirin_drm_driver = { > + .driver_features = DRIVER_GEM | DRIVER_MODESET | > + DRIVER_ATOMIC | DRIVER_RENDER, > + > + .fops = &kirin_drm_fops, > + .name = "kirin9xx", > + .desc = "Hisilicon Kirin9xx SoCs' DRM Driver", > + .date = "20170309", > + .major = 1, > + .minor = 0, > + > + DRM_GEM_CMA_VMAP_DRIVER_OPS > +}; Looks good now. > + > + > +static int compare_of(struct device *dev, void *data) > +{ > + return dev->of_node == data; > +} > + > +static int kirin_drm_bind(struct device *dev) > +{ > + struct kirin_drm_private *priv; > + struct drm_device *drm; > + int ret; > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + drm = &priv->drm; > + > + ret = devm_drm_dev_init(dev, drm, &kirin_drm_driver); > + if (ret) { > + kfree(priv); > + return ret; > + } > + drmm_add_final_kfree(drm, priv); > + > + ret = kirin_drm_kms_init(drm); > + if (ret) > + return ret; > + > + ret = drm_dev_register(drm, 0); > + if (ret) > + return ret; > + > + drm_fbdev_generic_setup(drm, 0); Should be last - after connector register. > + > + /* connectors should be registered after drm device register */ > + ret = kirin_drm_connectors_register(drm); > + if (ret) > + goto err_drm_dev_unregister; I am rather sure registering connectors are already taken care of by drm_dev_register(). The driver set DRIVER_MODESET so drm_modeset_register_all() is called which again registers all connectors. > + > + return 0; > + > +err_drm_dev_unregister: > + drm_dev_unregister(drm); > + kirin_drm_kms_cleanup(drm); > + drm_dev_put(drm); Not needed when using drmm_* and embedded drm_device > + > + return ret; > +} > + > +static void kirin_drm_unbind(struct device *dev) > +{ > + struct drm_device *drm_dev = dev_get_drvdata(dev); > + > + drm_dev_unregister(drm_dev); > + drm_atomic_helper_shutdown(drm_dev); > + kirin_drm_kms_cleanup(drm_dev); > + drm_dev_put(drm_dev); Not needed when using drmm_* and embedded drm_device > +} > + > +static const struct component_master_ops kirin_drm_ops = { > + .bind = kirin_drm_bind, > + .unbind = kirin_drm_unbind, > +}; > + > +static int kirin_drm_platform_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct component_match *match = NULL; > + struct device_node *remote; > + > + remote = of_graph_get_remote_node(np, 0, 0); > + if (!remote) > + return -ENODEV; > + > + drm_of_component_match_add(dev, &match, compare_of, remote); > + of_node_put(remote); > + > + return component_master_add_with_match(dev, &kirin_drm_ops, match); > +} > + > +static int kirin_drm_platform_remove(struct platform_device *pdev) > +{ > + component_master_del(&pdev->dev, &kirin_drm_ops); > + return 0; > +} > + > +static const struct of_device_id kirin_drm_dt_ids[] = { > + { .compatible = "hisilicon,hi3660-dpe", > + .data = (void *)FB_ACCEL_HI366x, > + }, > + { .compatible = "hisilicon,kirin970-dpe", > + .data = (void *)FB_ACCEL_KIRIN970, > + }, > + { /* end node */ }, > +}; > +MODULE_DEVICE_TABLE(of, kirin_drm_dt_ids); > + > +static struct platform_driver kirin_drm_platform_driver = { > + .probe = kirin_drm_platform_probe, > + .remove = kirin_drm_platform_remove, > + .suspend = kirin9xx_dss_drm_suspend, > + .resume = kirin9xx_dss_drm_resume, > + .driver = { > + .name = "kirin9xx-drm", > + .of_match_table = kirin_drm_dt_ids, > + }, > +}; > + > +module_platform_driver(kirin_drm_platform_driver); > + > +MODULE_AUTHOR("cailiwei "); > +MODULE_AUTHOR("zhengwanchun "); > +MODULE_DESCRIPTION("hisilicon Kirin SoCs' DRM master driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.h b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.h > new file mode 100644 > index 000000000000..9bfcb35d6fa5 > --- /dev/null > +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.h > @@ -0,0 +1,42 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2016 Linaro Limited. > + * Copyright (c) 2014-2016 Hisilicon Limited. > + * Copyright (c) 2014-2020, Huawei Technologies Co., Ltd > + */ > + > +#ifndef __KIRIN_DRM_DRV_H__ > +#define __KIRIN_DRM_DRV_H__ > + > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#define MAX_CRTC 2 > + > +struct kirin_drm_private { > + struct drm_device drm; Hmm, hare we have drm_device embedded - so some comments from previous review can be ignored - ups. > + struct drm_fb_helper *fbdev; Never assigned - can be deleted. kirin_fbdev_output_poll_changed() needs to be re-visited as it assumes fbdev is assigned. > + struct drm_crtc *crtc[MAX_CRTC]; > +}; > + > +struct kirin_fbdev { > + struct drm_fb_helper fb_helper; > + struct drm_framebuffer *fb; > +}; struct kirin_fbdev is unused - delete. > + > +/* provided by kirin9xx_drm_dss.c */ > +void kirin9xx_dss_drm_cleanup(struct drm_device *dev); > +int kirin9xx_dss_drm_suspend(struct platform_device *pdev, pm_message_t state); > +int kirin9xx_dss_drm_resume(struct platform_device *pdev); > +int dss_drm_init(struct drm_device *dev, u32 g_dss_version_tag); > + > +void dsi_set_output_client(struct drm_device *dev); > + > +#define to_drm_private(d) container_of(d, struct kirin_drm_private, drm) > + > +#endif /* __KIRIN_DRM_DRV_H__ */ > diff --git a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_dss.c b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_dss.c > new file mode 100644 > index 000000000000..ff93df229868 > --- /dev/null > +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_dss.c > @@ -0,0 +1,979 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Hisilicon Hi6220 SoC ADE(Advanced Display Engine)'s crtc&plane driver > + * > + * Copyright (c) 2016 Linaro Limited. > + * Copyright (c) 2014-2016 Hisilicon Limited. > + * Copyright (c) 2014-2020, Huawei Technologies Co., Ltd > + * > + * Author: > + * Xinliang Liu > + * Xinliang Liu > + * Xinwei Kong > + */ > + > +#include > +#include > +#include