* [PATCH v2] drm/rockchip: Use an enum to identify Rockchip VOPs @ 2016-11-03 19:46 Kristian H. Kristensen 2016-11-08 21:06 ` [v2] " Guenter Roeck 2017-10-18 17:52 ` [PATCH v2] " Brian Norris 0 siblings, 2 replies; 6+ messages in thread From: Kristian H. Kristensen @ 2016-11-03 19:46 UTC (permalink / raw) To: dri-devel; +Cc: Kristian H. Kristensen 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. Signed-off-by: Kristian H. Kristensen <hoegsberg@chromium.org> --- v2: Stripped chromeos annotations, fix compile errors for drivers I didn't compile when I first wrote the patch. 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, -- 2.8.0.rc3.226.g39d4020 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [v2] drm/rockchip: Use an enum to identify Rockchip VOPs 2016-11-03 19:46 [PATCH v2] drm/rockchip: Use an enum to identify Rockchip VOPs Kristian H. Kristensen @ 2016-11-08 21:06 ` Guenter Roeck 2017-10-18 17:52 ` [PATCH v2] " Brian Norris 1 sibling, 0 replies; 6+ messages in thread From: Guenter Roeck @ 2016-11-08 21:06 UTC (permalink / raw) To: Kristian Høgsberg; +Cc: Kristian H. Kristensen, dri-devel 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. > > 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. > > Signed-off-by: Kristian H. Kristensen <hoegsberg@chromium.org> > --- > v2: Stripped chromeos annotations, fix compile errors for drivers I didn't > compile when I first wrote the patch. > [ ... ] > > +enum vop_id vop_get_crtc_vop_id(struct drm_crtc *crtc) > +{ > + struct vop *vop = to_vop(crtc); > + > + return vop->data->id; > +} Missing EXPORT() causes build errors if calling code is built as module. Example arm64:allmodconfig: ERROR: "vop_get_crtc_vop_id" [drivers/gpu/drm/rockchip/dw-mipi-dsi.ko] undefined! ERROR: "vop_get_crtc_vop_id" [drivers/gpu/drm/rockchip/cdn-dp.ko] undefined! ERROR: "vop_get_crtc_vop_id" [drivers/gpu/drm/rockchip/analogix_dp-rockchip.ko] undefined! Guenter _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/rockchip: Use an enum to identify Rockchip VOPs 2016-11-03 19:46 [PATCH v2] drm/rockchip: Use an enum to identify Rockchip VOPs Kristian H. Kristensen 2016-11-08 21:06 ` [v2] " Guenter Roeck @ 2017-10-18 17:52 ` Brian Norris 2017-10-19 0:49 ` Mark yao 1 sibling, 1 reply; 6+ messages in thread From: Brian Norris @ 2017-10-18 17:52 UTC (permalink / raw) To: Kristian Högsberg Cc: linux-rockchip, Kristian H. Kristensen, Matthias Kaehlcke, Guenter Roeck, dri-devel 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. > > 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. > > Signed-off-by: Kristian H. Kristensen <hoegsberg@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, _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/rockchip: Use an enum to identify Rockchip VOPs 2017-10-18 17:52 ` [PATCH v2] " Brian Norris @ 2017-10-19 0:49 ` Mark yao 2017-10-19 17:46 ` Kristian Høgsberg 0 siblings, 1 reply; 6+ messages in thread From: Mark yao @ 2017-10-19 0:49 UTC (permalink / raw) To: Brian Norris, Kristian Högsberg Cc: linux-rockchip, Kristian H. Kristensen, Matthias Kaehlcke, dri-devel, Guenter Roeck 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. Mark >> >> 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. >> >> Signed-off-by: Kristian H. Kristensen <hoegsberg@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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip > > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/rockchip: Use an enum to identify Rockchip VOPs 2017-10-19 0:49 ` Mark yao @ 2017-10-19 17:46 ` Kristian Høgsberg 2017-10-20 1:38 ` Mark yao 0 siblings, 1 reply; 6+ messages in thread From: Kristian Høgsberg @ 2017-10-19 17:46 UTC (permalink / raw) To: Mark yao Cc: Brian Norris, dri-devel, linux-rockchip, Matthias Kaehlcke, Kristian H. Kristensen, Guenter Roeck On Wed, Oct 18, 2017 at 5:49 PM, Mark yao <mark.yao@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@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. >>> >>> Signed-off-by: Kristian H. Kristensen <hoegsberg@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@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-rockchip >> >> >> > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/rockchip: Use an enum to identify Rockchip VOPs 2017-10-19 17:46 ` Kristian Høgsberg @ 2017-10-20 1:38 ` Mark yao 0 siblings, 0 replies; 6+ messages in thread From: Mark yao @ 2017-10-20 1:38 UTC (permalink / raw) To: Kristian Høgsberg Cc: Brian Norris, dri-devel, linux-rockchip, Matthias Kaehlcke, Kristian H. Kristensen, Guenter Roeck On 2017年10月20日 01:46, Kristian Høgsberg wrote: > On Wed, Oct 18, 2017 at 5:49 PM, Mark yao <mark.yao@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@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@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@25:2400x1600 >>>> >>>> drives the eDP from little VOP and displays correctly. >>>> >>>> Signed-off-by: Kristian H. Kristensen <hoegsberg@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@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-rockchip >>> >>> >>> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip -- Ｍark Yao _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-20 1:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-03 19:46 [PATCH v2] drm/rockchip: Use an enum to identify Rockchip VOPs Kristian H. Kristensen 2016-11-08 21:06 ` [v2] " Guenter Roeck 2017-10-18 17:52 ` [PATCH v2] " Brian Norris 2017-10-19 0:49 ` Mark yao 2017-10-19 17:46 ` Kristian Høgsberg 2017-10-20 1:38 ` Mark yao
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.