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

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

On Sun, Sep 25, 2016 at 10:58:09PM +0200, Daniel Vetter wrote:
> On Sat, Sep 24, 2016 at 10:26:25PM +0800, Shawn Guo wrote:
> > It adds the initial ZTE VOU display controller DRM driver.  There are
> > still some features to be added, like overlay plane, scaling, and more
> > output devices support.  But it's already useful with dual CRTCs and
> > HDMI monitor working.
> > 
> > It's been tested on Debian Jessie LXDE desktop with modesetting driver.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> I've done a very quick read-through, looks real pretty. A few comments
> below and in-line.

Thanks much for looking at it.

> For testing, have you tried to run i-g-t validation tests per our
> documentation? See https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#validating-changes-with-igt

Sorry for my ignorance on that.  I'm on business travel right now, and
will give it a try once I get back to the hardware.

> >  drivers/gpu/drm/Kconfig          |   2 +
> >  drivers/gpu/drm/Makefile         |   1 +
> >  drivers/gpu/drm/zte/Kconfig      |   8 +
> >  drivers/gpu/drm/zte/Makefile     |   8 +
> >  drivers/gpu/drm/zte/zx_crtc.c    | 691 +++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/zte/zx_crtc.h    |  47 +++
> >  drivers/gpu/drm/zte/zx_drm_drv.c | 258 +++++++++++++++
> >  drivers/gpu/drm/zte/zx_drm_drv.h |  22 ++
> >  drivers/gpu/drm/zte/zx_hdmi.c    | 540 ++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/zte/zx_plane.c   | 362 ++++++++++++++++++++
> >  drivers/gpu/drm/zte/zx_plane.h   |  26 ++
> >  11 files changed, 1965 insertions(+)
> >  create mode 100644 drivers/gpu/drm/zte/Kconfig
> >  create mode 100644 drivers/gpu/drm/zte/Makefile
> >  create mode 100644 drivers/gpu/drm/zte/zx_crtc.c
> >  create mode 100644 drivers/gpu/drm/zte/zx_crtc.h
> >  create mode 100644 drivers/gpu/drm/zte/zx_drm_drv.c
> >  create mode 100644 drivers/gpu/drm/zte/zx_drm_drv.h
> >  create mode 100644 drivers/gpu/drm/zte/zx_hdmi.c
> >  create mode 100644 drivers/gpu/drm/zte/zx_plane.c
> >  create mode 100644 drivers/gpu/drm/zte/zx_plane.h
> 
> New entry in MAINTAINERS listening you (and probably dri-devel as the m-l)
> is missing.

Okay.  I will add a patch to do that in the next version.

> > +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);
> 
> drm_dev_register should be the last function call in your bind function.
> Similar for unbind, drm_dev_register should be called first.
> 
> As a consequence of that you can remove the drm_connector_(un)register
> calls, those are only needed for hotplugged connectors like dp mst. But
> with correct ordering of drm_dev_(un)register that function will also take
> care of connector registration and unregistration.

Aha, that's the trick to save the call to drm_connector_register() from
connector driver.  Thanks for the info.

> > +static int zx_hdmi_get_edid_block(void *data, u8 *buf, unsigned int block,
> > +				  size_t len)
> > +{
> > +	struct zx_hdmi *hdmi = data;
> > +	int retry = 0;
> > +	int ret = 0;
> > +	int i = 0;
> > +	u8 val;
> > +
> > +	/* Enable DDC master access */
> > +	val = hdmi_readb(hdmi, TPI_DDC_MASTER_EN);
> > +	val |= HW_DDC_MASTER;
> > +	hdmi_writeb(hdmi, TPI_DDC_MASTER_EN, val);
> > +
> > +	hdmi_writeb(hdmi, ZX_DDC_ADDR, 0xa0);
> > +	hdmi_writeb(hdmi, ZX_DDC_OFFSET, block * EDID_LENGTH);
> > +	/* Bits [9:8] of bytes */
> > +	hdmi_writeb(hdmi, ZX_DDC_DIN_CNT2, (len >> 8) & 0xff);
> > +	/* Bits [7:0] of bytes */
> > +	hdmi_writeb(hdmi, ZX_DDC_DIN_CNT1, len & 0xff);
> > +
> > +	/* Clear FIFO */
> > +	val = hdmi_readb(hdmi, ZX_DDC_CMD);
> > +	val &= ~DDC_CMD_MASK;
> > +	val |= DDC_CMD_CLEAR_FIFO;
> > +	hdmi_writeb(hdmi, ZX_DDC_CMD, val);
> > +
> > +	/* Kick off the read */
> > +	val = hdmi_readb(hdmi, ZX_DDC_CMD);
> > +	val &= ~DDC_CMD_MASK;
> > +	val |= DDC_CMD_SEQUENTIAL_READ;
> > +	hdmi_writeb(hdmi, ZX_DDC_CMD, val);
> 
> It looks like the ZX_DDC register range implements a hw i2c engine (since
> you specifiy port and offsets and everything). Please implement it as an
> i2c_adapter driver and use the normal drm_get_edid function.

Okay.  I will give it a try to see if it works.

> > +static int zx_gl_plane_atomic_check(struct drm_plane *plane,
> > +				    struct drm_plane_state *state)
> > +{
> > +	u32 src_w, src_h;
> > +
> > +	src_w = state->src_w >> 16;
> > +	src_h = state->src_h >> 16;
> > +
> > +	/* TODO: support scaling of the plane source */
> > +	if ((src_w != state->crtc_w) || (src_h != state->crtc_h))
> > +		return -EINVAL;
> 
> This is generally not enough checking. You probably need a call to
> drm_plane_helper_check_state.

Okay, will do.

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