Re: [PATCH v2 2/2] drm: zte: add initial vou drm driver

[Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

Hi Emil,

On Fri, Sep 30, 2016 at 01:34:14PM +0100, Emil Velikov wrote:
> Hi Shawn,
> 
> A couple of fly-by suggestions, which I hope you'll find useful :-)

Thanks for the suggestions.

> On 24 September 2016 at 15:26, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 
> > +
>  > +       val = ((vm.hsync_len - 1) << SYNC_WIDE_SHIFT) & SYNC_WIDE_MASK;
> To save some writing/minimise the chances to typos getting, in you can
> have single/collection to static inline functions similar to msm [1].
> On a similar note inline wrappers zte_read/write/mask (around
> writel/readl) will provide quite useful for debugging/tracing :-)
> 
> [1] drivers/gpu/drm/msm/adreno/a4xx.xml.h

I would not add a header file with hundreds or thousands of defines
while only tens of them are actually used.  For debugging, I prefer
to print particular registers than every single read/write, which
might not be easy to check what I want to check.

> > +       if (IS_ERR(zcrtc->pixclk))
> > +               return ERR_PTR(PTR_ERR(zcrtc->pixclk));
> You might want to s/ERR_PTR(PTR_ERR// or s/ERR_PTR(PTR_ERR/ERR_CAST/
> through the patch.

Aha, yes.

> > +static int zx_drm_bind(struct device *dev)
> > +{
> > +       struct drm_device *drm;
> > +       struct zx_drm_private *priv;
> > +       int ret;
> > +
> > +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       drm = drm_dev_alloc(&zx_drm_driver, dev);
> > +       if (!drm)
> > +               return -ENOMEM;
> > +
> > +       drm->dev_private = priv;
> > +       dev_set_drvdata(dev, drm);
> > +
> > +       drm_mode_config_init(drm);
> > +       drm->mode_config.min_width = 16;
> > +       drm->mode_config.min_height = 16;
> > +       drm->mode_config.max_width = 4096;
> > +       drm->mode_config.max_height = 4096;
> > +       drm->mode_config.funcs = &zx_drm_mode_config_funcs;
> > +
> > +       ret = drm_dev_register(drm, 0);
> The documentation states that drm_dev_register() should be called
> after the hardware is setup. which some drivers seems to interpret as
> ...
> 
> > +       if (ret)
> > +               goto out_free;
> > +
> > +       ret = component_bind_all(dev, drm);
> > +       if (ret) {
> > +               DRM_ERROR("Failed to bind all components\n");
> > +               goto out_unregister;
> > +       }
> > +
> > +       ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
> > +       if (ret < 0) {
> > +               DRM_ERROR("failed to initialise vblank\n");
> > +               goto out_unbind;
> > +       }
> > +
> > +       /*
> > +        * We will manage irq handler on our own.  In this case, irq_enabled
> > +        * need to be true for using vblank core support.
> > +        */
> > +       drm->irq_enabled = true;
> > +
> > +       drm_mode_config_reset(drm);
> > +       drm_kms_helper_poll_init(drm);
> > +
> > +       priv->fbdev = drm_fbdev_cma_init(drm, 32, drm->mode_config.num_crtc,
> > +                                        drm->mode_config.num_connector);
> ... calling it after these. If that's the correct case - perhaps we
> can throw a WARN or similar within the above functions to catch
> present/future misuse ?

Yes.  Daniel also pointed it out in his review of the patch.

> > +       if (IS_ERR(priv->fbdev)) {
> > +               ret = PTR_ERR(priv->fbdev);
> > +               priv->fbdev = NULL;
> > +               goto out_fini;
> > +       }
> > +
> > +       return 0;
> > +
> > +out_fini:
> > +       drm_kms_helper_poll_fini(drm);
> > +       drm_mode_config_cleanup(drm);
> > +       drm_vblank_cleanup(drm);
> > +out_unbind:
> > +       component_unbind_all(dev, drm);
> > +out_unregister:
> > +       drm_dev_unregister(drm);
> > +out_free:
> > +       dev_set_drvdata(dev, NULL);
> > +       drm_dev_unref(drm);
> > +       return ret;
> > +}
> > +
> > +static void zx_drm_unbind(struct device *dev)
> > +{
> > +       struct drm_device *drm = dev_get_drvdata(dev);
> > +       struct zx_drm_private *priv = drm->dev_private;
> > +
> > +       if (priv->fbdev) {
> > +               drm_fbdev_cma_fini(priv->fbdev);
> > +               priv->fbdev = NULL;
> > +       }
> > +       drm_kms_helper_poll_fini(drm);
> > +       component_unbind_all(dev, drm);
> > +       drm_vblank_cleanup(drm);
> > +       drm_mode_config_cleanup(drm);
> > +       drm_dev_unregister(drm);
> > +       drm_dev_unref(drm);
> > +       drm->dev_private = NULL;
> > +       dev_set_drvdata(dev, NULL);
> This and the teardown path in bind() are asymmetrical. Furthermore you
> want to call drm_dev_unregister() first, according to its
> documentation.
> As mentioned above - perhaps it's worth providing feedback for drivers
> which get the order wrong ?
> 
> 
> 
> > +static int zx_hdmi_bind(struct device *dev, struct device *master, void *data)
> > +{
> 
> 
> > +
> > +       clk_prepare_enable(hdmi->cec_clk);
> > +       clk_prepare_enable(hdmi->osc_clk);
> > +       clk_prepare_enable(hdmi->xclk);
> > +
> > +       ret = zx_hdmi_register(drm, hdmi);
> > +       if (ret)
> > +               return ret;
> > +
> 
> > +       return 0;
> > +}
> > +
> > +static void zx_hdmi_unbind(struct device *dev, struct device *master,
> > +                          void *data)
> > +{
> > +       struct zx_hdmi *hdmi = dev_get_drvdata(dev);
> > +
> > +       clk_disable_unprepare(hdmi->cec_clk);
> > +       clk_disable_unprepare(hdmi->osc_clk);
> > +       clk_disable_unprepare(hdmi->xclk);
> Nit: you want the teardown to happen in reverse order of the setup. I
> might have missed a few similar cases within the patch, so please
> double-check.

Okay, I will give it a check through the patch.

> > +static int zx_gl_get_fmt(uint32_t format)
> > +{
> > +       switch (format) {
> > +       case DRM_FORMAT_ARGB8888:
> > +       case DRM_FORMAT_XRGB8888:
> > +               return GL_FMT_ARGB8888;
> > +       case DRM_FORMAT_RGB888:
> > +               return GL_FMT_RGB888;
> > +       case DRM_FORMAT_RGB565:
> > +               return GL_FMT_RGB565;
> > +       case DRM_FORMAT_ARGB1555:
> > +               return GL_FMT_ARGB1555;
> > +       case DRM_FORMAT_ARGB4444:
> > +               return GL_FMT_ARGB4444;
> > +       default:
> > +               WARN_ONCE(1, "invalid pixel format %d\n", format);
> > +               return -EINVAL;
> Afaics the only user of this is atomic_update() and that function
> cannot fail. You might want to move this to the _check() function.
> Same logic goes for the rest of the driver, in case I've missed any.

The function does the conversion from DRM format values to the ones that
hardware accepts.  And I need to set up hardware register with the
converted value.

I suppose that the error case in 'default' should never be hit, since
all valid format have been reported to DRM core by gl_formats?  In that
case, I can simply drop the WARN and return a sane default format
instead?

Shawn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html