Re: [PATCH RFC 0/8] Add New DRM Driver for Hisilicon's Hi6220 SoC

[Daniel Stone <daniel@fooishbar.org>]

Hi Xinwei,
Thanks for this contribution! We look forward to seeing support for
these devices.

This isn't an exhaustive review, but two very high-level comments
which should result in a lot of changes ...

On 15 September 2015 at 10:37, Xinwei Kong
<kong.kongxinwei@hisilicon.com> wrote:
> 1. Hardware Detail
>   The display subsystem of Hi6220 SoC is shown as bellow:
>  +-----+       +----------+     +-----+     +---------+
>  |     |       |          |     |     |     |         |
>  | FB  |------>|   ADE    |---->| DSI |---->| External|
>  |     |       |          |     |     |     |  HDMI   |
>  +-----+       +----------+     +-----+     +---------+
>
> - ADE(Advanced Display Engine) is the display controller. It contains 7
> channels or pipes, 3 overlay and a LDI.
>   - A channel/pipe looks like: DMA-->clip-->scale-->ctrans/csc.
>   - Overlay is response to compose planes which come from 7 channels and
>   pass composed image to LDI.

This terminology is backwards from what we usually use in DRM, where
an overlay is a special case of DRM planes, and pipes are DRM CRTCs.

>   - LDI is response to generate timings and RGB data stream.
> - DSI converts the RGB data stream from ADE to DSI packets.
> - External HDMI module is connected with DSI bus. Now Hikey use a ADI's
>   ADV7533 external HDMI chip.

So this is basically just an implementation detail of DSI?

> 2. Software Detail
>   About the software organization and implementation detail:
> We have a main drm platform driver (hisi_drm_drv.c), ade platform driver
> (hisi_ade.c) and a dsi platform driver (hisi_drm_dsi.c). Ade driver
> implements the plane and crtc driver interfaces and dsi implements the
> encoder and connector driver interfaces. We take advantage of component
> framework to initialize each driver.
>   In order to support multi coming Hisilicon's SoCs, we plan to separate
> common driver code and SoC specific implemented code as possiple as we can.
> We abstract an ops for each component(crtc, plane, encoder and connector)
> to reuse the common interface implementation logic (FIXME: Not sure if we
> can achieve this target and if it is good or not). Thus, we put these
> common driver code into hisi_drm_drv/crtc/plane/encoder/connector.c files.

Please do not do this; in general, the abstraction layers cause more
problems than they create. We have only just finished removing all the
abstraction layers from drivers/gpu/drm/exynos/, which started off
with exactly the same idea, but only created problems. The issue is
that every time the DRM core interface changes, you have to make the
exact same changes in your copies of the interface. In general, there
seems to be no benefit to having these here: you can just assign the
DRM functions directly according to generation. See current Exynos for
an example of this.

The biggest issue though, is that this driver should become an atomic
modesetting driver. Atomic modesetting, rather than sending small
individual commands (enable CRTC, change plane position, etc) is based
on validating and passing around complete sets of hardware state.
Daniel Vetter's blog has an article on how to convert your driver:
http://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html

In addition, there are some drivers converted already that you can
look at: tegra (very simple), exynos (reasonably simple), fsl-dcu
(moderate), msm (quite complex), i915 (incredibly complex), rcar-du
(???).

Once your driver is converted to atomic and the abstraction layers
removed, then it will be much easier to review the submission in
detail.

Thanks very much!

Cheers,
Daniel