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=-11.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable 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 A8AE2C433E1 for ; Thu, 20 Aug 2020 15:16:23 +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 6A7B1206DA for ; Thu, 20 Aug 2020 15:16:23 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="XfPdM4H/"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="O4GFm5ix" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6A7B1206DA Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.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:MIME-Version:References:In-Reply-To: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=jZa9VxcYapi+1jdZjb+Xi80pq+7Wz2abVHCS08j/EmA=; b=XfPdM4H/ACbEbK5K3ekQMTNcW szNmWz/Y6BGIq4Ybmy0rIQuloJ4vuoyCYRsJ90pE0SvbtEW522hVhcz5L/EwNxUWhp4NfsG5+XJ6k InmmFMuneKPJ+AhI4ZVygS7Zm1+ypvx+uweL2KJJYsUW3w16efm+KvQvC+q6pWY5J/KMEsLrQ8ccf wbgLlFlQwBykcCiTyN3eaqZnRfPGFhbhg0U26PD6vQQyO1hGfKs8nJzBC7sPPH+Ib0P0Wo1O0yEfq jvPHBntd/oURGneY1nDsEavAMqqkPvdYc/PTxyc1gfN6MW3NvOVhw6RJlHtynw9C0X8m+OjnZeGbd yfz2ute4Q==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k8mGs-0005q4-4u; Thu, 20 Aug 2020 15:14:30 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1k8mFy-0005Bd-Sf for linux-arm-kernel@lists.infradead.org; Thu, 20 Aug 2020 15:13:36 +0000 Received: from coco.lan (ip5f5ad5a3.dynamic.kabel-deutschland.de [95.90.213.163]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id F17F3204EA; Thu, 20 Aug 2020 15:13:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1597936412; bh=/zCmwwU3sNcC0M45eP2ex87GldoS8TY7gBwIB4VYX1k=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=O4GFm5ixVVaWyhs64lYh5h2H5C06AZWeb4JaJGGG5ZefL9OEBdKDduar/r8aXYNZv gaeKj0O1fvqmxIlIbL11aPqsnRDDqFgSm3pzk8aIty8/L9QYDWAWbrKGhUyD6AdDnm ciVVz9EXHGXzvMby02yPYXLJsmDwRsrYhh+Zr3rQ= Date: Thu, 20 Aug 2020 17:13:22 +0200 From: Mauro Carvalho Chehab To: Sam Ravnborg Subject: Re: [PATCH 00/49] DRM driver for Hikey 970 Message-ID: <20200820171322.3b2e94fd@coco.lan> In-Reply-To: <20200820144808.GA186324@ravnborg.org> References: <20200819152120.GA106437@ravnborg.org> <20200819174027.70b39ee9@coco.lan> <20200819173558.GA3733@ravnborg.org> <20200820160649.54741194@coco.lan> <20200820144808.GA186324@ravnborg.org> X-Mailer: Claws Mail 3.17.5 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200820_111335_308053_3302BEC5 X-CRM114-Status: GOOD ( 37.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 Em Thu, 20 Aug 2020 16:48:08 +0200 Sam Ravnborg escreveu: > Hi Mauro. > > On Thu, Aug 20, 2020 at 04:06:49PM +0200, Mauro Carvalho Chehab wrote: > > Em Wed, 19 Aug 2020 19:35:58 +0200 > > Sam Ravnborg escreveu: > > > > I'm already handling the other comments from your review (I'll send a > > more complete comment about them after finishing), > If you get back only on things you do not understand or do not agree on > that would be fine. The rest should be visible in the changelog on the > updated patch - no need to do extra work here. > > > but I have a doubt what you meant about this: > > > > > +static int kirin_drm_bind(struct device *dev) > > > > +{ > > > > + struct drm_driver *driver = &kirin_drm_driver; > > > > + struct drm_device *drm_dev; > > > > + struct kirin_drm_private *priv; > > > > + int ret; > > > > + > > > > + drm_dev = drm_dev_alloc(driver, dev); > > > > + if (!drm_dev) > > > > + return -ENOMEM; > > > > + > > > > + ret = kirin_drm_kms_init(drm_dev); > > > > + if (ret) > > > > + goto err_drm_dev_unref; > > > > + > > > > + ret = drm_dev_register(drm_dev, 0); > > > There is better ways to do this. See drm_drv.c for the code example. > > > > Not sure if I understood your comment here. The drm_drv.c example also calls > > drm_dev_register(). > > This is indeed not obvious from my comments but what I wnated to say is > that the driver should embed drm_device in some struct, > maybe in "struct kirin_drm_private". > > This should also be part of the referenced example. > > I hope this clarifies it. Yeah. I was already doing those changes ;-) Something like the enclosed patch, right? Btw, I'm not sure if the error handling part is ok, as I didn't check what the devm stuff does at the subsystem. - On a separate question, I was unable to use the helper macros, as it sounds that there's no macro with this: .dumb_create = drm_gem_cma_dumb_create_internal, The existing DRM_GEM_CMA_VMAP_DRIVER_OPS uses, instead drm_gem_cma_dumb_create(). I'm not sure if this driver can use such function instead. Thanks, Mauro staging: hikey9xx/gpu: use drm_managed interface Use a more modern design for the driver binding logic by using drm_managed and getting rid of drm->dev_private. Signed-off-by: Mauro Carvalho Chehab diff --git a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c index c7736f4d74b7..600c89605cc0 100644 --- a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c @@ -29,12 +29,13 @@ #include #include #include +#include #include "kirin9xx_drm_drv.h" static int kirin_drm_kms_cleanup(struct drm_device *dev) { - struct kirin_drm_private *priv = dev->dev_private; + struct kirin_drm_private *priv = to_drm_private(dev); static struct kirin_dc_ops const *dc_ops; if (priv->fbdev) @@ -45,15 +46,13 @@ static int kirin_drm_kms_cleanup(struct drm_device *dev) drm_kms_helper_poll_fini(dev); dc_ops->cleanup(dev); drm_mode_config_cleanup(dev); - devm_kfree(dev->dev, priv); - dev->dev_private = NULL; return 0; } static void kirin_fbdev_output_poll_changed(struct drm_device *dev) { - struct kirin_drm_private *priv = dev->dev_private; + struct kirin_drm_private *priv = to_drm_private(dev); dsi_set_output_client(dev); @@ -69,18 +68,20 @@ static const struct drm_mode_config_funcs kirin_drm_mode_config_funcs = { static int kirin_drm_kms_init(struct drm_device *dev) { - struct kirin_drm_private *priv = dev->dev_private; + struct kirin_drm_private *priv = to_drm_private(dev); static struct kirin_dc_ops const *dc_ops; int ret; - priv = devm_kzalloc(dev->dev, sizeof(*priv), GFP_KERNEL); + priv = drmm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; dev->dev_private = priv; dev_set_drvdata(dev->dev, dev); - drm_mode_config_init(dev); + ret = drmm_mode_config_init(dev); + if (ret) + return ret; dev->mode_config.min_width = 0; dev->mode_config.min_height = 0; @@ -94,20 +95,20 @@ static int kirin_drm_kms_init(struct drm_device *dev) dc_ops = of_device_get_match_data(dev->dev); ret = dc_ops->init(dev); if (ret) - goto err_mode_config_cleanup; + return ret; /* bind and init sub drivers */ ret = component_bind_all(dev->dev, dev); if (ret) { DRM_ERROR("failed to bind all component.\n"); - goto err_dc_cleanup; + return ret; } /* vblank init */ ret = drm_vblank_init(dev, dev->mode_config.num_crtc); if (ret) { DRM_ERROR("failed to initialize vblank.\n"); - goto err_unbind_all; + return ret; } /* with irq_enabled = true, we can use the vblank feature. */ dev->irq_enabled = true; @@ -119,28 +120,10 @@ static int kirin_drm_kms_init(struct drm_device *dev) drm_kms_helper_poll_init(dev); return 0; - -err_unbind_all: - component_unbind_all(dev->dev, dev); -err_dc_cleanup: - dc_ops->cleanup(dev); -err_mode_config_cleanup: - drm_mode_config_cleanup(dev); - devm_kfree(dev->dev, priv); - dev->dev_private = NULL; - - return ret; } DEFINE_DRM_GEM_CMA_FOPS(kirin_drm_fops); -static int kirin_gem_cma_dumb_create(struct drm_file *file, - struct drm_device *dev, - struct drm_mode_create_dumb *args) -{ - return drm_gem_cma_dumb_create_internal(file, dev, args); -} - static int kirin_drm_connectors_register(struct drm_device *dev) { struct drm_connector_list_iter conn_iter; @@ -176,11 +159,11 @@ static int kirin_drm_connectors_register(struct drm_device *dev) static struct drm_driver kirin_drm_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_RENDER, - .fops = &kirin_drm_fops, + .fops = &kirin_drm_fops, .gem_free_object = drm_gem_cma_free_object, .gem_vm_ops = &drm_gem_cma_vm_ops, - .dumb_create = kirin_gem_cma_dumb_create, + .dumb_create = drm_gem_cma_dumb_create_internal, .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, @@ -207,42 +190,48 @@ static int compare_of(struct device *dev, void *data) static int kirin_drm_bind(struct device *dev) { struct drm_driver *driver = &kirin_drm_driver; - struct drm_device *drm_dev; struct kirin_drm_private *priv; + struct drm_device *drm; int ret; - drm_dev = drm_dev_alloc(driver, dev); - if (!drm_dev) + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) return -ENOMEM; - ret = kirin_drm_kms_init(drm_dev); + drm = &priv->drm; + + ret = devm_drm_dev_init(dev, drm, driver); + if (ret) { + kfree(priv); + return ret; + } + drmm_add_final_kfree(drm, priv); + + ret = kirin_drm_kms_init(drm); if (ret) - goto err_drm_dev_unref; + return ret; - ret = drm_dev_register(drm_dev, 0); + ret = drm_dev_register(drm, 0); if (ret) - goto err_kms_cleanup; + return ret; - drm_fbdev_generic_setup(drm_dev, 0); - priv = drm_dev->dev_private; + drm_fbdev_generic_setup(drm, 0); /* connectors should be registered after drm device register */ - ret = kirin_drm_connectors_register(drm_dev); + ret = kirin_drm_connectors_register(drm); if (ret) goto err_drm_dev_unregister; DRM_INFO("Initialized %s %d.%d.%d %s on minor %d\n", driver->name, driver->major, driver->minor, driver->patchlevel, - driver->date, drm_dev->primary->index); + driver->date, drm->primary->index); return 0; err_drm_dev_unregister: - drm_dev_unregister(drm_dev); -err_kms_cleanup: - kirin_drm_kms_cleanup(drm_dev); -err_drm_dev_unref: - drm_dev_put(drm_dev); + drm_dev_unregister(drm); + kirin_drm_kms_cleanup(drm); + drm_dev_put(drm); return ret; } @@ -252,6 +241,7 @@ 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); } diff --git a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.h b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.h index 58f6fc7be347..09255d136c54 100644 --- a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.h +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.h @@ -31,6 +31,7 @@ struct kirin_dc_ops { }; struct kirin_drm_private { + struct drm_device drm; struct drm_fb_helper *fbdev; struct drm_crtc *crtc[MAX_CRTC]; }; @@ -44,4 +45,6 @@ extern const struct kirin_dc_ops kirin960_dss_dc_ops; extern const struct kirin_dc_ops kirin970_dss_dc_ops; 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__ */ _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel