* [PATCH] drm/rockchip: Use an enum to identify Rockchip VOPs
@ 2016-11-01 21:41 Kristian H. Kristensen
2016-11-03 15:47 ` Sean Paul
0 siblings, 1 reply; 3+ messages in thread
From: Kristian H. Kristensen @ 2016-11-01 21:41 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.
BUG=chrome-os-partner:56407
TEST=verify 'modetest -M rockchip -s 32@28:2400x1600' looks right
Signed-off-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Change-Id: If5c5f36bcee09113008ee5155a13337f0e69371f
---
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 | 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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/rockchip: Use an enum to identify Rockchip VOPs
2016-11-01 21:41 [PATCH] drm/rockchip: Use an enum to identify Rockchip VOPs Kristian H. Kristensen
@ 2016-11-03 15:47 ` Sean Paul
2016-11-03 19:49 ` Kristian Høgsberg
0 siblings, 1 reply; 3+ messages in thread
From: Sean Paul @ 2016-11-03 15:47 UTC (permalink / raw)
To: Kristian H. Kristensen; +Cc: Kristian H. Kristensen, DRI mailing list
On Tue, Nov 1, 2016 at 3:41 PM, Kristian H. Kristensen
<hoegsberg@gmail.com> 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 <hoegsberg@chromium.org>
> 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.
> ---
> 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 <seanpaul@chromium.org>
> 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
> 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] 3+ messages in thread
* Re: [PATCH] drm/rockchip: Use an enum to identify Rockchip VOPs
2016-11-03 15:47 ` Sean Paul
@ 2016-11-03 19:49 ` Kristian Høgsberg
0 siblings, 0 replies; 3+ messages in thread
From: Kristian Høgsberg @ 2016-11-03 19:49 UTC (permalink / raw)
To: Sean Paul; +Cc: Kristian H. Kristensen, DRI mailing list
[-- Attachment #1.1: Type: text/plain, Size: 11811 bytes --]
On Thu, Nov 3, 2016 at 8:47 AM Sean Paul <seanpaul@chromium.org> wrote:
> On Tue, Nov 1, 2016 at 3:41 PM, Kristian H. Kristensen
> <hoegsberg@gmail.com> 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 <hoegsberg@chromium.org>
> > 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.
>
Yup, I sent a v2 with the headers stripped and fixed the compile errors in
the
encoder modules I didn't initially compile.
Kristian
>
>
> > ---
> > 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 <seanpaul@chromium.org>
>
>
> > 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
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
[-- Attachment #1.2: Type: text/html, Size: 20442 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-11-03 19:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-01 21:41 [PATCH] drm/rockchip: Use an enum to identify Rockchip VOPs Kristian H. Kristensen
2016-11-03 15:47 ` Sean Paul
2016-11-03 19:49 ` Kristian Høgsberg
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.