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