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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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__ */
