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 [off-list ref] wrote:

quoted

+

 > +       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.

quoted

+       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.

quoted

+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
...

quoted

+       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.

quoted

+       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 ?

quoted

+static int zx_hdmi_bind(struct device *dev, struct device *master, void *data)
+{

quoted

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

quoted

+       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.

quoted

+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