On 16 September 2015 at 23:23, Daniel Stone <daniel@xxxxxxxxxxxxx> 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@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.
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.
> 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.
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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
