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

Hi Mauro.

Quick feedback below.

Sam

On Thu, Aug 20, 2020 at 05:13:22PM +0200, Mauro Carvalho Chehab wrote:

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.

From the documentation of drm_gem_cma_dumb_create_internal:

* It should not be used directly * as their &drm_driver.dumb_create callback.

I would expect drm_gem_cma_dumb_create() to be the right choice. So you can go ahead with DRM_GEM_CMA_VMAP_DRIVER_OPS

(I hope I am right, not the are i know much about)

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);

This should also be gone when you are using drmm_mode_config_init()

• 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);

It is assigned a few lines later.

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;

OK, I am confused here. This code allocates a struct kirin_drm_private. But the calling function does the same. What am I missing here? Coffee?

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);

I think this is not needed. But the component framework confuses me.

• drm_atomic_helper_shutdown(drm_dev); kirin_drm_kms_cleanup(drm_dev); drm_dev_put(drm_dev);

This should likewise go I think.

} 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__ */