Hi Brian,
On 2017年12月07日 05:52, 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 <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?
So should we change this to
1、remove dw_mipi_dsi_encoder_disable, and in
dw_mipi_dsi_encoder_mode_set:
drm_of_encoder_active_endpoint_id ->
clk_prepare_enable ->
pm_runtime_get_sync ->
grf_config ->
pm_runtime_put_sync ->
clk_prepare_disable?
or
2、use dw_mipi_dsi_encoder_disable, and in
dw_mipi_dsi_encoder_mode_set:
pm_runtime_get_sync ->
drm_of_encoder_active_endpoint_id ->
clk_prepare_enable ->
grf_config ->
clk_prepare_disable?
and call pm_runtime_put_sync if there is a failure in
drm_of_encoder_active_endpoint_id or clk_prepare_enable
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
