Re: [PATCH v2] drm/rockchip: Use an enum to identify Rockchip VOPs

[Brian Norris <briannorris@chromium.org>]

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