Re: [PATCH v5 1/3] drm/rockchip: Add basic drm driver

From: Mark yao <mark.yao@rock-chips.com>
To: Daniel Kurtz <djkurtz@chromium.org>
Cc: "Mark Rutland" <mark.rutland@arm.com>,
	"Heiko Stübner" <heiko@sntech.de>,
	linux-doc@vger.kernel.org, kever.yang@rock-chips.com,
	dri-devel <dri-devel@lists.freedesktop.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	xjq@rock-chips.com, zyw@rock-chips.com,
	linux-api@vger.kernel.org, "jeff chen" <cym@rock-chips.com>,
	linux-rockchip@lists.infradead.org, kfx@rock-chips.com,
	"Grant Likely" <grant.likely@linaro.org>,
	wxt@rock-chips.com, huangtao@rock-chips.com,
	devicetree@vger.kernel.org, "Pawel Moll" <pawel.moll@arm.com>,
	"Ian Campbell" <ijc+devicetree@hellion.org.uk>,
	yxj@rock-chips.com, "Eddie Cai" <cf@rock-chips.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Stéphane Marchesin" <marcheu@chromium.org>,
	"simon xue" <xxm@rock-chips.com>,
	xw@rock-chips.com,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Randy Dunlap" <rdunlap@infradead.or>
Subject: Re: [PATCH v5 1/3] drm/rockchip: Add basic drm driver
Date: Fri, 26 Sep 2014 09:26:16 +0800	[thread overview]
Message-ID: <5424C0B8.3000402@rock-chips.com> (raw)
In-Reply-To: <5424067B.7090908@rock-chips.com>


[-- Attachment #1.1: Type: text/plain, Size: 2561 bytes --]

On 2014年09月25日 20:11, Mark yao wrote:
> On 2014年09月25日 16:58, Mark yao wrote:
>> On 2014年09月25日 00:20, Daniel Kurtz wrote:
>>> Hi Mark,
>>>
>>> Please review comments inline...
>>>
>>> On Wed, Sep 24, 2014 at 10:12 AM, Mark yao<mark.yao@rock-chips.com>  wrote:
>>> To match the enum name, use ROCKCHIP_OUTPUT_TYPE_*.
>>> Also, no need to explicitly set the first one to 0.
>>> However, see below.  I don't think we to modify the drm_display_mode
>>> to include an output type.
>> but vop devices need know the connector type, connector enable 
>> register is in vop.
>> can I do that like under to  get connector type for crtc?
>>
>>     static int rockchip_get_connector_type(struct drm_crtc *crtc)
>>     {
>>           struct drm_device *dev = crtc->dev;
>>           struct drm_connector * connector;
>>
>>           list_for_each_entry(connector, 
>> &dev->mode_config.connector_list, head) {
>>           if (!connector->encoder)
>>                   continue;
>>           /*
>>            * one crtc only has one connector in my case, so just find 
>> the first connector at list.
>>            */
>>           if (connector->encoder->crtc == crtc)
>>                   break;
>>     }
>>
>>     if (!connector)
>>             return -EINVAL;
>>
>>     return connector->connector_type;
>> } 
> Oh, sorry, forgot to drop this comment,
> for connector type problem, I try to new a help function for encoder 
> to call as Daniel advices.
>>>> +#define to_rockchip_plane(x) container_of(x, struct rockchip_plane, base)
>>>> +
>>>> +struct rockchip_plane {
>>>> +       int id;
>>>> +       struct drm_plane base;
>>>> +       const struct vop_win *win;
>>>> +       struct vop_context *ctx;
>>> Isn't ctx just: to_vop_ctx(base->crtc)
>>>
>> OK. we can use to_vop_ctx(base->crtc) to get ctx. 
> I have do a test to use "to_vop_ctx(base->crtc)", but found that 
> "base->crtc" maybe not init.
> for cursor plane, base->crtc always is NULL. and disable_plane will fail.
> maybe we can add "base->crtc = crtc" at update_plane, but it seems not 
> good.
> so I think still use "rockchip_plane->ctx" would be better.
>
> -Mark
I found that: plane->crtc will be set if update_plane success, and will 
be set NULL if disable_plane success.
so disable_plane must after update_plane.
disable_plane get crtc==NULL problem is that disable_plane was called 
before update_plane or been called  twice.
for this reason we can just check if crtc is NULL at disable_plane.

-Mark


[-- Attachment #1.2: Type: text/html, Size: 5279 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel