On 16 September 2015 at 23:23, Daniel Stone daniel@fooishbar.org wrote: Hi Daniel, thank you so much for your good advice:-) I am xinwei write the hisi drm driver together. I'll reply your comments.
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:
- 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.
I think I misunderstood the pipe terminology. I thought pipe is same as plane before, but now I understand that a pipe will has its own CRTC and generally different pipe handle different display content, right? And in our SoC, a Overlay component do the same thing as a compositor. And a channel may handle the primary plane or a overlay plane.
- 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?
Yes.
- 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.
OK, actually, I also think a abstraction layer may not good when we finish it. We will drop the abstraction in next version of patch.
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.
Is it done by call the DRM_IOCTL_MODE_ATOMIC ioctl in the userland?
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.
We have converted to atomic for this version of patches. But maybe we need to test atomic mode setting with the DRM_IOCTL_MODE_ATOMIC ioctl.
Thanks, -Xinliang
Thanks very much!
Cheers, Daniel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel