Re: [PATCH 00/49] DRM driver for Hikey 970

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Thu, 20 Aug 2020 16:48:08 +0200
Sam Ravnborg <sam@ravnborg.org> 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 <sam@ravnborg.org> 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 <mchehab+huawei@kernel.org>

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 <drm/drm_of.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
+#include <drm/drm_managed.h>
 
 #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__ */