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=-6.8 required=3.0 tests=BAYES_00, 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 B9655C433E3 for ; Mon, 24 Aug 2020 21:24:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 82F4720706 for ; Mon, 24 Aug 2020 21:24:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727073AbgHXVY4 (ORCPT ); Mon, 24 Aug 2020 17:24:56 -0400 Received: from asavdk3.altibox.net ([109.247.116.14]:56188 "EHLO asavdk3.altibox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726519AbgHXVY4 (ORCPT ); Mon, 24 Aug 2020 17:24:56 -0400 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 Cc: Greg Kroah-Hartman , 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 , 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 , Jakub Kicinski , mauro.chehab@huawei.com, Rob Clark , linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Liuyao An , Rongrong Zou , bpf@vger.kernel.org, "David S. Miller" 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-Type: text/plain; charset=us-ascii 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 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.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