Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754153AbdLGOac (ORCPT ); Thu, 7 Dec 2017 09:30:32 -0500 Received: from mx07-00178001.pphosted.com ([62.209.51.94]:8163 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753762AbdLGOa3 (ORCPT ); Thu, 7 Dec 2017 09:30:29 -0500 From: Philippe CORNU To: Brian Norris , Nickey Yang CC: "robh+dt@kernel.org" , "heiko@sntech.de" , "mark.rutland@arm.com" , "airlied@linux.ie" , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "linux-rockchip@lists.infradead.org" , "laurent.pinchart@ideasonboard.com" , "seanpaul@chromium.org" , "mka@chromium.org" , "hoegsberg@gmail.com" , "architt@codeaurora.org" , Yannick FERTRE , "hl@rock-chips.com" , "zyw@rock-chips.com" , "xbl@rock-chips.com" Subject: Re: [PATCH v6 3/3] drm/rockchip: Add ROCKCHIP DW MIPI DSI controller driver Thread-Topic: [PATCH v6 3/3] drm/rockchip: Add ROCKCHIP DW MIPI DSI controller driver Thread-Index: AQHTbnHm2eNSfHLwrkS1Ff+72S5FiKM2y26AgAEWjgA= Date: Thu, 7 Dec 2017 14:29:43 +0000 Message-ID: References: <1512551301-12946-1-git-send-email-nickey.yang@rock-chips.com> <1512551301-12946-4-git-send-email-nickey.yang@rock-chips.com> <20171206215243.GB14257@google.com> In-Reply-To: <20171206215243.GB14257@google.com> Accept-Language: fr-FR, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.75.127.47] Content-Type: text/plain; charset="utf-8" Content-ID: <882346E769984843BF962A44CA60E9F9@st.com> MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-12-07_05:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id vB7EUeHI027996 Content-Length: 7490 Lines: 210 Hi Brian, On 12/06/2017 10:52 PM, Brian Norris wrote: > Hi Nickey, others, > > I just want to highlight a thing or two here. Otherwise, my > 'Reviewed-by' still basically stands (FWIW). > > On Wed, Dec 06, 2017 at 05:08:21PM +0800, Nickey Yang wrote: >> Add the ROCKCHIP DSI controller driver that uses the Synopsys DesignWare >> MIPI DSI host controller bridge. >> >> Signed-off-by: Nickey Yang >> Signed-off-by: Brian Norris >> Reviewed-by: Brian Norris >> Reviewed-by: Sean Paul >> --- >> change: >> >> v2: >> add err_pllref, remove unnecessary encoder.enable & disable >> correct spelling mistakes >> v3: >> call dw_mipi_dsi_unbind() in dw_mipi_dsi_rockchip_unbind() >> fix typo, use of_device_get_match_data(), >> change some ‘bind()’ logic into 'probe()' >> add 'dev_set_drvdata()' >> v4: >> return -EINVAL when can not get best_freq >> add a clarifying comment when get vco >> add review tag >> v5: >> keep our power domain enabled while touching GRF >> v6: >> change func dw_mipi_encoder_disable name to >> dw_mipi_dsi_encoder_disable > > We noticed a regression w.r.t. pm_runtime_*() handling using this patch, > hence the pm_runtime changes in v5/v6. We actually need to keep our > power domain enabled in the mode_set() function, where we start to > configure some Rockchip-specific registers (GRF). More on that below. > >> >> drivers/gpu/drm/rockchip/Kconfig | 2 +- >> drivers/gpu/drm/rockchip/Makefile | 2 +- >> drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 1349 ----------------------- >> drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c | 785 +++++++++++++ >> drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 2 +- >> drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 2 +- >> 6 files changed, 789 insertions(+), 1353 deletions(-) >> delete mode 100644 drivers/gpu/drm/rockchip/dw-mipi-dsi.c >> create mode 100644 drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c >> > > ... > >> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c >> new file mode 100644 >> index 0000000..66ab6fe >> --- /dev/null >> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c >> @@ -0,0 +1,785 @@ > > ... > >> +static void dw_mipi_dsi_encoder_mode_set(struct drm_encoder *encoder, >> + struct drm_display_mode *mode, >> + struct drm_display_mode *adjusted) >> +{ >> + struct dw_mipi_dsi_rockchip *dsi = to_dsi(encoder); >> + const struct rockchip_dw_dsi_chip_data *cdata = dsi->cdata; >> + int val, ret, mux; >> + >> + mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, >> + &dsi->encoder); >> + if (mux < 0) >> + return; >> + /* >> + * For the RK3399, the clk of grf must be enabled before writing grf >> + * register. And for RK3288 or other soc, this grf_clk must be NULL, >> + * the clk_prepare_enable return true directly. >> + */ >> + ret = clk_prepare_enable(dsi->grf_clk); >> + if (ret) { >> + DRM_DEV_ERROR(dsi->dev, "Failed to enable grf_clk: %d\n", ret); >> + return; >> + } >> + pm_runtime_get_sync(dsi->dev); > > What happens if there's a clk_prepare_enable() failure or failure to > retrieve the endpoint ID earlier in this function? You won't call > pm_runtime_get_*()...but might we still see a call to > dw_mipi_dsi_encoder_disable(), which would mean we get unbalanced > runtime PM status? > > Also (and more importantly), is it fair to do all of this in mode_set()? > I believe Archit asked about this before, and the reason we're doing > this stuff in mode_set() now (where previously, the Rockchip driver was > doing it in ->enable()) is because when Philippe extracted the synopsys > bridge driver, that code migrated to ->mode_set(). Regarding mode_set(), let's me explain more what I did: - transform the original code from Rockchip (encoder + connector drm api) to a generic bridge (bridge drm api). - add panel-bridge interface - ... (more details in the change log https://www.spinics.net/lists/dri-devel/msg147167.html) So we have: crtc -> dw mipi dsi bridge -> panel-bridge "bridge next" -> panel The bridge pre_enable() calls first the "bridge next" pre_enable() (ie the panel-bridge pre-enable) before calling the bridge pre_enable (ie the dw mipi dsi bridge pre-enable (not used here). see in drm_bridge_pre_enable() in drm_bridge.c for details The panel-bridge pre_enable() calls drm_panel_prepare(). See in bridge/panel.c for details. So in the dw mipi dsi bridge, we need to configure and start "everything" before the bridge pre_enable so I put the "former" encoder enable() source code into the new bridge mode_set(). Tell me if I am not clear enough as it is not so easy to explain : ) Thanks, Philippe :-) > > But, I'm reading the comments on drm_encoder_helper_funcs::mode_set, and > I see: > > /** > * @mode_set: > * > * This callback is used to update the display mode of an encoder. > * > * Note that the display pipe is completely off when this function is > * called. Drivers which need hardware to be running before they program > * the new display mode (because they implement runtime PM) should not > * use this hook, because the helper library calls it only once and not > * every time the display pipeline is suspend using either DPMS or the > * new "ACTIVE" property. Such drivers should instead move all their > * encoder setup into the @enable callback. > > That sounds like perhaps the synopsys driver should be moving to use > ->enable() instead, so the Rockchip driver can do that as well? > > At any rate, I haven't found any real problem with using mode_set() so > far, but I was just curious what the recommended practice was. > >> + val = cdata->dsi0_en_bit << 16; >> + if (mux) >> + val |= cdata->dsi0_en_bit; >> + regmap_write(dsi->grf_regmap, cdata->grf_switch_reg, val); >> + >> + if (cdata->grf_dsi0_mode_reg) >> + regmap_write(dsi->grf_regmap, cdata->grf_dsi0_mode_reg, >> + cdata->grf_dsi0_mode); >> + >> + clk_disable_unprepare(dsi->grf_clk); >> +} >> + >> +static int >> +dw_mipi_dsi_encoder_atomic_check(struct drm_encoder *encoder, >> + struct drm_crtc_state *crtc_state, >> + struct drm_connector_state *conn_state) >> +{ >> + struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state); >> + struct dw_mipi_dsi_rockchip *dsi = to_dsi(encoder); >> + >> + switch (dsi->format) { >> + case MIPI_DSI_FMT_RGB888: >> + s->output_mode = ROCKCHIP_OUT_MODE_P888; >> + break; >> + case MIPI_DSI_FMT_RGB666: >> + s->output_mode = ROCKCHIP_OUT_MODE_P666; >> + break; >> + case MIPI_DSI_FMT_RGB565: >> + s->output_mode = ROCKCHIP_OUT_MODE_P565; >> + break; >> + default: >> + WARN_ON(1); >> + return -EINVAL; >> + } >> + >> + s->output_type = DRM_MODE_CONNECTOR_DSI; >> + >> + return 0; >> +} >> + >> +static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder) >> +{ >> + struct dw_mipi_dsi_rockchip *dsi = to_dsi(encoder); >> + >> + pm_runtime_put(dsi->dev); >> +} >> + >> +static const struct drm_encoder_helper_funcs >> +dw_mipi_dsi_encoder_helper_funcs = { >> + .mode_set = dw_mipi_dsi_encoder_mode_set, >> + .atomic_check = dw_mipi_dsi_encoder_atomic_check, >> + .disable = dw_mipi_dsi_encoder_disable, >> +}; > > ... > > Brian >