Re: [PATCH v6 3/3] drm/rockchip: Add ROCKCHIP DW MIPI DSI controller driver

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@rock-chips.com Signed-off-by: Brian Norris briannorris@chromium.org Reviewed-by: Brian Norris briannorris@chromium.org Reviewed-by: Sean Paul seanpaul@chromium.org

---

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