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@xxxxxxxxxxxxx> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
