From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark yao Subject: Re: [PATCH v5 1/3] drm/rockchip: Add basic drm driver Date: Fri, 26 Sep 2014 09:26:16 +0800 Message-ID: <5424C0B8.3000402@rock-chips.com> References: <1411524672-14524-1-git-send-email-mark.yao@rock-chips.com> <1411524746-14635-1-git-send-email-mark.yao@rock-chips.com> <5423D951.4030703@rock-chips.com> <5424067B.7090908@rock-chips.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1769245416==" Return-path: In-Reply-To: <5424067B.7090908@rock-chips.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Kurtz Cc: Mark Rutland , =?UTF-8?B?SGVpa28gU3TDvGJuZXI=?= , linux-doc@vger.kernel.org, kever.yang@rock-chips.com, dri-devel , "linux-kernel@vger.kernel.org" , xjq@rock-chips.com, zyw@rock-chips.com, linux-api@vger.kernel.org, jeff chen , linux-rockchip@lists.infradead.org, kfx@rock-chips.com, Grant Likely , wxt@rock-chips.com, huangtao@rock-chips.com, devicetree@vger.kernel.org, Pawel Moll , Ian Campbell , yxj@rock-chips.com, Eddie Cai , Rob Herring , =?UTF-8?B?U3TDqXBoYW5lIE1hcmNoZXNpbg==?= , simon xue , xw@rock-chips.com, Greg Kroah-Hartman , Randy Dunlap List-Id: linux-api@vger.kernel.org This is a multi-part message in MIME format. --===============1769245416== Content-Type: multipart/alternative; boundary="------------040503080203000708080305" This is a multi-part message in MIME format. --------------040503080203000708080305 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable On 2014=E5=B9=B409=E6=9C=8825=E6=97=A5 20:11, Mark yao wrote: > On 2014=E5=B9=B409=E6=9C=8825=E6=97=A5 16:58, Mark yao wrote: >> On 2014=E5=B9=B409=E6=9C=8825=E6=97=A5 00:20, Daniel Kurtz wrote: >>> Hi Mark, >>> >>> Please review comments inline... >>> >>> On Wed, Sep 24, 2014 at 10:12 AM, Mark yao = 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=20 >> 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 =3D crtc->dev; >> struct drm_connector * connector; >> >> list_for_each_entry(connector,=20 >> &dev->mode_config.connector_list, head) { >> if (!connector->encoder) >> continue; >> /* >> * one crtc only has one connector in my case, so just find=20 >> the first connector at list. >> */ >> if (connector->encoder->crtc =3D=3D crtc) >> break; >> } >> >> if (!connector) >> return -EINVAL; >> >> return connector->connector_type; >> }=20 > Oh, sorry, forgot to drop this comment, > for connector type problem, I try to new a help function for encoder=20 > 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.=20 > I have do a test to use "to_vop_ctx(base->crtc)", but found that=20 > "base->crtc" maybe not init. > for cursor plane, base->crtc always is NULL. and disable_plane will fai= l. > maybe we can add "base->crtc =3D crtc" at update_plane, but it seems no= t=20 > 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=20 be set NULL if disable_plane success. so disable_plane must after update_plane. disable_plane get crtc=3D=3DNULL problem is that disable_plane was called= =20 before update_plane or been called twice. for this reason we can just check if crtc is NULL at disable_plane. -Mark --------------040503080203000708080305 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On 2014=E5=B9=B409=E6=9C=8825=E6=97=A5= 20:11, Mark yao wrote:
On 2014=E5=B9=B409=E6=9C=8825=E6=97=A5= 16:58, Mark yao wrote:
On 2014=E5=B9=B409=E6=9C=8825=E6=97= =A5 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=C2=A0 get connector type for crtc?
=C2=A0=C2=A0=C2=A0 static int rockchip_get_connector_type(struct = drm_crtc *crtc)
=C2=A0=C2=A0=C2=A0 {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct drm= _device *dev =3D crtc->dev;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct drm= _connector * connector;

=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 list_for_e= ach_entry(connector, &dev->mode_config.connector_list, head) {
=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!connector-= >encoder)
=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 continue;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /*
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * on= e crtc only has one connector in my case, so just find the first connector at list.
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */ =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (connector-&= gt;encoder->crtc =3D=3D crtc)
=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 break;
=C2=A0=C2=A0=C2=A0 }

=C2=A0=C2=A0=C2=A0 if (!connector)
=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret= urn -EINVAL;

=C2=A0=C2=A0=C2=A0 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 =3D 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=3D=3DNULL problem is that disable_plane was called before update_plane or been called=C2=A0 twice.
for this reason we can just check if crtc is NULL at disable_plane.
-Mark




--------------040503080203000708080305-- --===============1769245416== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============1769245416==--