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

[Xinliang Liu]

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>