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 <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().
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
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
