<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On 16 September 2015 at 23:23, Daniel Stone <span dir="ltr"><<a href="mailto:daniel@fooishbar.org" target="_blank">daniel@fooishbar.org</a>></span> wrote:</div><div class="gmail_quote">Hi Daniel, thank you so much for your good advice:-)</div><div class="gmail_quote">I am xinwei write the hisi drm driver together. I'll reply your comments.<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi Xinwei,<br>
Thanks for this contribution! We look forward to seeing support for<br>
these devices.<br>
<br>
This isn't an exhaustive review, but two very high-level comments<br>
which should result in a lot of changes ...<br>
<br>
On 15 September 2015 at 10:37, Xinwei Kong<br>
<span class=""><<a href="mailto:kong.kongxinwei@hisilicon.com">kong.kongxinwei@hisilicon.com</a>> wrote:<br>
> 1. Hardware Detail<br>
>   The display subsystem of Hi6220 SoC is shown as bellow:<br>
>  +-----+       +----------+     +-----+     +---------+<br>
>  |     |       |          |     |     |     |         |<br>
>  | FB  |------>|   ADE    |---->| DSI |---->| External|<br>
>  |     |       |          |     |     |     |  HDMI   |<br>
>  +-----+       +----------+     +-----+     +---------+<br>
><br>
> - ADE(Advanced Display Engine) is the display controller. It contains 7<br>
> channels or pipes, 3 overlay and a LDI.<br>
>   - A channel/pipe looks like: DMA-->clip-->scale-->ctrans/csc.<br>
>   - Overlay is response to compose planes which come from 7 channels and<br>
>   pass composed image to LDI.<br>
<br>
</span>This terminology is backwards from what we usually use in DRM, where<br>
an overlay is a special case of DRM planes, and pipes are DRM CRTCs.<br></blockquote><div> </div><div>I think I misunderstood the pipe terminology. I thought pipe is same as plane before,</div><div>but now I understand that a pipe will has its own CRTC and generally different pipe</div><div>handle different display content, right?</div><div>And in our SoC, a Overlay component do the same thing as a compositor. And a channel</div><div>may handle the primary plane or a overlay plane.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
>   - LDI is response to generate timings and RGB data stream.<br>
> - DSI converts the RGB data stream from ADE to DSI packets.<br>
> - External HDMI module is connected with DSI bus. Now Hikey use a ADI's<br>
>   ADV7533 external HDMI chip.<br>
<br>
</span>So this is basically just an implementation detail of DSI?</blockquote><div>Yes. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
> 2. Software Detail<br>
>   About the software organization and implementation detail:<br>
> We have a main drm platform driver (hisi_drm_drv.c), ade platform driver<br>
> (hisi_ade.c) and a dsi platform driver (hisi_drm_dsi.c). Ade driver<br>
> implements the plane and crtc driver interfaces and dsi implements the<br>
> encoder and connector driver interfaces. We take advantage of component<br>
> framework to initialize each driver.<br>
>   In order to support multi coming Hisilicon's SoCs, we plan to separate<br>
> common driver code and SoC specific implemented code as possiple as we can.<br>
> We abstract an ops for each component(crtc, plane, encoder and connector)<br>
> to reuse the common interface implementation logic (FIXME: Not sure if we<br>
> can achieve this target and if it is good or not). Thus, we put these<br>
> common driver code into hisi_drm_drv/crtc/plane/encoder/connector.c files.<br>
<br>
</span>Please do not do this; in general, the abstraction layers cause more<br>
problems than they create. We have only just finished removing all the<br>
abstraction layers from drivers/gpu/drm/exynos/, which started off<br>
with exactly the same idea, but only created problems. The issue is<br>
that every time the DRM core interface changes, you have to make the<br>
exact same changes in your copies of the interface. In general, there<br>
seems to be no benefit to having these here: you can just assign the<br>
DRM functions directly according to generation. See current Exynos for<br>
an example of this.<br></blockquote><div> </div><div>OK, actually, I also think a  abstraction layer may not good when we finish it.</div><div>We will drop the abstraction in next version of patch.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
The biggest issue though, is that this driver should become an atomic<br>
modesetting driver. Atomic modesetting, rather than sending small<br>
individual commands (enable CRTC, change plane position, etc) is based<br>
on validating and passing around complete sets of hardware state.<br></blockquote><div> </div><div>Is it done by call the DRM_IOCTL_MODE_ATOMIC ioctl in the userland?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Daniel Vetter's blog has an article on how to convert your driver:<br>
<a href="http://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html" rel="noreferrer" target="_blank">http://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html</a><br>
<br>
In addition, there are some drivers converted already that you can<br>
look at: tegra (very simple), exynos (reasonably simple), fsl-dcu<br>
(moderate), msm (quite complex), i915 (incredibly complex), rcar-du<br>
(???).<br>
<br>
Once your driver is converted to atomic and the abstraction layers<br>
removed, then it will be much easier to review the submission in<br>
detail.<br></blockquote><div> </div><div>We have converted to atomic for this version of patches. But maybe we need to test</div><div>atomic mode setting with the DRM_IOCTL_MODE_ATOMIC ioctl.</div><div><br></div><div>Thanks,</div><div>-Xinliang</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Thanks very much!<br>
<br>
Cheers,<br>
Daniel<br>
_______________________________________________<br>
dri-devel mailing list<br>
<a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a><br>
</blockquote></div><br></div></div>