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 <nickey.yang@xxxxxxxxxxxxxx> > Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx> > Reviewed-by: Brian Norris <briannorris@xxxxxxxxxxxx> > Reviewed-by: Sean Paul <seanpaul@xxxxxxxxxxxx> > --- > 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(). 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel