Message-ID: <55FBE840.2040406@hisilicon.com>
Date:	Fri, 18 Sep 2015 18:32:32 +0800
From:	Xinwei Kong <kong.kongxinwei@...ilicon.com>
To:	Daniel Stone <daniel@...ishbar.org>
CC:	David Airlie <airlied@...ux.ie>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	dri-devel <dri-devel@...ts.freedesktop.org>,
	<haojian.zhuang@...aro.org>, <james.yanglong@...ilicon.com>,
	<yinshengbao@...ilicon.com>, <xuyiping@...ilicon.com>,
	<xuwei5@...ilicon.com>, <qijiwen@...ilicon.com>,
	<puck.chen@...ilicon.com>, <yanhaifeng@...ilicon.com>,
	<fangdechun@...ilicon.com>, <andy.green@...aro.org>,
	<gongyu@...ilicon.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	<ml.yang@...ilicon.com>, <liguozhu@...ilicon.com>
Subject: Re: [PATCH RFC 0/8] Add New DRM Driver for Hisilicon's Hi6220 SoC

hi Daniel Stone:

On 2015/9/16 23:23, Daniel Stone wrote:
> 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@...ilicon.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.
> 
>>   - 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?
> 
>> 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.
> 
I understand that you want to let us remove the hisi_drm_crtc/plane/
encoder/connector.c files in my driver.

When we plan to use abstraction layers, our purpose is that our commmon
drm interface will be used in diff hisilicon soc platform such as mobile
series、TV series soc , if this common interface don't use in diff soc
or not take advantage of following DRM core interface changes, we will fix it.

but if I will port hisi_drm_crtc/plane/.c file code into hisi_ade.c file
and port hisi_drm_encoder/connector.c into hisi_drm_dsi.c file,
when we add some other soc platform, we will be similar to create hardware
ip *.c file, if DRM core interface will changes , we will change all refering
ip.c file.

I wish that you can give me some guides for this abstraction layers.

Thank you
xinwei


> 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.
> 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.
> 
> Thanks very much!
> 
> Cheers,
> Daniel
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists