Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751962AbcDRJ0M (ORCPT ); Mon, 18 Apr 2016 05:26:12 -0400 Received: from dougal.metanate.com ([90.155.101.14]:45483 "EHLO metanate.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751515AbcDRJ0K (ORCPT ); Mon, 18 Apr 2016 05:26:10 -0400 Date: Mon, 18 Apr 2016 10:25:38 +0100 From: John Keeping To: Mark Yao Cc: David Airlie , Heiko Stuebner , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/rockchip: get rid of rockchip_drm_crtc_mode_config Message-ID: <20160418102538.0ed730ca.john@metanate.com> In-Reply-To: <1460948611-32259-1-git-send-email-mark.yao@rock-chips.com> References: <1460948611-32259-1-git-send-email-mark.yao@rock-chips.com> Organization: Metanate Ltd X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-unknown-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11397 Lines: 306 Hi Mark, On Mon, 18 Apr 2016 11:03:31 +0800, Mark Yao wrote: > We need to take care of the vop status when use > rockchip_drm_crtc_mode_config, if vop is disabled, > the function would failed, that is terrible. > > Save connector type and output mode on drm_display_mode->private_flags > at encoder mode_fixup, then we can configure the type and mode safely > on crtc mode_set. Since Rockchip is atomic, shouldn't this be using atomic_check hooks and a subclassed crtc_state structure? > Signed-off-by: Mark Yao > --- > drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 37 +++++++--------- > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 44 ++++++++++--------- > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 5 +-- > drivers/gpu/drm/rockchip/inno_hdmi.c | 5 +-- > drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 2 - > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 51 +++++++++-------------- > drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 6 +++ > 7 files changed, 69 insertions(+), 81 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > index a1d94d8..0344e52 100644 > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > @@ -97,7 +97,21 @@ rockchip_dp_drm_encoder_mode_fixup(struct drm_encoder *encoder, > const struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode) > { > - /* do nothing */ > + /* > + * FIXME(Yakir): driver should configure the CRTC output video > + * mode with the display information which indicated the monitor > + * support colorimetry. > + * > + * But don't know why the CRTC driver seems could only output the > + * RGBaaa rightly. For example, if connect the "innolux,n116bge" > + * eDP screen, EDID would indicated that screen only accepted the > + * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP > + * screen would show a blue picture (RGB888 show a green picture). > + * But if I configure CTRC to RGBaaa, and eDP driver still keep > + * RGB666 input video mode, then screen would works prefect. > + */ > + adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(eDP, AAAA); > + > return true; > } > > @@ -114,27 +128,6 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder) > int ret; > u32 val; > > - /* > - * FIXME(Yakir): driver should configure the CRTC output video > - * mode with the display information which indicated the monitor > - * support colorimetry. > - * > - * But don't know why the CRTC driver seems could only output the > - * RGBaaa rightly. For example, if connect the "innolux,n116bge" > - * eDP screen, EDID would indicated that screen only accepted the > - * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP > - * screen would show a blue picture (RGB888 show a green picture). > - * But if I configure CTRC to RGBaaa, and eDP driver still keep > - * RGB666 input video mode, then screen would works prefect. > - */ > - ret = rockchip_drm_crtc_mode_config(encoder->crtc, > - DRM_MODE_CONNECTOR_eDP, > - ROCKCHIP_OUT_MODE_AAAA); > - if (ret < 0) { > - dev_err(dp->dev, "Could not set crtc mode config (%d)\n", ret); > - return; > - } > - > ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder); > if (ret < 0) > return; > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > index 7975158..49c4c72 100644 > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > @@ -875,11 +875,34 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder) > clk_disable_unprepare(dsi->pclk); > } > > +static bool dw_mipi_dsi_encoder_mode_fixup(struct drm_encoder *encoder, > + const struct drm_display_mode *mode, > + struct drm_display_mode *adjusted_mode) > +{ > + struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder); > + > + switch (dsi->format) { > + case MIPI_DSI_FMT_RGB888: > + adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P888); > + break; > + case MIPI_DSI_FMT_RGB666: > + adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P666); > + break; > + case MIPI_DSI_FMT_RGB565: > + adjusted_mode->private_flags = ROCKCHIP_DSP_MODE(DSI, P565); > + break; > + default: > + WARN_ON(1); > + return false; > + } > + > + return true; > +} > + > 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 interface_pix_fmt; > u32 val; > > if (clk_prepare_enable(dsi->pclk)) { > @@ -895,24 +918,6 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder) > > clk_disable_unprepare(dsi->pclk); > > - switch (dsi->format) { > - case MIPI_DSI_FMT_RGB888: > - interface_pix_fmt = ROCKCHIP_OUT_MODE_P888; > - break; > - case MIPI_DSI_FMT_RGB666: > - interface_pix_fmt = ROCKCHIP_OUT_MODE_P666; > - break; > - case MIPI_DSI_FMT_RGB565: > - interface_pix_fmt = ROCKCHIP_OUT_MODE_P565; > - break; > - default: > - WARN_ON(1); > - return; > - } > - > - rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_DSI, > - interface_pix_fmt); > - > if (mux) > val = DSI0_SEL_VOP_LIT | (DSI0_SEL_VOP_LIT << 16); > else > @@ -924,6 +929,7 @@ static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder) > > static struct drm_encoder_helper_funcs > dw_mipi_dsi_encoder_helper_funcs = { > + .mode_fixup = dw_mipi_dsi_encoder_mode_fixup, > .commit = dw_mipi_dsi_encoder_commit, > .mode_set = dw_mipi_dsi_encoder_mode_set, > .disable = dw_mipi_dsi_encoder_disable, > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > index d5cfef7..a72af30 100644 > --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > @@ -186,6 +186,8 @@ dw_hdmi_rockchip_encoder_mode_fixup(struct drm_encoder *encoder, > const struct drm_display_mode *mode, > struct drm_display_mode *adj_mode) > { > + adj_mode->private_flags = ROCKCHIP_DSP_MODE(HDMIA, AAAA); > + > return true; > } > > @@ -201,9 +203,6 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder) > u32 val; > int mux; > > - rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA, > - ROCKCHIP_OUT_MODE_AAAA); > - > mux = drm_of_encoder_active_endpoint_id(hdmi->dev->of_node, encoder); > if (mux) > val = HDMI_SEL_VOP_LIT | (HDMI_SEL_VOP_LIT << 16); > diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c > index 10d62ff..ef6ad91 100644 > --- a/drivers/gpu/drm/rockchip/inno_hdmi.c > +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c > @@ -500,9 +500,6 @@ static void inno_hdmi_encoder_enable(struct drm_encoder *encoder) > { > struct inno_hdmi *hdmi = to_inno_hdmi(encoder); > > - rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA, > - ROCKCHIP_OUT_MODE_P888); > - > inno_hdmi_set_pwr_mode(hdmi, NORMAL); > } > > @@ -517,6 +514,8 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder, > const struct drm_display_mode *mode, > struct drm_display_mode *adj_mode) > { > + adj_mode->private_flags = ROCKCHIP_DSP_MODE(HDMIA, P888); > + > return true; > } > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h > index 00d17d7..683210b 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h > @@ -68,8 +68,6 @@ void rockchip_drm_atomic_work(struct work_struct *work); > int rockchip_register_crtc_funcs(struct drm_crtc *crtc, > const struct rockchip_crtc_funcs *crtc_funcs); > void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc); > -int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, int connector_type, > - int out_mode); > int rockchip_drm_dma_attach_device(struct drm_device *drm_dev, > struct device *dev); > void rockchip_drm_dma_detach_device(struct drm_device *drm_dev, > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > index a619f12..1a611cf 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > @@ -818,38 +818,6 @@ static const struct drm_plane_funcs vop_plane_funcs = { > .atomic_destroy_state = vop_atomic_plane_destroy_state, > }; > > -int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, > - int connector_type, > - int out_mode) > -{ > - struct vop *vop = to_vop(crtc); > - > - if (WARN_ON(!vop->is_enabled)) > - return -EINVAL; > - > - switch (connector_type) { > - case DRM_MODE_CONNECTOR_LVDS: > - VOP_CTRL_SET(vop, rgb_en, 1); > - break; > - case DRM_MODE_CONNECTOR_eDP: > - VOP_CTRL_SET(vop, edp_en, 1); > - break; > - case DRM_MODE_CONNECTOR_HDMIA: > - VOP_CTRL_SET(vop, hdmi_en, 1); > - break; > - case DRM_MODE_CONNECTOR_DSI: > - VOP_CTRL_SET(vop, mipi_en, 1); > - break; > - default: > - DRM_ERROR("unsupport connector_type[%d]\n", connector_type); > - return -EINVAL; > - }; > - VOP_CTRL_SET(vop, out_mode, out_mode); > - > - return 0; > -} > -EXPORT_SYMBOL_GPL(rockchip_drm_crtc_mode_config); > - > static int vop_crtc_enable_vblank(struct drm_crtc *crtc) > { > struct vop *vop = to_vop(crtc); > @@ -943,6 +911,8 @@ static void vop_crtc_enable(struct drm_crtc *crtc) > u16 vact_st = adjusted_mode->vtotal - adjusted_mode->vsync_start; > u16 vact_end = vact_st + vdisplay; > uint32_t val; > + int type = ROCKCHIP_OUT_MODE_TYPE(adjusted_mode->private_flags); > + int out_mode = ROCKCHIP_OUT_MODE(adjusted_mode->private_flags); > > vop_enable(crtc); > /* > @@ -985,6 +955,23 @@ static void vop_crtc_enable(struct drm_crtc *crtc) > val |= (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC) ? 0 : 1; > val |= (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC) ? 0 : (1 << 1); > VOP_CTRL_SET(vop, pin_pol, val); > + switch (type) { > + case DRM_MODE_CONNECTOR_LVDS: > + VOP_CTRL_SET(vop, rgb_en, 1); > + break; > + case DRM_MODE_CONNECTOR_eDP: > + VOP_CTRL_SET(vop, edp_en, 1); > + break; > + case DRM_MODE_CONNECTOR_HDMIA: > + VOP_CTRL_SET(vop, hdmi_en, 1); > + break; > + case DRM_MODE_CONNECTOR_DSI: > + VOP_CTRL_SET(vop, mipi_en, 1); > + break; > + default: > + DRM_ERROR("unsupport connector_type[%d]\n", type); > + } > + VOP_CTRL_SET(vop, out_mode, out_mode); > > VOP_CTRL_SET(vop, htotal_pw, (htotal << 16) | hsync_len); > val = hact_st << 16; > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > index 071ff0b..4c63a66 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > @@ -183,6 +183,12 @@ struct vop_data { > /* for use special outface */ > #define ROCKCHIP_OUT_MODE_AAAA 15 > > +#define ROCKCHIP_OUT_MODE_TYPE(x) ((x) >> 16) > +#define ROCKCHIP_OUT_MODE(x) ((x) & 0xffff) > +#define ROCKCHIP_DSP_MODE(type, mode) \ > + ((DRM_MODE_CONNECTOR_##type << 16) | \ > + (ROCKCHIP_OUT_MODE_##mode & 0xffff)) > + > enum alpha_mode { > ALPHA_STRAIGHT, > ALPHA_INVERSE,