On 16 September 2015 at 23:23, Daniel Stone <daniel at 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 at 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. > 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 at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150917/375d1cb8/attachment.html>
