<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, Nov 3, 2016 at 8:47 AM Sean Paul <<a href="mailto:seanpaul@chromium.org">seanpaul@chromium.org</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Nov 1, 2016 at 3:41 PM, Kristian H. Kristensen <<a href="mailto:hoegsberg@gmail.com" class="gmail_msg" target="_blank">hoegsberg@gmail.com</a>> wrote: > We used to call drm_of_encoder_active_endpoint_id() from > rockchip_dp_drm_encoder_atomic_check() to determine the endpoint for the > active encoder. However, the encoder isn't necessarily active at this > point or it may be connected to different crtc than what we're switching > to. Instead, look at the crtc from the drm_crtc_state. This fixes wrong > colors when driving the eDP with the big VOP. Further, we can identify > the type of VOP we're dealing with by just putting a VOP id enum in the > vop_data. > > On way to test this is to use the modetest tool from libdrm: > > $ modetest -M rockchip -s 32@28:2400x1600 > > which displays dark or black colors because we fail to look up the > endpoint and use default 0 (which is ROCKCHIP_OUT_MODE_P888) for big > VOP instead of RGB10 as required. > > For reference, > > $ modetest -M rockchip -s 32@25:2400x1600 > > drives the eDP from little VOP and displays correctly. > > BUG=chrome-os-partner:56407 > TEST=verify 'modetest -M rockchip -s 32@28:2400x1600' looks right > Signed-off-by: Kristian H. Kristensen <<a href="mailto:hoegsberg@chromium.org" class="gmail_msg" target="_blank">hoegsberg@chromium.org</a>> > Change-Id: If5c5f36bcee09113008ee5155a13337f0e69371f Thanks for the patch, Kristian. In the future, can you please strip out BUG/TEST/Change-Id when you upstream? I think checkpatch will gently remind you of this as well. </blockquote><div> </div><div>Yup, I sent a v2 with the headers stripped and fixed the compile errors in the</div><div>encoder modules I didn't initially compile.</div><div> </div><div>Kristian</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > --- > drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 45 ++++++++++--------------- > drivers/gpu/drm/rockchip/cdn-dp-core.c | 19 +++++------ cdn-dp isn't upstream yet (I need to get that posted), I'll work on this next week when I'm back from Plumbers. If that goes smoothly, I can push this patch on top. Reviewed-by: Sean Paul <<a href="mailto:seanpaul@chromium.org" class="gmail_msg" target="_blank">seanpaul@chromium.org</a>> > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 9 +++-- > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 7 ++++ > drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 10 ++++++ > drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 4 +++ > 6 files changed, 54 insertions(+), 40 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > index 558a3bc..9ae4a9c 100644 > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > @@ -196,14 +196,16 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder) > int ret; > u32 val; > > - ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder); > - if (ret < 0) > - return; > - > - if (ret) > + switch (vop_get_crtc_vop_id(encoder->crtc)) { > + case RK3399_VOP_LIT: > val = dp->data->lcdsel_lit; > - else > + break; > + case RK3399_VOP_BIG: > val = dp->data->lcdsel_big; > + break; > + default: > + return; > + } > > dev_dbg(dp->dev, "vop %s output to dp\n", (ret) ? "LIT" : "BIG"); > > @@ -231,34 +233,23 @@ rockchip_dp_drm_encoder_atomic_check(struct drm_encoder *encoder, > struct drm_connector_state *conn_state) > { > struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state); > - struct rockchip_dp_device *dp = to_dp(encoder); > - int ret; > > - /* > - * The hardware IC designed that VOP must output the RGB10 video > - * format to eDP contoller, and if eDP panel only support RGB8, > - * then eDP contoller should cut down the video data, not via VOP > - * contoller, that's why we need to hardcode the VOP output mode > - * to RGA10 here. > - */ > - > - ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder); > - if (ret < 0) > - return 0; > - > - switch (dp->data->chip_type) { > - case RK3399_EDP: > + switch (vop_get_crtc_vop_id(crtc_state->crtc)) { > + case RK3399_VOP_LIT: > /* > * For RK3399, VOP Lit must code the out mode to RGB888, > * VOP Big must code the out mode to RGB10. > */ > - if (ret) > - s->output_mode = ROCKCHIP_OUT_MODE_P888; > - else > - s->output_mode = ROCKCHIP_OUT_MODE_AAAA; > + s->output_mode = ROCKCHIP_OUT_MODE_P888; > break; > - > default: > + /* > + * The hardware IC designed that VOP must output the RGB10 video > + * format to eDP contoller, and if eDP panel only support RGB8, > + * then eDP contoller should cut down the video data, not via VOP > + * contoller, that's why we need to hardcode the VOP output mode > + * to RGA10 here. > + */ > s->output_mode = ROCKCHIP_OUT_MODE_AAAA; > break; > } > diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c > index f020e2e..b9e6184 100644 > --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c > +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c > @@ -570,21 +570,20 @@ static void cdn_dp_encoder_mode_set(struct drm_encoder *encoder, > video->v_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NVSYNC); > video->h_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NHSYNC); > > - ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder); > - if (ret < 0) { > - DRM_DEV_ERROR(dp->dev, "Could not get vop id, %d", ret); > - return; > - } > - > - DRM_DEV_DEBUG_KMS(dp->dev, "vop %s output to cdn-dp\n", > - (ret) ? "LIT" : "BIG"); > state = to_rockchip_crtc_state(encoder->crtc->state); > - if (ret) { > + switch (vop_get_crtc_vop_id(encoder->crtc)) { > + case RK3399_VOP_LIT: > + DRM_DEV_DEBUG_KMS(dp->dev, "vop LIT output to cdn-dp\n"); > val = DP_SEL_VOP_LIT | (DP_SEL_VOP_LIT << 16); > state->output_mode = ROCKCHIP_OUT_MODE_P888; > - } else { > + break; > + case RK3399_VOP_BIG: > + DRM_DEV_DEBUG_KMS(dp->dev, "vop BIG output to cdn-dp\n"); > val = DP_SEL_VOP_LIT << 16; > state->output_mode = ROCKCHIP_OUT_MODE_AAAA; > + break; > + default: > + break; > } > > ret = cdn_dp_grf_write(dp, GRF_SOC_CON9, val); > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > index dedc65b..c2b0f5d 100644 > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > @@ -878,7 +878,6 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder) > static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder) > { > struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder); > - int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder); > u32 val; > > if (clk_prepare_enable(dsi->pclk)) { > @@ -894,10 +893,14 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder) > > clk_disable_unprepare(dsi->pclk); > > - if (mux) > + switch (vop_get_crtc_vop_id(crtc_state->crtc)) { > + case RK3399_VOP_LIT: > val = DSI0_SEL_VOP_LIT | (DSI0_SEL_VOP_LIT << 16); > - else > + break; > + default: > val = DSI0_SEL_VOP_LIT << 16; > + break; > + } > > regmap_write(dsi->grf_regmap, GRF_SOC_CON6, val); > dev_dbg(dsi->dev, "vop %s output to dsi0\n", (mux) ? "LIT" : "BIG"); > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > index d5ea4ab..e849f26 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > @@ -141,6 +141,13 @@ struct vop { > struct vop_win win[]; > }; > > +enum vop_id vop_get_crtc_vop_id(struct drm_crtc *crtc) > +{ > + struct vop *vop = to_vop(crtc); > + > + return vop->data->id; > +} > + > static inline void vop_writel(struct vop *vop, uint32_t offset, uint32_t v) > { > writel(v, vop->regs + offset); > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > index 5a4faa85..2c54bcc 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > @@ -135,8 +135,16 @@ struct vop_win_data { > enum drm_plane_type type; > }; > > +enum vop_id { > + RK3036_VOP, > + RK3288_VOP, > + RK3399_VOP_BIG, > + RK3399_VOP_LIT, > +}; > + > struct vop_data { > const struct vop_reg_data *init_table; > + uint32_t id; > unsigned int table_size; > const struct vop_ctrl *ctrl; > const struct vop_intr *intr; > @@ -321,5 +329,7 @@ static inline int scl_vop_cal_lb_mode(int width, bool is_yuv) > return lb_mode; > } > > +enum vop_id vop_get_crtc_vop_id(struct drm_crtc *crtc); > + > extern const struct component_ops vop_component_ops; > #endif /* _ROCKCHIP_DRM_VOP_H */ > diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > index aaede6b..a46e2c8 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > @@ -131,6 +131,7 @@ static const struct vop_reg_data rk3036_vop_init_reg_table[] = { > }; > > static const struct vop_data rk3036_vop = { > + .id = RK3036_VOP, > .init_table = rk3036_vop_init_reg_table, > .table_size = ARRAY_SIZE(rk3036_vop_init_reg_table), > .ctrl = &rk3036_ctrl_data, > @@ -272,6 +273,7 @@ static const struct vop_intr rk3288_vop_intr = { > }; > > static const struct vop_data rk3288_vop = { > + .id = RK3288_VOP, > .init_table = rk3288_init_reg_table, > .table_size = ARRAY_SIZE(rk3288_init_reg_table), > .intr = &rk3288_vop_intr, > @@ -340,6 +342,7 @@ static const struct vop_reg_data rk3399_init_reg_table[] = { > }; > > static const struct vop_data rk3399_vop_big = { > + .id = RK3399_VOP_BIG, > .init_table = rk3399_init_reg_table, > .table_size = ARRAY_SIZE(rk3399_init_reg_table), > .intr = &rk3399_vop_intr, > @@ -359,6 +362,7 @@ static const struct vop_win_data rk3399_vop_lit_win_data[] = { > }; > > static const struct vop_data rk3399_vop_lit = { > + .id = RK3399_VOP_LIT, > .init_table = rk3399_init_reg_table, > .table_size = ARRAY_SIZE(rk3399_init_reg_table), > .intr = &rk3399_vop_intr, > -- > 2.8.0.rc3.226.g39d4020 > > _______________________________________________ > dri-devel mailing list > <a href="mailto:dri-devel@lists.freedesktop.org" class="gmail_msg" target="_blank">dri-devel@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/dri-devel" rel="noreferrer" class="gmail_msg" target="_blank">https://lists.freedesktop.org/mailman/listinfo/dri-devel</a> </blockquote></div></div>