On 2017?10?20? 01:46, Kristian H?gsberg wrote: > On Wed, Oct 18, 2017 at 5:49 PM, Mark yao <mark.yao at rock-chips.com> wrote: >> On 2017?10?19? 01:52, Brian Norris wrote: >>> Hi Kristian, >>> >>> On Thu, Nov 03, 2016 at 12:46:48PM -0700, Kristian H?gsberg 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. >> >> Unfortunately, vop iomux and vop data are not one-to-one correspondence. >> >> Such as rk3288, vop big and vop little both use rk3288_vop data, if you use >> vop data id will not be able >> to distinguish between them. > What would you suggest then? We need this to be able to drive eDP from > the big VOP. We've been carrying this patch for quite a while and it > hasn't regressed neither rk3288 nor rk3399 boards. > > Kristian > >> Mark >>>> >>>> On way to test this is to use the modetest tool from libdrm: >>>> >>>> $ modetest -M rockchip -s 32 at 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. The display issue already be fixed, and your kernel seems old See the patch: http://patchwork.freedesktop.org/patch/msgid/1495885416-22216-1-git-send-email-mark.yao at rock-chips.com Now eDP always set its outmode as ROCKCHIP_OUT_MODE_AAAA. And on vop driver, force RGB10 to RGB888 if vop not support rgb10bit. drivers/gpu/drm/rockchip/rockchip_drm_vop.c: /* * if vop is not support RGB10 output, need force RGB10 to RGB888. */ if (s->output_mode == ROCKCHIP_OUT_MODE_AAAA && !(vop_data->feature & VOP_FEATURE_OUTPUT_RGB10)) s->output_mode = ROCKCHIP_OUT_MODE_P888; Thanks. >>>> >>>> For reference, >>>> >>>> $ modetest -M rockchip -s 32 at 25:2400x1600 >>>> >>>> drives the eDP from little VOP and displays correctly. >>>> >>>> Signed-off-by: Kristian H. Kristensen <hoegsberg at chromium.org> >>>> --- >>>> v2: Stripped chromeos annotations, fix compile errors for drivers I >>>> didn't >>>> compile when I first wrote the patch. >>> Do you plan to fix the errors Guenter pointed out and resend? This is >>> still causing conflicts between our ChromeOS kernel trees and upstream. >>> >>> Also, please CC linux-rockchip on Rockchip-related patches. It's much >>> easier for me to find your work that way :) >>> >>> Brian >>> >>>> drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 45 >>>> ++++++++++--------------- >>>> drivers/gpu/drm/rockchip/cdn-dp-core.c | 19 +++++------ >>>> drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 12 ++++--- >>>> 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, 56 insertions(+), 41 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..b01a82a 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,13 +893,18 @@ static void dw_mipi_dsi_encoder_commit(struct >>>> drm_encoder *encoder) >>>> clk_disable_unprepare(dsi->pclk); >>>> - if (mux) >>>> + switch (vop_get_crtc_vop_id(encoder->crtc)) { >>>> + case RK3399_VOP_LIT: >>>> + dev_dbg(dsi->dev, "vop LIT output to dsi0\n"); >>>> val = DSI0_SEL_VOP_LIT | (DSI0_SEL_VOP_LIT << 16); >>>> - else >>>> + break; >>>> + default: >>>> + dev_dbg(dsi->dev, "vop BIG output to dsi0\n"); >>>> 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"); >>>> } >>>> static int >>>> 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, >>> _______________________________________________ >>> Linux-rockchip mailing list >>> Linux-rockchip at lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-rockchip >>> >>> >>> >> _______________________________________________ >> dri-devel mailing list >> dri-devel at lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip -- ?ark Yao